Skip to content

[WIP] check that cell crossings does not exceed threshold - #6156

Closed
JustinRayAngus wants to merge 21 commits into
BLAST-WarpX:developmentfrom
JustinRayAngus:villasenor_check_cell_crossings
Closed

[WIP] check that cell crossings does not exceed threshold#6156
JustinRayAngus wants to merge 21 commits into
BLAST-WarpX:developmentfrom
JustinRayAngus:villasenor_check_cell_crossings

Conversation

@JustinRayAngus

@JustinRayAngus JustinRayAngus commented Sep 11, 2025

Copy link
Copy Markdown
Contributor

The implicit solvers permit time steps that violate the CFL condition for particles. Sometimes this results in a particle gathering or depositing from/to an invalid position that is outside the ghost region of the box, which results in a segfault with no information about why the segfault occurred.

This PR adds a check that the number of particle cell crossings in the Villasenor gather/deposit routines is within the number permitted based on the number of ghost cells. The code aborts when a violation occurs with an appropriate error message.

@JustinRayAngus JustinRayAngus added the component: implicit solvers Anything related to implicit solvers label Sep 11, 2025
@JustinRayAngus JustinRayAngus changed the title abort when particle cell crossings exceeds threshold. check that number of particle cell crossings does not exceed the threshold Sep 11, 2025
@JustinRayAngus
JustinRayAngus requested a review from ax3l September 11, 2025 18:41
@JustinRayAngus JustinRayAngus changed the title check that number of particle cell crossings does not exceed the threshold check that cell crossings does not exceed threshold Sep 12, 2025
ax3l pushed a commit that referenced this pull request Sep 12, 2025
PR #6156 is failing a CI test because adding one additional int
parameter in the ImplicitPushXPSubOrbits() routine pushes the kernel
argument size beyond the 2048 byte limit.

This PR is an attempt to cleanup the ImplicitPushXPSubOrbits() function
and to hopefully minimize the size of the kernel argument as much as
possible.

@WeiqunZhang @ax3l @atmyers Any ideas for reducing this kernel argument
size?
@JustinRayAngus
JustinRayAngus force-pushed the villasenor_check_cell_crossings branch from e4f9291 to 991bc90 Compare September 12, 2025 18:45
@JustinRayAngus JustinRayAngus changed the title check that cell crossings does not exceed threshold [WIP] check that cell crossings does not exceed threshold Sep 13, 2025
@JustinRayAngus

Copy link
Copy Markdown
Contributor Author

@atmyers mentioned during a meeting that amrex::numParticlesOutOfRange() should handle the same situation this PR is targeting. We believe it is not catching the out-of-bounds particles for the implicit solver because the particles are at the time-centered position. This may require a separate PR to change it such that the implicit particles own the position at step n+1 rather than n+1/2. For now, this PR is converted to a WIP.

@JustinRayAngus

Copy link
Copy Markdown
Contributor Author

@atmyers mentioned during a meeting that amrex::numParticlesOutOfRange() should handle the same situation this PR is targeting. We believe it is not catching the out-of-bounds particles for the implicit solver because the particles are at the time-centered position. This may require a separate PR to change it such that the implicit particles own the position at step n+1 rather than n+1/2. For now, this PR is converted to a WIP.

Looks like amrex::numParticlesOutOfRange() is only called inside AMREX_ASSERT_WITH_MESSAGE(), which I believe only works when compiled with debug. This PR offers a light way method for checking the particles are within range when using the Villasenor depositions using parameters that are already computed - no extra work outside comparing two int values.

One non-ideal aspect of this check is that it will abort if any particle has a range larger than that permitted by the ghost cells, even in situations where the particle is not near the box boundary and no segfault would occur otherwise. However, probably not that non-ideal because such particles will most likely be at a box boundary eventually.

@JustinRayAngus

JustinRayAngus commented Sep 15, 2025

Copy link
Copy Markdown
Contributor Author

@WeiqunZhang @dpgrote @ax3l Question remains on what to do for GPU, since AMReX::Abort() doesn't work on device.

@dpgrote

dpgrote commented Sep 15, 2025

Copy link
Copy Markdown
Member

@WeiqunZhang @dpgrote @ax3l Question remains on what to do for GPU, since AMReX::Abort() doesn't work on device.

You can do what is done in the implicit push for unconverged particles. Create a Gpu::Buffer to count the number of bad particles, and check it after the kernel is finished.

@JustinRayAngus
JustinRayAngus force-pushed the villasenor_check_cell_crossings branch from 991bc90 to 53f383f Compare September 17, 2025 16:08
@JustinRayAngus JustinRayAngus changed the title [WIP] check that cell crossings does not exceed threshold check that cell crossings does not exceed threshold Sep 17, 2025
@JustinRayAngus
JustinRayAngus force-pushed the villasenor_check_cell_crossings branch from 8ca0223 to 5a98deb Compare September 18, 2025 13:40
@JustinRayAngus JustinRayAngus changed the title check that cell crossings does not exceed threshold [WIP] check that cell crossings does not exceed threshold Sep 18, 2025
@ax3l ax3l added bug Something isn't working bug: affects latest release Bug also exists in latest release version labels Mar 25, 2026
Comment on lines +1606 to +1614
// Abort if cell crossings are larger than permitted
if (cell_crossings_x > max_crossings) {
amrex::Gpu::Atomic::AddNoRet(out_of_range_particles_ptr,1);
#if !defined(AMREX_USE_GPU)
amrex::Abort("Error: cell_crossings_x = " + std::to_string(cell_crossings_x) +
" is greater than the maximum allowed = " +
std::to_string(max_crossings));
#endif
}

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.

This looks quite expensive for hot loops in kernels.

We could consider either checking this only in debug mode or compiling (as a template parameter) to variants that we can select at runtime (one checked and one unchecked).

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.

Thanks for the feedback. This PR has gone somewhat stale. Weiqun had a suggestion above, but for now this is on the back burner. Many more important things to worry about.

RemiLehe pushed a commit that referenced this pull request Sep 1, 2026
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

Copy link
Copy Markdown
Contributor Author

Closing this PR. It was replaced by PR #7225

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug: affects latest release Bug also exists in latest release version bug Something isn't working component: implicit solvers Anything related to implicit solvers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants