Rework handling of roundoff domain extent #3199
Merged
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
dpgrote
changed the title
dpgrote
changed the title
dpgrote
changed the title
atmyers
approved these changes
Apr 5, 2023
atmyers
reviewed
Apr 6, 2023
|
Thank you for this! |
5 tasks
atmyers
added a commit
that referenced
this pull request
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.
This was referenced Apr 10, 2023
Merged
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
## Summary This reworks previous changes that created the round off domain which was done to avoid round off errors when locating particles in the domain, specifically when handling periodic boundaries. This PR changes how this is done so that the roundoff domain makes as small a change as possible. ## Additional background Previous PRs AMReX-Codes#2839 and AMReX-Codes#2950 created a round off domain that has a slightly smaller extent that the actual domain. Any particles that ended up outside the round off domain but inside the actual domain were shifted to be on the boundary of the round off domain. The main purpose was to handle problems with periodic boundary with mixed precision (single precision particles and double precision grid). In that case, it is possible for a particle to have `z < lo` and `z+(hi-lo) > hi`. It is also possible for a particle to have `z < hi` but `(z-lo)*dzinv > ihi` putting it in a grid cell beyond the domain. Unfortunately, the roundoff domains was being set further inside that was needed and would affect valid particles that would not have those issues. While the problem is small, the effects can be noticeable when examining simulation properties, for example charge conservation in WarpX. This PR changes the roundoff domain so that only problematic particles are affected. The code now uses `std::nextafter` to find the exact place where the problems occurs. At the lower end, it ensures that the casted position will be greater than ProbLo. At the upper end, it ensures that `(z-lo)*dzinv <= ihi`. One comment is that this assumes that particle grid coordinates will always be calculated at the higher precision, with expressions like `iz = (int)(z - lo)`, without `lo` being cast to `ParticleReal`. Co-authored-by: atmyers <[email protected]>
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
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.
Summary
This reworks previous changes that created the round off domain which was done to avoid round off errors when locating particles in the domain, specifically when handling periodic boundaries. This PR changes how this is done so that the roundoff domain makes as small a change as possible.
Additional background
Previous PRs #2839 and #2950 created a round off domain that has a slightly smaller extent that the actual domain. Any particles that ended up outside the round off domain but inside the actual domain were shifted to be on the boundary of the round off domain. The main purpose was to handle problems with periodic boundary with mixed precision (single precision particles and double precision grid). In that case, it is possible for a particle to have
z < loandz+(hi-lo) > hi. It is also possible for a particle to havez < hibut(z-lo)*dzinv > ihiputting it in a grid cell beyond the domain. Unfortunately, the roundoff domains was being set further inside that was needed and would affect valid particles that would not have those issues. While the problem is small, the effects can be noticeable when examining simulation properties, for example charge conservation in WarpX.This PR changes the roundoff domain so that only problematic particles are affected. The code now uses
std::nextafterto find the exact place where the problems occurs. At the lower end, it ensures that the casted position will be greater than ProbLo. At the upper end, it ensures that(z-lo)*dzinv <= ihi.One comment is that this assumes that particle grid coordinates will always be calculated at the higher precision, with expressions like
iz = (int)(z - lo), withoutlobeing cast toParticleReal.Checklist
The proposed changes: