-
Notifications
You must be signed in to change notification settings - Fork 189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Exporter: allow extrapolation into excisions #6056
Exporter: allow extrapolation into excisions #6056
Conversation
e8dd821
to
25febf1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exporter test failed on one of the CI jobs.
const double radial_distance_this_block = | ||
x_logical->get(direction.dimension()) * direction.sign(); | ||
if (radial_distance_this_block < 1.) { | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I follow the logic correctly, if we hit this case the point is not inside the excision sphere. Should that be an error or an early return?
* The point will be reported to belong to one of the blocks that abut the | ||
* excision sphere and will have a logical coordinate outside the range [-1, 1] | ||
* in the radial direction. This is useful for extrapolating data into excision | ||
* regions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if input_point
is not inside excision_sphere
? Either document the result or state it is considered invalid to call the function in that case.
continue; | ||
} | ||
// The checks above should leave only 1 valid block, so return that | ||
return make_id_pair(domain::BlockId(block_id), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could there be two valid blocks for points on the extrapolation of the boundary between two blocks into the sphere? The blocks are being pulled from an unordered container, so always choosing the first one isn't deterministic.
src/IO/Exporter/Exporter.cpp
Outdated
for (size_t s = 0; s < block_logical_coords->size(); ++s) { | ||
auto& block_logical_coord = (*block_logical_coords)[s]; | ||
// Process only points inside the excision sphere. These points were found | ||
// by `block_logical_coordinates` above, have a radial logical coordinate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
block_logical_coordinates
doesn't appear anywhere above.
* \cite Etienne2008re adjusted for distorted excisions: we choose uniformly | ||
* spaced radial anchor points spaced as $\Delta r = 0.3 r_\mathrm{AH}$ in the | ||
* grid frame (where the excision is spherical), then map the anchor points to | ||
* the distorted frame (where we have the target point) and do a 7th order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code has constexpr size_t extrapolation_order = 8;
Thanks for your review @wthrowe! I pushed a fixup. |
I'm investigating the NaNs in some of the unit tests, but the fixup is ready for your review @wthrowe |
// range [-1, 1] | ||
// range [-1, 1]. Also discard if the point is on the upper angular face of | ||
// the block, to disambiguate which of the two blocks sharing the face the | ||
// point is in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work. The blocks don't have aligned frames, so some interfaces are two lower sides or two upper sides.
src/IO/Exporter/Exporter.hpp
Outdated
* the distorted frame (where we have the target point) and do a 7th order | ||
* polynomial extrapolation into the excision region. | ||
* the distorted frame (where we have the target point) and do a polynomial | ||
* extrapolation into the excision region. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it 8th order or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a 7th order polynomial with 8 collocation points. However in the polynomial_interpolation
function the name order
is degree + 1
, so I used that naming here as well:
constexpr size_t order = Degree + 1; |
To avoid confusion I renamed to
num_extrapolation_anchors
.
a27ab51
to
3369b36
Compare
3369b36
to
779a034
Compare
@wthrowe I pushed a new fixup. I dropped the changes to |
OK, looks good. Squash and fix the clang-tidy complaint. |
This can be useful to fill the excision region with (non-constraint-satisfying) data so it can be imported into moving puncture codes.
779a034
to
af2f718
Compare
I don't see a connection 🤷♂️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Let's let the rerun finish, but feel free to merge once that's done (or I'll do it later, but I'm leaving for the day soon).
Proposed changes
This is needed to fill the excision region with (non-constraint-satisfying) data so it can be imported into moving puncture codes.
Upgrade instructions
Code review checklist
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.
bugfix
ornew feature
if appropriate.Further comments