Improving comments on current deposition algorithms and fixing bug in ghost cell folding inside direct current deposition algo#77
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates documentation/comments around the Esirkepov and J_from_rhov current deposition implementations and adjusts the periodic-boundary test expectations to match corrected ghost-cell folding behavior in the direct (from rhov) deposition path.
Changes:
- Updated
test_J_from_rhov_periodic_boundary_crossingto assert the expected split of deposited current between the first and last physical faces after periodic folding. - Removed the staggered-periodic ghost remapping in
J_from_rhovso periodic seam handling is owned byfold_ghost_cells. - Added/expanded inline comments in
J_from_rhovandEsirkepov_currentfor algorithm readability.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/particle_test.py | Updates the periodic-boundary crossing current test to validate the corrected folding behavior with explicit expected contributions. |
| PyPIC3D/deposition/J_from_rhov.py | Removes periodic ghost remap helper and adds clarifying comments around stencil prep/folding/filtering. |
| PyPIC3D/deposition/Esirkepov.py | Adds inline explanatory comments for the Esirkepov deposition flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+29
to
+37
| x_active = axis_has_active_cells(Nx, ghost_cells=True) | ||
| y_active = axis_has_active_cells(Ny, ghost_cells=True) | ||
| z_active = axis_has_active_cells(Nz, ghost_cells=True) | ||
| # determine which axes are null (N = 1) and thus can be skipped in loops to | ||
| # reduce computational cost for 2D and 1D simulations | ||
| bc_x = world["boundary_conditions"]["x"] | ||
| bc_y = world["boundary_conditions"]["y"] | ||
| bc_z = world["boundary_conditions"]["z"] | ||
| # determine the boundary conditions for each axis to handle ghost cells correctly |
| indexing="ij", | ||
| ) | ||
| combos = jnp.stack([ii.ravel(), jj.ravel(), kk.ravel()], axis=1) | ||
| # get the list of indicies to map over |
| oxw = [tmp, oxw[0], oxw[1], oxw[2], tmp] | ||
| oyw = [tmp, oyw[0], oyw[1], oyw[2], tmp] | ||
| ozw = [tmp, ozw[0], ozw[1], ozw[2], tmp] | ||
| # pad the weights with zeros so they can rolled. |
Comment on lines
+139
to
+140
| # if the axis has ghost cells, collapse the stencil to combine contributions from ghost cells into the corresponding interior cells. | ||
|
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment on lines
68
to
+84
| @@ -82,7 +77,11 @@ def J_from_rhov(particles, J, constants, world, grid=None, filter="bilinear"): | |||
| bc_y, | |||
| wind=world["y_wind"], | |||
| ghost_cells=True, | |||
| ) | |||
| ) | |||
| # prepare the particle axis stencil for the y-axis, which includes determining the grid points that | |||
| # contribute to the deposition and the corresponding weights based on the particle's position and | |||
| # shape factor. This is done for all three axes (x, y, z) to handle 3D deposition correctly. | |||
|
|
|||
| indexing="ij", | ||
| ) | ||
| combos = jnp.stack([ii.ravel(), jj.ravel(), kk.ravel()], axis=1) | ||
| # get the list of indicies to map over |
Comment on lines
95
to
+103
| x0 = compute_particle_anchor(x, grid[0], shape_factor) | ||
| y0 = compute_particle_anchor(y, grid[1], shape_factor) | ||
| z0 = compute_particle_anchor(z, grid[2], shape_factor) | ||
| # compute the nearest grid point to the particle's current position | ||
|
|
||
| old_x0 = compute_particle_anchor(old_x, grid[0], shape_factor) | ||
| old_y0 = compute_particle_anchor(old_y, grid[1], shape_factor) | ||
| old_z0 = compute_particle_anchor(old_z, grid[2], shape_factor) | ||
| # compute the nearest grid point to the particle's old position |
| oxw = [tmp, oxw[0], oxw[1], oxw[2], tmp] | ||
| oyw = [tmp, oyw[0], oyw[1], oyw[2], tmp] | ||
| ozw = [tmp, ozw[0], ozw[1], ozw[2], tmp] | ||
| # pad the weights with zeros so they can rolled. |
Comment on lines
+29
to
+37
| x_active = axis_has_active_cells(Nx, ghost_cells=True) | ||
| y_active = axis_has_active_cells(Ny, ghost_cells=True) | ||
| z_active = axis_has_active_cells(Nz, ghost_cells=True) | ||
| # determine which axes are null (N = 1) and thus can be skipped in loops to | ||
| # reduce computational cost for 2D and 1D simulations | ||
| bc_x = world["boundary_conditions"]["x"] | ||
| bc_y = world["boundary_conditions"]["y"] | ||
| bc_z = world["boundary_conditions"]["z"] | ||
| # determine the boundary conditions for each axis to handle ghost cells correctly |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
PyPIC3D/deposition/J_from_rhov.py:95
- There is trailing whitespace after the closing parenthesis on this line, which can cause noisy diffs and may fail whitespace/formatting checks. Please remove the extra spaces.
bc_z,
wind=world["z_wind"],
ghost_cells=True,
)
# prepare the particle axis stencil for the z-axis, which includes determining the grid points that
# contribute to the deposition and the corresponding weights based on the particle's position and
Comment on lines
+29
to
+37
| x_active = axis_has_active_cells(Nx, ghost_cells=True) | ||
| y_active = axis_has_active_cells(Ny, ghost_cells=True) | ||
| z_active = axis_has_active_cells(Nz, ghost_cells=True) | ||
| # determine which axes are null (N = 1) and thus can be skipped in loops to | ||
| # reduce computational cost for 2D and 1D simulations | ||
| bc_x = world["boundary_conditions"]["x"] | ||
| bc_y = world["boundary_conditions"]["y"] | ||
| bc_z = world["boundary_conditions"]["z"] | ||
| # determine the boundary conditions for each axis to handle ghost cells correctly |
Comment on lines
95
to
+103
| x0 = compute_particle_anchor(x, grid[0], shape_factor) | ||
| y0 = compute_particle_anchor(y, grid[1], shape_factor) | ||
| z0 = compute_particle_anchor(z, grid[2], shape_factor) | ||
| # compute the nearest grid point to the particle's current position | ||
|
|
||
| old_x0 = compute_particle_anchor(old_x, grid[0], shape_factor) | ||
| old_y0 = compute_particle_anchor(old_y, grid[1], shape_factor) | ||
| old_z0 = compute_particle_anchor(old_z, grid[2], shape_factor) | ||
| # compute the nearest grid point to the particle's old position |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment on lines
+29
to
+33
| x_active = axis_has_active_cells(Nx, ghost_cells=True) | ||
| y_active = axis_has_active_cells(Ny, ghost_cells=True) | ||
| z_active = axis_has_active_cells(Nz, ghost_cells=True) | ||
| # determine which axes are null (N = 1) and thus can be skipped in loops to | ||
| # reduce computational cost for 2D and 1D simulations |
Comment on lines
95
to
+103
| x0 = compute_particle_anchor(x, grid[0], shape_factor) | ||
| y0 = compute_particle_anchor(y, grid[1], shape_factor) | ||
| z0 = compute_particle_anchor(z, grid[2], shape_factor) | ||
| # compute the nearest grid point to the particle's current position | ||
|
|
||
| old_x0 = compute_particle_anchor(old_x, grid[0], shape_factor) | ||
| old_y0 = compute_particle_anchor(old_y, grid[1], shape_factor) | ||
| old_z0 = compute_particle_anchor(old_z, grid[2], shape_factor) | ||
| # compute the nearest grid point to the particle's old position |
Member
Author
|
Also implementing the Higuera-Cary particle pusher and refactoring particle pusher logic. |
Comment on lines
+28
to
+33
| @@ -45,25 +29,32 @@ def J_from_rhov(particles, J, constants, world, grid=None, filter="bilinear"): | |||
| x_active = axis_has_active_cells(Nx, ghost_cells=True) | |||
| y_active = axis_has_active_cells(Ny, ghost_cells=True) | |||
| z_active = axis_has_active_cells(Nz, ghost_cells=True) | |||
| # determine which axes are null (N = 1) and thus can be skipped in loops to | |||
| # reduce computational cost for 2D and 1D simulations | |||
Comment on lines
+16
to
+24
| """ | ||
| Updates the velocity of a single particle using the Boris algorithm. | ||
| Args: | ||
| x (float): Initial x position of the particle. | ||
| y (float): Initial y position of the particle. | ||
| z (float): Initial z position of the particle. | ||
| vx (float): Initial x component of the particle's velocity. | ||
| vy (float): Initial y component of the particle's velocity. | ||
| vz (float): Initial z component of the particle's velocity. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
PyPIC3D/deposition/J_from_rhov.py:10
axis_has_active_cellsis imported but no longer used in this function after the refactor. Please remove the unused import to avoid lint failures and keep the module clean.
from PyPIC3D.boundary_conditions.grid_and_stencil import (
axis_has_active_cells,
collapse_axis_stencil,
prepare_particle_axis_stencil,
)
tests/boris_test.py:205
assertGreater(newvx[1], newvx[0])compares JAX array scalars; depending on JAX version this can raise becauseunittestexpects a Pythonboolfrom the comparison. Consider casting the values to Python floats (e.g.,float(newvx[1])) or usingself.assertTrue(float(newvx[1]) > float(newvx[0])).
# per-particle charge and mass should work through the vectorized path
self.assertEqual(newvx.shape, (2,))
self.assertGreater(newvx[1], newvx[0])
self.assertTrue(jnp.allclose(newvy, jnp.zeros(2)))
Comment on lines
+176
to
+180
| {"C": 10.0}, | ||
| ) | ||
| # electric field in x should accelerate the particle in x only | ||
| self.assertGreater(vx, 0.0) | ||
| self.assertTrue(jnp.allclose(jnp.array([vy, vz]), jnp.array([0.0, 0.0]))) |
This file contains hidden or 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
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.
Improved comments on Esirkepov and J_from_rhov methods.
Fixed bug that caused periodic ghost cells to swap values.