GK: loss cone updater refactored for arbitrary geometry - #997
GK: loss cone updater refactored for arbitrary geometry#997Maxwell-Rosen wants to merge 73 commits into
Conversation
…e maximum magnetic field is determined. Bmag max is stored as a gkyl_array. Right now, we only do this for bmag, but we need to store phi as a 1d maximum array. I haven't decided on the final design for how 2x OAP simulations should be accomodated. Perhpas we need some general gkyl_dg_array_reduce methods that take a 2D array and turn it into a 1D array instead of a 0D number. This is a kind of reduction method, but it's not a total reduction. I tested the regression test included for a 2x2v boltzmann mirror and the output of the magnetic field looks correct. The current implementation evaluates bmag at cell corners, but we ideally should do the corners in Z, but the quadrature nodes in psi.
- Introduced a new header file `gkyl_array_dg_find_peaks.h` that defines a structure and functions for finding peaks (local maxima, minima, and boundary values) in DG fields. - Implemented an internal structure in `gkyl_array_dg_find_peaks_priv.h` to manage peak finding operations, including storage for peak values and coordinates. - Removed unused initialization and writing of `bmag_max` arrays in `gyrokinetic.c` to streamline the geometry setup process. - Deleted the `gkyl_gk_geometry_bmag_max_init` and `gkyl_gk_geometry_bmag_max_release` functions from `gk_geometry.c` as they are no longer needed, simplifying the geometry management.
…es another DG array at the peaks of the initilized array. Add appropriate unit tests which pass to ctest_array_dg_find_peaks. Update gk_species_fdot_multiplier to use the project_on_peaks function with phi. Now, everything passed to loss_cone_mask_gyrokinetic is a gkyl_array. The loss_cone_mask is updated accordingly
…ount of compution we need for evaluating phi at its peak in the app. Unit tests pass. Regression tests look fine as well. They're all valgrind clean. I think the right way to do the paralellism is to do the peak finding on a global bmag, just like how it is done for the position_map, then when we evaluate phi, all processes evaluate it at this peak, however only one will return a true value. This process will broadcast the array to the rest of the processes
…on at the peak locations
… method, which is just like find_peaks, but it computes the global maximum or minimum.
…nd regression tests are brought over from another branch. Unit tests for the array mask, loss cone mask, and the regression tests for the kinetic electron POA mirror are valgrind free
…grind clean. Regression test is added and produces reasonable results.
…formance - Introduced a helper function `mkarr` to streamline array allocation for GPU and CPU. - Removed the `gkyl_loss_cone_mask_gyrokinetic_Dbmag_quad_wall` function and integrated its logic into the main processing flow. - Updated the GPU kernel `gkyl_loss_cone_mask_gyrokinetic_Dbmag_quad_cu_ker` to compute `Dbmag_quad` directly from `bmag_peak` instead of `bmag_max`. - Enhanced tandem mirror support by adding handling for `bmag_peak` and `phi_m` in the GPU kernels. - Simplified the logic for determining trapped particles in the `gkyl_loss_cone_mask_gyrokinetic_ker` and `gkyl_loss_cone_mask_gyrokinetic_quad_ker` functions. - Improved readability and maintainability by restructuring conditional checks and variable assignments.
…plier. Add possibility of kinetic electrons and tandem mirrors. The damping regression test is failing, both here and on main. They are for different issues. Main fails because the loss_cone updater has an issue. Here, it fails because it's using scale_by_cell with a multi-component array. I'm not sure the right way to fix this
…ove the aspects about the cellwise evaluation and quadrature points because that breaks the array_scale_by_cell method which is used
… refactor this in the future, but it is just proof of concept for now to make sure it works correctly.
… arrays need to be passed to objects like the loss_cone_mask, where it expects these to be GPU arrays. It's just easier to have this module fit the archatecture of the rest of the code, rather than doing something different and copying between device and host. It wouldn't interface well. Claude generated most of the cuda code, with strong guidence from Maxwell
…h compute sanitizer with the array_find_peaks which was causing crashes in the loss cone mask. These issues are fixed. There was some funny business regarding the basis being on the host vs device. Refactor the allocations in the GPU kernels to not be inside the kernels. Instead, it's allocated at init time. The GPU code pulses, which is odd, but it runs. The 2x2v POA regression test runs and is compute sanitizer clean. The other POA tests do not error either on GPU and are compute sanitizer clean.
…itting code to main and broke a unit test with the geometry enum changes
…hi_smooth_global array for improved performance and consistency across computations.
…ion of doing the kinetic electron and tandem mirror. The code is built again to make sure nothing is affected.
…=2 relevant code for the peak finders
…egression test (and in my production simulations)
…-run allgather Root cause The reset path itself was fine â�� the bug was the NCCL allgather of bmag being re-issued mid-run. The loss-cone init in gk_species_fdot_multiplier.c did: gkyl_comm_array_allgather(app->comm, &app->local, &app->global, app->gk_geom->geo_corn.bmag, fdmul->bmag_global); bmag is static geometry, but gk_species_fdot_multiplier_reset does a full release â�� init, so this collective fired again on every reset. On Perlmutter's new stack (cudatoolkit/13.0 + nccl/2.29.2-cu13 + AWS Libfabric/GDR), issuing that collective from a reset corrupts the run â�� matching every observation: - 1 GPU fine (no real collective) / 4 GPU all NaN / 8 GPU edge ranks NaN / both phases right after a reset / didn't exist before the CUDA13/NCCL upgrade. - Without reset it's fine because the only mid-run collective that breaks is the reset's re-gather â�� the per-step phi allgather and the app-creation bmag allgather both work. On the host/device question: I checked, and geometry is already on device at app-creation fdot init too â�� app_new runs app_new_geom (hostâ��device swap at gyrokinetic.c:442-446) before app_new_solver creates species. So host-vs-device wasn't the difference; the new NCCL just doesn't tolerate that mid-run gather. The fix Gather the static global bmag once (in the reliable app-construction context) and reuse it across resets, so no allgather is ever issued from a reset: - struct gk_species gains fdot_bmag_global (cached, gathered lazily on first loss-cone init). - Loss-cone (re)inits now gkyl_array_acquire that cache instead of re-gathering. - Released at species teardown; refcounting keeps reset release/init balanced. After this, no fdot reset issues any collective (FIXED_FACTOR never did; loss-cone now reuses the cache) â�� which is exactly why it should clear the "both phases, right after reset" failure. 3 files, +21/â��5; CPU build is clean (0 errors). I can't run the NCCL path here, so please rebuild with configure.perlmutter.gpu.sh and re-test the 4-GPU case (the cleanest repro: it went from all-NaN to expected to be fine). One thing to watch: if any of your real input decks first enable the loss-cone multiplier via a mid-run reset (rather than having it on at app creation like failure.c), the one-time gather would then happen mid-run. If you want it bullet-proof for that case too, say the word and I'll make the cache gather eagerly during species construction regardless of the initial multiplier type
…void mid-run allgather" This reverts commit 9c6d202.
- Implemented a new gyrokinetic simulation code for a mirror configuration that supports two species: ions and electrons. - Introduced a pseudo orbit-averaged (POA) integrator with distinct phases for orbit averaging and full dynamics. - Defined plasma parameters, geometry, and source terms for the simulation context. - Added functions for magnetic field calculations, density evaluations, and velocity mappings for both species. - Included diagnostics for integrated quantities and phase-space outputs. - Established a framework for running multiple phases with adjustable parameters and time-stepping controls.
…n on multiple nodes / GPUs on the latest NCCL and CUDA. It has nothing to do with NCCL or CUDA!!!! it's becuase time_dilation_scale_const can be initilized to nan when using gkyl_malloc, which happens when the poa_phases are initilized, however the regression tests do not pass this parameter into the poa_phases struct, so the regression tests were fine, but my production simulations were not. using calloc initilizes everything to zero, which leads to an elegant handling of this parameter. Here is claude's final response. It was never NCCL â�� it was uninitialized memory, and MPI just exposed it. In rt_gk_failure.c, poa_phases is gkyl_malloc'd (plain malloc, not zeroed) and the phase loop never sets time_dilation_scale_const. So run_phase feeds an uninitialized value into the loss-cone multiplier: - Serial / 1-GPU â�� poa_phases lands on fresh OS pages (zeroed) â�� field is 0.0 â�� the old if (x <= 0.0) guard catches it â�� 1.0 â�� runs fine (your = 1 runs). - MPI â�� MPI_Init dirties the heap first, so the field reuses garbage holding a NaN bit pattern â�� -nan. -nan <= 0.0 is false, so it slips through, multiplies into the multiplier, and dt becomes -0/NaN. Each rank gets different garbage â�� looked like a "some regions / comms" bug for the whole investigation. Fixes applied (both compile clean) 1. Guard in gk_species_fdot_multiplier.c (what you asked for): falls back to 1.0 for any non-finite or non-positive time_dilation_scale_const, with a one-time warning. - Crucial subtlety: the build uses -ffast-math, so isfinite()/x!=x get optimized away (clang even warned -Wnan-infinity-disabled). The guard therefore detects non-finite via the bit pattern ((u >> 52) & 0x7ff == 0x7ff), which fast-math can't elide. 2. Root cause in rt_gk_failure.c: gkyl_malloc â�� gkyl_calloc for poa_phases, so unset fields (time_dilation_scale_const, f_threshold, the _ion/_elc variants) are zero instead of garbage. You need to rebuild â�� your three runs above were the old binary. With the rebuild, the 4-GPU run should print the warning (or nothing, with the calloc fix) and proceed with scale = 1.0 instead of NaN. One thing worth deciding: with time_dilation_scale_const correctly defaulting to 1.0, the OAP gets no time dilation. If the production sim intends a specific dilation factor during OAP, set it explicitly in the phase loop (poa_phases[2*i].time_dilation_scale_const = ...) â�� otherwise the calloc/guard just makes "unset" mean "1.0" deterministically. Want me to wire an explicit OAP value through, or leave it at 1.0?
|
Looking at the "Inclusion of Sheath Wall Potentials", why do you have to introduce augmented escape barriers (EBs), instead of defining the left and right EBs as where ? If this is valid it seems to me like it simplifies the physical motivation/logic. |
|
I'm not sure I understand. It seems like the equations you have in the comment are just a rephrasing of the ones in the PR description. If anything, I think my formulation of this is clearer because I explicitly use the term I asked Codex about this, and it says the sheath potential modifications aren't currently implemented. I think it's misunderstanding, though, since |
…tiplier
- Introduced new enum `gkyl_loss_cone_boundary_type` to classify boundary behaviors (open, sheath, closed).
- Updated `gk_species_fdot_multiplier` to handle lower and upper wall potentials for sheath boundaries.
- Modified `gkyl_loss_cone_mask_gyrokinetic` to incorporate wall potentials in escape barrier calculations.
- Enhanced tests to cover various boundary condition scenarios, ensuring correct behavior for both open and closed boundaries.
- Adjusted CUDA kernel to accommodate new parameters for wall potentials.
- Sheath barriers now use the distinct material-wall potential, including grounded,
biased, asymmetric, and evolving walls.
- Corrected path scanning to include both cell traces and domain endpoints.
- Added roundoff-safe cutoff equality handling.
- Wired species BCs and global wall potentials through gyrokinetic/apps/
|
I meant to have the latest, most powerful models inspect this PR. It found that the wall potential wasn't included in this implementation (although it's zero for the sheath BCs), so I had it implement that to be more comprehensive and physical |
…ement for species
…ctions and update boundary condition applications
…pdate initialization in species
Refactor phi wall boundary conditions into species table
manauref
left a comment
There was a problem hiding this comment.
Detailed comments in the review. Overall this seems good, thanks for doing it Max. My only overall comments are:
- It's clear that LLMs wrote this and several of its choices seem odd. I think the code would've been improved (and reviewer's time saved) with a more careful read/thought prior to PR submission.
- Please explain how correctness is determined or understood. What is each unit test checking and how was the correct value obtained?
- I think the equations/logic for escape barriers can be made clearer, as in my earlier comment, but we don't have to act on that now.
| @@ -1,16 +1,24 @@ | |||
| #include <assert.h> | |||
| #include <gkyl_gyrokinetic_priv.h> | |||
There was a problem hiding this comment.
why are you editing the gk_species_damping module? I thought you weren't using this nor needed to modify it?
There was a problem hiding this comment.
I'm editing the damping module because there is code in there that utilizes the loss cone updater. Nobody is using this feature, so I deprecated it.
| if (dbmag_vals[i] > 0 && dbmag_vals[i-1] < 0){ | ||
| // Near-zero derivative: B is locally flat here, record as a minimum. | ||
| // Use continue so this is mutually exclusive with the sign-change checks below. | ||
| if (fabs(dbmag_vals[i]) < 1e-10) { |
There was a problem hiding this comment.
should this comparison use the derivative normalized to some sensible value instead? perhaps something like
dbmag_vals[i] / ( B at this location / dz ) < 1e-10
typically comparing a physical value (dB/dz in this case) to a single number tends to be less robust.
Same for all the dbmag comparisons below.
There was a problem hiding this comment.
It's just checking if Bmag is increasing or decreasing here. The difference is taken elsewhere
| @@ -0,0 +1,1030 @@ | |||
| #include <math.h> | |||
There was a problem hiding this comment.
how is this reg test different than rt_gk_mirror_boltz_elc_poa_1x2v_p1.c? if it has kinetic elc, then name it so.
and are all these regression tests running in under 90 s?
There was a problem hiding this comment.
Deleted. I think this was doing ion-electron collisions, which is why it was a _2species
| #include <acutest.h> | ||
|
|
||
| #include <gkyl_util.h> | ||
| #define _USE_MATH_DEFINES |
There was a problem hiding this comment.
don't see why this is needed
There was a problem hiding this comment.
Not needed. deleted
|
|
||
| #ifndef M_PI | ||
| #define M_PI 3.14159265358979323846 | ||
| #endif |
There was a problem hiding this comment.
don't think this is needed
if necessary see gkyl_const.h
There was a problem hiding this comment.
I agree. It might be doing this because there is a longstanding linting issue (or its just on my laptop) where it doesn't recognize M_PI.
| int conf_node = node / num_vel_nodes; | ||
| int vel_node = node % num_vel_nodes; |
There was a problem hiding this comment.
note that this hardcodes a particular ordering to the nodes, which is not desirable. It's best to use ranges where that order is abstracted out and can more easily be changed
|
|
||
| GKYL_CU_DH | ||
| static inline int | ||
| conf_node_z_endpoint_index(int cdim, int conf_node, int zdim) |
There was a problem hiding this comment.
add some prefix, like gk_lcm_, to all these static private functions so it's clear where they are and that they don't come from other headers
There was a problem hiding this comment.
and add comments describing them, and their inputs when they are not absolutely obvious (cdim is an example of obvious)
| */ | ||
| void | ||
| gkyl_loss_cone_mask_gyrokinetic_advance_cu(gkyl_loss_cone_mask_gyrokinetic *up, | ||
| #ifdef __cplusplus |
There was a problem hiding this comment.
don't know why this is needed. We've never had this before
| } | ||
|
|
||
| void | ||
| extern "C" void |
There was a problem hiding this comment.
i'd prefer that the use of extern C follows the pattern used before and in other .cu files
- Updated data structures to use configuration-space P1 corners instead of nodes. - Renamed functions and variables for clarity, reflecting the new corner-based approach. - Modified escape barrier calculations to accommodate corner evaluations. - Improved CUDA kernel to handle corner indices and streamline phase-space cell processing. - Enhanced assertions and error handling for trajectory types and boundary conditions. - Removed unnecessary dependencies and included relevant headers for clarity.
Hamiltonian-based Classification of Loss and Trapped Orbits in Mirror Geometry
This PR was generated with the assistance of GitHub Copilot for the unit tests and the updater functions. Most of it is LLM-generated, but I've read it all and understand it.
Consider a one-dimensional coordinate$z\in[z_L,z_R]$ aligned with a magnetic field line, with magnetic-field magnitude $B(z)$ and electrostatic potential $\phi(z)$ . For a species of mass $m_s$ and charge $q_s$ , the guiding-center Hamiltonian in $(z,v_\parallel,\mu)$ coordinates is
where$v_\parallel$ is the parallel velocity and $\mu$ is the magnetic moment. In the absence of collisions, $H$ and $\mu$ are invariants of motion, and particle trajectories lie along contours of constant $H$ in $(z, v_\parallel)$ .$v_\parallel=0$ , hence
Turning points satisfy
where$U$ is the Yushmanov potential.
Prior work simply identified the trapped/passing boundary based on the
loss cone criterion
In that implementation,$B_m,\phi_m$ are determined at the peaks of $B$ . However, the effective potential experienced by particles is $U$ , which combines both $B$ and $\phi$ . This can be essential for identifying the trapped region of phase space induced by angled neutral-beam injection of ions at $45^\circ$ , where an off-axis density peak occurs. Furthermore, this implementation only uses a single loss criterion per region, when multiple can exist, for instance, in tandem mirrors. A Hamiltonian-based region-identification algorithm is more accurate and flexible because it directly addresses the Yushmanov potential.
Two-Wall Escape Criterion
One issue with a Hamiltonian-based identifier is that$H$ is non-unique. For instance, consider a symmetric simple mirror without an electric potential, just two peaks in B. The magnetic field has fourfold degeneracy at the half-maximum. The points outside the peaks in $B$ should be labeled passing, while those between peaks of $B$ should be trapped.
A trajectory at fixed$(H,\mu)$ can escape through the left wall at $z_L$ if it can traverse the interval $[z_L,z]$ without violating $H\ge U$ . For a phase-space point $(z,v_\parallel,\mu)$ , define the escape barrier to the left wall as
and the escape barrier to the right wall as
The minimum escape barrier required for escape through at least one wall is
Defining the signed loss function
Inclusion of Sheath Wall Potentials
The field$\phi(z)$ does not directly represent the sheath drop at the wall, but instead it is assumed that $\phi=0$ in Gkeyll's sheath boundary conditions. Let $\phi_L^{\mathrm{bc}}$ and $\phi_R^{\mathrm{bc}}$ denote prescribed wall potentials at $z_L$ and $z_R$ , respectively, and let $\phi(z_L)$ and $\phi(z_R)$ be the values implied by the loaded field data near the walls. These are left general, but taken as zero in the simulations. The effective wall potentials are taken as
The corresponding augmented path barriers are
with an augmented escape barrier
Replacing$EB$ by $\widetilde{EB}$
yields a classifier that includes sheath reflection consistently.
Integration into the Gkeyll Workflow
Within Gkeyll, one first interpolates the fields to nodal values, computes$EB$ , calculates the loss barrier, and then, if any corners of the cell are not orbiting, the cell is not evolved.
Community Standards
layer/zeroshould have a unit test, e.g.,core/zero.Testing: (x (yes), blank (no))
make checkand unit tests all pass.Additional notes
Here is a comparison in a beam simulation with the current loss mask and the old one.
This is an R=32 mirror case with Boltzmann electrons, showing the difference in the fdot multiplier. Red means these cells are now masked with the new updater, while blue means cells that were masked, but are not with this version. In other words, this is new_mask - old_mask. We see the population of small mu particles which are trapped due to the off-axis beam injection, which is what we expect.
Here is another view, this time in pyvista

Here is a simulation run with the new loss cone mask. I'm re-running a beam R=32 case, which was used for the paper Gyrokinetic equilibria of high temperature superconducting magnetic mirrors

This did decrease the time-step during the OAP to 3.4e-7 versus 2.6e-6 seconds. Perhaps a more coarse mesh in mu would help.