-
Notifications
You must be signed in to change notification settings - Fork 213
Fix random roundoff cylindrical map errors #3430
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
Merged
wthrowe
merged 4 commits into
sxs-collaboration:develop
from
markscheel:FixRandomRoundoffCylErrors
Aug 17, 2021
Merged
Fix random roundoff cylindrical map errors #3430
wthrowe
merged 4 commits into
sxs-collaboration:develop
from
markscheel:FixRandomRoundoffCylErrors
Aug 17, 2021
Conversation
This file contains hidden or 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
This was done because the tests failed by roundoff when the random number generator chose parameters too close to the edges of the allowed region. This passed the test with --repeat-until-fail 100000 Specific changes: - Increase min allowed size of annulus. - Don't let the annulus get too far off center. - Don't allow annulus to be too thick. - Don't allow annulus to be too thin. - Keep proj point away from surface of sphere.
nilsvu
requested changes
Aug 12, 2021
| const std::array<double, 3>& sphere_center, double radius, | ||
| bool pick_larger_root, bool pick_root_greater_than_one) noexcept; | ||
| bool pick_larger_root, bool pick_root_greater_than_one, | ||
| bool solve_for_root_minus_one = true) noexcept; |
Member
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 you add some docs for the added argument to the function-documentation?
Member
|
LGTM, please squash |
Sometimes we might not want to solve for lambda-1 in try_scale_factor. This makes a difference with roundoff.
This was done because the random-number tests that chose
parameters were sometimes choosing parameters that failed
by roundoff.
Now this passes with --repeat-until-fail 100000
The particular changes:
- FocallyLiftedMapHelpers::try_scale_factor now has a choice when
setting up the quadratic equation to solve for lambda. It can
choose to solve for lambda-1 (the default, and the previous
behavior), or it can solve directly for lambda. The difference is
only roundoff but makes a difference in roundoff tests.
- Accordingly, FocallyLiftedSide, when solving for lambda_tide, now
sets up the quadratic to solve for lambda_tilde instead of
lambda_tilde-1.
- We separated out a particular use case: projection point exactly on
one of the z-planes when sphere_two > sphere_one. This use case is
now tested tested separately, and has different allowed parameters.
Changes in the allowed parameters for the (now 3) different cases:
- For sphere_two > sphere_one and projection point on the z-plane:
- center_one must have the same x and y coordinates as center_two.
- Rho must be zero (to roundoff)
- The z-planes must be farther from the edge of sphere_one.
- Radius_two is allowed to be larger, and radius_one cannot be
too small.
- For sphere_two > sphere_one and projection point not on the z-plane:
- rho must be smaller than previously allowed.
- radius_one cannot be too small and radius_two cannot be too large.
- proj_center must be sufficiently far from the z-planes.
- For sphere_two < sphere_one
- Keep projection_point far enough from surface of sphere_two
- Keep sphere_one and sphere_two sufficiently far apart.
- Keep the z-planes sufficiently far from the centers or edges of
sphere_one.
39e82d4 to
b38046f
Compare
Contributor
Author
|
squashed |
Member
Member
|
I'll take it, but you should submit an approval @nilsleiffischer. |
nilsvu
approved these changes
Aug 16, 2021
wthrowe
approved these changes
Aug 16, 2021
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
Change allowed bounds in cylinder domains to fix occasional roundoff failures in tests.
Fixes #3380
See detailed comments in commit messages.
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