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 periodic boundary bug in #3199 #3243

Merged
merged 3 commits into from
Apr 7, 2023
Merged

Conversation

atmyers
Copy link
Member

@atmyers atmyers commented Apr 7, 2023

PR #3199 broke the Nyx regression tests. The issue is we require the particle positions to be in [plo, phi). This won't happen if rhi is exactly equal to phi. The fix is to make sure rhi is always slightly inside the domain.

EDIT - fixed this in a different way. Now, when applying periodic boundaries, the particle positions will be set to be strictly less than rhi.

The proposed changes:

  • fix a bug or incorrect behavior in AMReX
  • add new capabilities to AMReX
  • changes answers in the test suite to more than roundoff level
  • are likely to significantly affect the results of downstream AMReX users
  • include documentation in the code and/or rst files, if appropriate

@atmyers atmyers requested a review from jmsexton03 April 7, 2023 17:25
@dpgrote
Copy link
Contributor

dpgrote commented Apr 7, 2023

I had setup roundoff_hi to mirror phi intentionally so that the two were consistent. A particle at either roundoff_hi or phi should be considered out of the domain. See in outsideRoundoffDomain that the check for outside is x >= roundoff_hi[0].

@WeiqunZhang
Copy link
Member

Maybe enforcePeriodic in AMReX_ParticleUtil.H needs to be updated. For example, if (p.pos(idim) >= phi[idim]) should be if (p.pos(idim) >= rhi[idim]).

@atmyers
Copy link
Member Author

atmyers commented Apr 7, 2023

In that case, it seems that we need to change the code in enforcePeriodic to make sure the particle position is < rhi after applying periodic boundaries here: https://github.com/AMReX-Codes/amrex/blob/development/Src/Particle/AMReX_ParticleUtil.H#L586

EDIT - crossed in the air with Weiqun.

if (p.pos(idim) > rhi[idim]) {
p.pos(idim) = rhi[idim];
if (p.pos(idim) >= rhi[idim]) {
p.pos(idim) = std::nextafter(p.pos(idim), rlo[idim]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this looks like the right thing to do.

@atmyers atmyers merged commit eab8bee into AMReX-Codes:development Apr 7, 2023
@ax3l
Copy link
Member

ax3l commented Apr 9, 2023

Hi, I think we have a similar test in pyAMReX that breaks with GNU 10.1 now.
When you have a chance, let me know what to update pls :)
AMReX-Codes/pyamrex#120

@ax3l
Copy link
Member

ax3l commented Apr 10, 2023

There seem to be also a series of regressions in WarpX:
ECP-WarpX/WarpX#3822

WeiqunZhang added a commit that referenced this pull request 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:
- #3243
- #3199
guj pushed a commit to guj/amrex that referenced this pull request Jul 13, 2023
PR AMReX-Codes#3199 broke the Nyx regression tests. The issue is we require the
particle positions to be in [plo, phi). This won't happen if rhi is
exactly equal to phi. The fix is to make sure rhi is always slightly
inside the domain.

EDIT - fixed this in a different way. Now, when applying periodic
boundaries, the particle positions will be set to be strictly less than
rhi.
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
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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants