Skip to content

Enforce particle orbit bounds for implicit solvers - #7225

Merged
RemiLehe merged 23 commits into
BLAST-WarpX:developmentfrom
JustinRayAngus:mass_matrices_cell_crossings
Sep 1, 2026
Merged

Enforce particle orbit bounds for implicit solvers#7225
RemiLehe merged 23 commits into
BLAST-WarpX:developmentfrom
JustinRayAngus:mass_matrices_cell_crossings

Conversation

@JustinRayAngus

@JustinRayAngus JustinRayAngus commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

This PR adds particle bounds checks prior to field gather and current deposition for the implicit solvers. The checks are included for each suborbit when particle suborbits are used.

This PR also adds checks that the number of cell crossings in each direction are within the bounds determined by particles.max_grid_crossings when using the mass matrices for the Jacobian and using the Villasenor deposition. For that configuration, particles.max_grid_crossings determines that size of the mass matrix containers.

Note that the particle bounds check prior to gather/deposition is not sufficient to ensure that each particle orbit remains within the maximum allowed value set by particles.max_grid_crossings. This is because a particle with a large orbit can be located anywhere within the tilebox, whereas the bounds check only catches particles near the tilebox boundaries.

When a particle is found to be out of bounds, or has too many cell crossings, the simulation aborts. In the future, the abort can be replaced with a trigger to subcycle the implicit step (see PR #7000).

This PR replaces PR #6156.

@JustinRayAngus JustinRayAngus added the component: implicit solvers Anything related to implicit solvers label Aug 30, 2026
@JustinRayAngus
JustinRayAngus requested a review from dpgrote August 30, 2026 20:08
@JustinRayAngus
JustinRayAngus requested a review from atmyers August 31, 2026 15:35
Comment thread Source/Particles/Deposition/MassMatricesDeposition.H Outdated
@JustinRayAngus

JustinRayAngus commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

@dpgrote @atmyers

I may be able to simplify some of the checks I've added here for particles being within range using:

AMREX_ASSERT_WITH_MESSAGE(
    amrex::numParticlesOutOfRange(pti, range) == 0,
    "Particles shape does not fit within tile (CPU) or guard cells (GPU) used for current deposition");

But it's not clear to me what this actually checks. I presume it uses the stored position of the particle and checks that it is within the specified range of the valid region of the box/tile that it lives in. Is that correct?

If yes, then I would need to convert the position from time-centered to time n+1 prior to calling this.

How does it work for curvilinear geometries? Does it convert xp and yp to rp = sqrt(xp^2 + yp^2)?

@JustinRayAngus JustinRayAngus changed the title Enforce particle grid-crossing and suborbit bounds for implicit solvers Enforce particle orbit bounds for implicit solvers Sep 1, 2026
Comment thread Source/Particles/Deposition/MassMatricesDeposition.H Fixed
amrex::Dim3 const & nodal_hi)
{
using namespace amrex::literals;
[[maybe_unused]] amrex::ParticleReal const xp_np1 = 2.0_prt*xp_nph - xp_n;

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 don't know if it matters, but this is duplicating the calculation of xp_np1 here and also of x_new below (excepting lo) when called from the deposition routines. Is there a nice way of handling this to avoid this duplication? This might make this routine simpler, avoiding the code for different geometries. Though maybe not since the pusher routine which calls this doesn't calculate x_new.

Though a related question is why is the pusher routine calling this and not the gather where it matters?

@JustinRayAngus JustinRayAngus Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, there is duplicate code for computing xp_np1. It's not just here though, it's in a lot of places. I've thought about this previously and I think it would be optimal to change the paradigm that the particle position lives at time n + 1/2 during the implicit solve to living at time n + 1. This should be done in a separate PR, as it will be a much bigger change.

Here is what CodeX has to say about putting the check inside doGatherShapeNImplicit.

I would keep the check in PushXPSingleStep, not move it into doGatherShapeNImplicit().

  Reasons:

  - doGatherShapeNImplicit() does not currently know nodal_lo, nodal_hi, or the error counter. Adding them
    would couple a low-level gather routine to pusher error handling.

  - Gathering can be disabled with do_gather == false; the particle-position invariant should still be
    enforced.

  - The final Picard position update may not be followed by another gather, so a gather-only check could miss
    the final invalid position.

  - Deposition also needs the invariant independently, since it is another memory-access boundary.
  - The current placement returns before an unsafe gather and validates every updated position, including the
    final one.

  A check inside the gather could provide extra defensive protection, but it could not replace the pusher and
  deposition checks. It would mainly duplicate them while requiring additional arguments or a changed return
  type.

  The clean ownership is:

  - PushXPSingleStep: validate every trial orbit and stop before gathering.
  - Implicit deposition routines: validate before depositing.
  - Host wrappers: inspect device counters and abort.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@dpgrote The current calls to ParticleUtils::isImplicitParticlePositionInBounds() in ImplicitPushXP.cpp are placed after the particle position updates. This ensures that each particle is in bounds for both the gather and deposition operations, the latter of which occurs locally for suborbit particles.

@JustinRayAngus

JustinRayAngus commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

@dpgrote I could add a boolean flag to ParticleUtils::isImplicitParticlePositionInBounds() that says whether the passed position is to be interpreted at time n + 1/2 or at time n + 1, but I'm not sure that is a good design, or that it really saves that much.

@dpgrote dpgrote left a comment

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.

The code is all good.

@RemiLehe
RemiLehe merged commit 55db96b into BLAST-WarpX:development Sep 1, 2026
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: implicit solvers Anything related to implicit solvers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants