Skip to content
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

Fix Roundoff Domain #3247

Merged

Conversation

WeiqunZhang
Copy link
Member

@WeiqunZhang WeiqunZhang commented Apr 10, 2023

There was a flaw in amrex::Geometry::computeRoundoffDomain. If probhi is close to zero, std::nextafter towards negative infinity will be a very small number (e.g., 1.e-300). So the function will be able to find the lowest value of roundoff_hi that's outside the domain within a reasonable number of iterations.

In the new implementation, we use bisection to find the lowest value of roundoff_lo that's inside the domain and the highest value fo roundoff_hi that's inside the domain, up to a tolerance.

X-ref:

@WeiqunZhang
Copy link
Member Author

A test is here. https://github.com/WeiqunZhang/amrex-devtests/blob/main/roundoff_domain/main.cpp It takes up to 53 iterations.

Need to test with different compiler optimization flags and fp model flags.

Also need to test single precision and mixed precision.

@WeiqunZhang WeiqunZhang force-pushed the fix_roundoff_domain branch 2 times, most recently from a060252 to fb3b6d4 Compare April 10, 2023 06:03
@WeiqunZhang
Copy link
Member Author

Using std::nextafter seems very fragile. Switch to a tolerance based on std::numeric_limits::epsilon now. The max number of iterations in the test is now 17.

@WeiqunZhang WeiqunZhang force-pushed the fix_roundoff_domain branch 2 times, most recently from aefa307 to 3d89977 Compare April 10, 2023 17:30
There was a flaw in amrex::Geometry::computeRoundoffDomain. If probhi is
close to zero, std::nextafter towards negative infinity will be a very small
number (e.g., 1.e-300). So the function will be able to find the lowest
value of roundoff_hi that's outside the domain within a reasonable number of
iterations.

In the new implementation, we use bisection to find the lowest value of
roundoff_lo that's inside the domain and the highest value fo roundoff_hi
that's inside the domain, up to a tolerance.
@WeiqunZhang WeiqunZhang marked this pull request as ready for review April 10, 2023 19:52
@ax3l ax3l added the bug label Apr 10, 2023
@WeiqunZhang WeiqunZhang merged commit bb47ec9 into AMReX-Codes:development Apr 10, 2023
@WeiqunZhang WeiqunZhang deleted the fix_roundoff_domain branch April 10, 2023 22:50
WeiqunZhang added a commit to WeiqunZhang/amrex that referenced this pull request Apr 10, 2023
Also fix an assertion issue in AMReX-Codes#3247.
WeiqunZhang added a commit to WeiqunZhang/amrex that referenced this pull request Apr 10, 2023
Also fix an assertion issue in AMReX-Codes#3247.
WeiqunZhang added a commit to WeiqunZhang/amrex that referenced this pull request Apr 10, 2023
Also fix an assertion issue in AMReX-Codes#3247.
WeiqunZhang added a commit to WeiqunZhang/amrex that referenced this pull request Apr 11, 2023
Also fix an assertion issue in AMReX-Codes#3247.
WeiqunZhang added a commit to WeiqunZhang/amrex that referenced this pull request Apr 11, 2023
Also fix an assertion issue in AMReX-Codes#3247.
WeiqunZhang added a commit to WeiqunZhang/amrex that referenced this pull request Apr 11, 2023
Also fix an assertion issue in AMReX-Codes#3247.
WeiqunZhang added a commit to WeiqunZhang/amrex that referenced this pull request Apr 11, 2023
Also fix an assertion issue in AMReX-Codes#3247.
WeiqunZhang added a commit to WeiqunZhang/amrex that referenced this pull request Apr 11, 2023
The new test exposes an issue with Intel compiler. So we have to rewrite the
computeRoundoffDomain slightly.

Also fix an assertion issue in AMReX-Codes#3247.
ax3l pushed a commit that referenced this pull request Apr 11, 2023
The new test exposes an issue with Intel compiler. So we have to rewrite
the computeRoundofDomain function in a slightly different way to avoid
roundoff difference between `auto r = f(x-y)` and `z = x-y; auto r =
f(z)`.

Also fix an assertion issue in #3247.
guj pushed a commit to guj/amrex that referenced this pull request Jul 13, 2023
There was a flaw in amrex::Geometry::computeRoundoffDomain. If probhi is
close to zero, std::nextafter towards negative infinity will be a very
small number (e.g., 1.e-300). So the function will be able to find the
lowest value of roundoff_hi that's outside the domain within a
reasonable number of iterations.

In the new implementation, we use bisection to find the lowest value of
roundoff_lo that's inside the domain and the highest value fo
roundoff_hi that's inside the domain, up to a tolerance.

X-ref:
- AMReX-Codes#3243
- AMReX-Codes#3199
guj pushed a commit to guj/amrex that referenced this pull request Jul 13, 2023
The new test exposes an issue with Intel compiler. So we have to rewrite
the computeRoundofDomain function in a slightly different way to avoid
roundoff difference between `auto r = f(x-y)` and `z = x-y; auto r =
f(z)`.

Also fix an assertion issue in AMReX-Codes#3247.
WeiqunZhang pushed a commit to WeiqunZhang/amrex that referenced this pull request Sep 28, 2023
  * Rework handling of roundoff domain extent (AMReX-Codes#3199)

  * Fix periodic boundary bug in AMReX-Codes#3199 (AMReX-Codes#3243)

  * Fix Roundoff Domain (AMReX-Codes#3247)

  * Add Tests/RoundoffDomain (AMReX-Codes#3248)

  * Roundoff errors in computeRoundoffDomain (AMReX-Codes#3255)
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.

3 participants