Fix interpolation stencil misalignment in RotatingStar
#6260
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wthrowe
requested changes
Aug 30, 2024
| }; | ||
| if (target_abs_cos_theta > cos_theta_[angular_index]) { | ||
| angular_stencil_adjust++; | ||
| }; |
There was a problem hiding this comment.
[optional] Two extra semicolons.
There was a problem hiding this comment.
Squashed the semicolons immediately. Added the unit test in a fixup commit. The test attempts to interpolate at a point which would've caused the original stencil construction to fail (I tested it with the old method and it did indeed fail).
ncorsobh
force-pushed
the
RotatingStarFix
branch
2 times, most recently
from
4437f7d
to
a0ae0b9
Compare
|
Looks good. Squash. |
ncorsobh
force-pushed
the
RotatingStarFix
branch
from
a0ae0b9
to
9ebd612
Compare
wthrowe
approved these changes
Sep 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
In
RotatingStar, the setup of the stencils for the interpolation of theRotNSdata was slightly offset. This resulted in rare edge cases where the target point could fall outside of the stencil if the rounding behaved unfavorably, especially at high resolutions. However, even when the target point fell inside the stencil, it would likely be closer to its upper edge.This PR both fixes the rounding issue that resulted in the target point falling outside of the stencil and improves on the stencil construction overall. Now, if the stencil size is an even number, the target point will always be placed between the middle two points of the stencil, and if the stencil size is an odd number, the target point will be closest to the central point in the stencil.
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