Skip to content

Clamp particles shifted from plo boundary against rhi, rather than back to plo#2814

Merged
atmyers merged 1 commit intoAMReX-Codes:developmentfrom
PhilMiller:pm/periodic
Jun 14, 2022
Merged

Clamp particles shifted from plo boundary against rhi, rather than back to plo#2814
atmyers merged 1 commit intoAMReX-Codes:developmentfrom
PhilMiller:pm/periodic

Conversation

@PhilMiller
Copy link
Copy Markdown
Contributor

@PhilMiller PhilMiller commented Jun 7, 2022

Summary

BLAST-WarpX/warpx#3155
Particles found outside the low end of the periodic boundary were shifted up by the domain size to be close to the high end boundary. If they were outside the rounding domain, and would thus trip up integer index calculations, they were being clamped back to the low end of the domain, rather than the high end. Looking again, I think this was actually just erroneous.

Additional background

See #2679 (comment)

Checklist

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

@PhilMiller
Copy link
Copy Markdown
Contributor Author

@ax3l ax3l requested review from WeiqunZhang and atmyers June 8, 2022 23:07
@ax3l ax3l added the bug label Jun 8, 2022
}
// clamp to avoid precision issues;
if (p.pos(idim) >= rhi[idim]) {
p.pos(idim) = static_cast<ParticleReal>(plo[idim]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@atmyers I am guessing the reason for the old approach is rhi might be >= phi, which would fail the assertion later in this function. Maybe we should change function computeRoundoffDomain to make sure rhi < phi. We could test if phi - tolerance passes the test. If so, we don't need to the bisect. If not, we use that as the upper bound for bisect.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I will address this in a follow-on PR, where I'll also handle having both a roundoff_hi_d and roundoff_hi_f

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.

4 participants