Add wedge transition for shape map #5616
Conversation
knelli2
force-pushed
the
wedge_transition
branch
from
010e3ef
to
be96c3e
Compare
| // class is not to be used otherwise. | ||
| class SphereTransition : public Sphere {}; |
There was a problem hiding this comment.
Not super happy about this, but I would like to rename the class because the original name is redundant. If reviewers are strongly against this, I can forgo the name change
There was a problem hiding this comment.
Renaming seems fine. We could add a way to override the name used for the charm registration if we hit this often.
That said, are you sure this doesn't need its own my_PUP_ID and similar?
There was a problem hiding this comment.
The reason I added this was because of the versioning test in Test_Domain where we deserialize a BCO domain from H5 that was serialized with the old transition function name. Not sure if there's a good way to override charm registration name when reading something in that was already written. Open to suggestions
There was a problem hiding this comment.
I meant we could provide a way to override the second argument in
PUPable_reg2(class_to_register,
registration_name<class_to_register>().c_str());but if it's only for one thing probably not worth it.
Ping on the question about my_PUP_ID.
There was a problem hiding this comment.
I tried compiling with
class SphereTransition : public Sphere {};
PUP::able::PUP_ID SphereTransition::my_PUP_ID = 0;and got an error about no member named 'my_PUP_ID'. The goal is to have the SphereTransition class be identical to the Sphere class in every way except name, so I think it makes sense to not give it its own PUP_ID.
There was a problem hiding this comment.
Yes, you'd need to add the stuff in the class body as well.
I think it makes sense to not give it its own PUP_ID.
But you are assigning two PUP_IDs, since you register twice, just one is overwriting the other. Looking through the charm code I think this has the effect that you cannot serialize the first one registered through a base class pointer. Or maybe you can but it is recorded as being the other type, not sure. If you want to do this you definitely need tests that the serialization works the way you expect it to.
There was a problem hiding this comment.
Hmm ok. This is getting too complicated and I'd rather not run the risk of breaking serialization things like this, so I'll just forgo the name change. It's not a huge deal, I'll just name future transition functions properly. If there ever comes a time where we drop support for certain versions of maps, then we can revisit this.
knelli2
force-pushed
the
wedge_transition
branch
from
be96c3e
to
86a3bdd
Compare
| @@ -95,6 +95,11 @@ class OrientationMap { | |||
| /// The corresponding Orientation of the host in the frame of the neighbor. | |||
| OrientationMap<VolumeDim> inverse_map() const; | |||
|
|
|||
| /// The stored map directions | |||
| const std::array<Direction<VolumeDim>, VolumeDim>& mapped_directions() const { | |||
There was a problem hiding this comment.
If you're exposing this, document what it means. I think dropping this commit and using operator() is clearer, though.
| // class is not to be used otherwise. | ||
| class SphereTransition : public Sphere {}; |
There was a problem hiding this comment.
Renaming seems fine. We could add a way to override the name used for the charm registration if we hit this often.
That said, are you sure this doesn't need its own my_PUP_ID and similar?
| @@ -0,0 +1,7 @@ | |||
| # Distributed under the MIT License. | |||
There was a problem hiding this comment.
Add directory to parent CMakeLists.txt. Looks like done in wrong commit.
| Surface this_wedge_outer_surface_{}; | ||
| OrientationMap<3> orientation_map_{}; | ||
| Direction<3> direction_{}; | ||
| static constexpr double eps_ = std::numeric_limits<double>::epsilon() * 100; |
| double radius{}; | ||
| double sphericity{}; | ||
| template <typename T> | ||
| T distance(const std::array<T, 3>& coords) const; |
| // can be rotated from their original orientation to now "aligned" with +z. | ||
| // Therefore we can continue with computing the original radius. | ||
| const std::array<double, 3> rotated_coords = | ||
| discrete_rotation(orientation_map_.inverse_map(), target_coords); |
There was a problem hiding this comment.
You could rotate first, and then you only have to check if you're in the +z wedge.
| CAPTURE_FOR_ERROR(rotated_coords); | ||
|
|
||
| // If distorted radius is 0, this means the map is the identity so the radius | ||
| // and the original radius are equal. Also we don't want to divide by 0 below |
There was a problem hiding this comment.
I'm confused about how the radius can be 0 inside a wedge.
There was a problem hiding this comment.
Ah, this is a poor name for the variable then. The "distorted radius" is
| // because of the above check. | ||
| if (overall_outer_surface_.sphericity == 1.0) { | ||
| const T total_factor = one_over_denom * outer_distance_minus_radius * | ||
| make_factor(overall_inner_surface_); |
There was a problem hiding this comment.
Since you seem concerned with such things in this function, pretty sure T total_factor = make_factor(...); total_factor *= ...; saves you an allocation.
| result[2] += total_factor * (square(radius) - square(rotated_coords[2])); | ||
| } else if (overall_inner_surface_.sphericity == 1.0) { | ||
| std::array<T, 3> outer_deriv_distance = | ||
| make_array<3>(make_factor(overall_outer_surface_)); |
There was a problem hiding this comment.
Rewriting this like the previous case where you compute an overall factor instead of an array will save allocations and FLOPs.
Probably also for the case below, but I haven't worked that one out in enough detail to be sure.
| // checks that the magnitudes are all between `r_min_` and `r_max_` | ||
| template <typename T> | ||
| void Wedge::check_distances([ | ||
| [maybe_unused]] const std::array<T, 3>& coords) const { |
There was a problem hiding this comment.
Is this actually what clang-format wants? Yuck.
|
@wthrowe Pushed a fixup. I'll remove that one commit and fix the wrong commit CMakeLists.txt thing when I squash |
| // class is not to be used otherwise. | ||
| class SphereTransition : public Sphere {}; |
There was a problem hiding this comment.
I meant we could provide a way to override the second argument in
PUPable_reg2(class_to_register,
registration_name<class_to_register>().c_str());but if it's only for one thing probably not worth it.
Ping on the question about my_PUP_ID.
knelli2
force-pushed
the
wedge_transition
branch
from
c6dad6b
to
53ba065
Compare
tests/Unit/Domain/CoordinateMaps/TimeDependent/ShapeMapTransitionFunctions/Test_Wedge.cpp
Outdated
Show resolved
Hide resolved
src/Domain/CoordinateMaps/TimeDependent/ShapeMapTransitionFunctions/Wedge.hpp
Outdated
Show resolved
Hide resolved
knelli2
force-pushed
the
wedge_transition
branch
from
53ba065
to
9aac01b
Compare
knelli2
force-pushed
the
wedge_transition
branch
from
9aac01b
to
f565075
Compare
|
Please don't squash fixups before I have a chance to look at them. |
|
Oh sorry @wthrowe that's totally my bad. I can |
Proposed changes
A new transition function for the shape map that is to be used in concentric wedge blocks. The inner and outer boundaries of these wedges can be spherical, flat, or in-between just like the Wedge coordinate map. This transition will mostly be used for the rectangular BinaryCompactObject domain for the shells and cubes around each object (PR to come for that).
Needed for #5600.
Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments