Skip to content

Conversation

@markscheel
Copy link
Contributor

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

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Further comments

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.
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;
Copy link
Member

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?

@nilsvu
Copy link
Member

nilsvu commented Aug 16, 2021

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.
@markscheel markscheel force-pushed the FixRandomRoundoffCylErrors branch from 39e82d4 to b38046f Compare August 16, 2021 20:09
@markscheel
Copy link
Contributor Author

squashed

@nilsvu
Copy link
Member

nilsvu commented Aug 16, 2021

@kidder @wthrowe Would one of you take a brief look and merge this?

@wthrowe
Copy link
Member

wthrowe commented Aug 16, 2021

I'll take it, but you should submit an approval @nilsleiffischer.

@wthrowe wthrowe merged commit d9d0936 into sxs-collaboration:develop Aug 17, 2021
@markscheel markscheel deleted the FixRandomRoundoffCylErrors branch August 27, 2021 21:24
@nilsvu nilsvu added the bugfix label Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Approx error in Unit.Domain.CoordinateMaps.CylindricalFlatSide test

3 participants