[SPH][NIMHD] Add Non-Ideal MHD terms#1765
Open
y-lapeyre wants to merge 16 commits into
Open
Conversation
Contributor
|
Thanks @y-lapeyre for opening this PR! You can do multiple things directly here: Once the workflow completes a message will appear displaying informations related to the run. Also the PR gets automatically reviewed by gemini, you can: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for Non-Ideal Magnetohydrodynamics (NIMHD) by adding Ohmic resistivity, Hall effect, and Ambipolar Diffusion terms. Key changes include the addition of NIMHD configuration parameters, mathematical kernels for current and heating terms, and a new CFL timestep constraint for Ambipolar Diffusion. Review feedback highlighted a critical architectural issue regarding the pairwise calculation of electric current, which should be a field quantity requiring a two-pass approach. Additionally, several potential division-by-zero bugs were identified in the mathematical kernels and timestep calculations, along with a likely incorrect gradient index in the induction term and misleading argument names in the Python bindings.
Collaborator
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for Non-Ideal Magnetohydrodynamics (NIMHD) in the SPH solver, adding configuration parameters, mathematical formulations for current density and resistivity, and the corresponding derivative updates. However, several critical issues must be addressed: a bug where the current density J_a is passed by value instead of reference in MagCurrentJ_sum (discarding accumulated values), a race condition and event synchronization bug in the CFL timestep calculation due to variable shadowing, and potential out-of-bounds reads in compute_J from iterating over ghost particles that lack neighbor lists. Additionally, minor improvements are recommended, such as removing unused parameters in B_NI_terms, cleaning up a temporary comment, and renaming a Python binding parameter for consistency.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Collaborator
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces Non-Ideal Magnetohydrodynamics (NIMHD) support to the SPH solver, including configurations for Ohmic resistivity, Hall effect, and Ambipolar diffusion, as well as the calculation of electric current density and non-ideal heating/induction terms. The review feedback highlights critical issues with SYCL event shadowing that can lead to concurrent kernel execution and data races on the timestep buffer. Other recommendations include evaluating the NIMHD flag at compile-time, skipping self-interactions in SPH neighbor loops, removing commented-out code, and resolving a developer query about the correct kernel gradient.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
tdavidcl
changed the title
Collaborator
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements Non-Ideal Magnetohydrodynamics (NIMHD) support in the SPH solver, introducing configuration parameters for Ohmic, Hall, and Ambipolar diffusion, a new ComputeJ module to calculate magnetic current density, and integrating non-ideal terms into derivative updates and timestep constraints. Feedback highlights critical mathematical and implementation bugs in mhd.hpp, such as passing J_a by value instead of reference, incorrect normalization of Bhat, a sign error in the Ambipolar Diffusion term, and an incorrect negative sign in the heating rate. Additionally, several unused variables and large blocks of commented-out dead code in ComputeJ.cpp, UpdateDerivs.cpp, and Solver.cpp should be removed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+47
to
+48
| Tvec D = etaO * J + etaH * sycl::cross(J, Bhat) | ||
| + etaAD * sycl::cross(sycl::cross(J, Bhat), Bhat); |
Contributor
There was a problem hiding this comment.
The Ambipolar Diffusion term in WursterD has the wrong sign. Since (J x Bhat) x Bhat is mathematically equal to -J_perp, adding this term results in a negative contribution to D . J (which would mean cooling instead of heating). It must be subtracted to ensure a positive contribution to the non-ideal dissipation and heating.
Suggested change
| Tvec D = etaO * J + etaH * sycl::cross(J, Bhat) | |
| + etaAD * sycl::cross(sycl::cross(J, Bhat), Bhat); | |
| Tvec D = etaO * J + etaH * sycl::cross(J, Bhat) | |
| - etaAD * sycl::cross(sycl::cross(J, Bhat), Bhat); |
| template<class Tvec, class Tscal, MHDType MHD_mode = NonIdeal> | ||
| inline Tscal u_NI_heating(Tvec D, Tvec J, Tscal rho) { | ||
|
|
||
| return -sycl::dot(D, J) * sham::inv_sat_zero(rho); |
Contributor
There was a problem hiding this comment.
The non-ideal SPH heating rate u_NI_heating has an incorrect negative sign. Non-ideal dissipation (Ohmic, Hall, Ambipolar) always converts magnetic energy into thermal energy, meaning it must act as a heating source (positive contribution to du_dt). With the corrected signs in WursterD, D . J is positive, so the heating rate should be positive.
Suggested change
| return -sycl::dot(D, J) * sham::inv_sat_zero(rho); | |
| return sycl::dot(D, J) * sham::inv_sat_zero(rho); |
| Tvec xyz_a = r[id_a]; // could be recovered from lambda | ||
|
|
||
| Tscal h_a = hpart[id_a]; | ||
| Tscal dint = h_a * h_a * Rkern * Rkern; |
Contributor
There was a problem hiding this comment.
The variable dint is defined but never used in this function. It should be removed to keep the code clean.
| Tscal omega_a = omega[id_a]; | ||
| Tscal sub_fact_a = rho_a_sq * omega_a; | ||
|
|
||
| Tscal part_omega_sum = 0; |
Contributor
There was a problem hiding this comment.
The variable part_omega_sum is defined but never used in this function. It should be removed.
| pcache.complete_event_state(resulting_events); | ||
| }); | ||
| } | ||
| */ |
Contributor
There was a problem hiding this comment.
There is a very large block of commented-out code (from line 1091 to 1449). Leaving large blocks of dead/commented-out code reduces maintainability and readability. Please remove this block entirely.
Comment on lines
+1992
to
+1996
| // communicate_merge_ghosts_fields(); | ||
| // modules::UpdateDerivs<Tvec, Kern> derivs(context, solver_config, storage); | ||
| // Tscal const mu_0 = solver_config.get_constant_mu_0(); | ||
| // derivs.compute_J(mu_0); | ||
| // reset_merge_ghosts_fields(); |
Contributor
There was a problem hiding this comment.
These commented-out lines are dead code and should be removed to keep the codebase clean.
Contributor
Workflow reportworkflow report corresponding to commit aaf9874 Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Clang-tidy diff reportDoxygen diff with
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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
1 participant
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.
Add and enable the use of no-ideal MHD in the MHD solver, using the structure I previously set out for it.
Tests:
Could be merged as is and fixed with a 2nd PR that adds the wave damping CI test.