Anchored Dual-pass HLLD for Hypoelasticity (+ HLLC and interface-consistent HLL)#1414
Anchored Dual-pass HLLD for Hypoelasticity (+ HLLC and interface-consistent HLL)#1414ChrisZYJ wants to merge 103 commits into
Conversation
|
@ChrisZYJ - pushed two correctness fixes to this branch (bb0090e), both flagged by the Claude review:
Both compile. |
|
@sbryngelson Many thanks for the changes! Speaking of point 2, there's an important, subtle bug outside of hypoelasticity: "num_fluids > 2 .and. alt_soundspeed" is not prohibited in the checkers, which silently computes the wrong "K-term". We should add a checker to enforce that alt_soundspeed is only used for 2 components (alt_soundspeed is only for the 5-equation model). I can open a separate PR for that. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds hypoelasticity support for additional Riemann solvers, including an anchored dual-pass HLLD path, and wires up new non-conservative (NC) RHS dataflow/validation/documentation to support these solvers.
Changes:
- Adds hypoelasticity HLLD dual-pass dispatch plus supporting state buffers (hat-R flux set) and NC interface-velocity export.
- Introduces new runtime/toolchain parameters + validation for hypoelasticity solver options (ADC, interface-consistent RHS, energy guard, HLL u-interface mode).
- Updates docs, examples, and golden test artifacts for the new solver capabilities.
Reviewed changes
Copilot reviewed 70 out of 110 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| toolchain/mfc/test/case.py | Loosens tolerance for hypoelasticity test comparisons. |
| toolchain/mfc/params/descriptions.py | Documents newly introduced hypoelasticity/HLL configuration parameters. |
| toolchain/mfc/params/definitions.py | Registers new parameters and adds constraints/visibility metadata. |
| toolchain/mfc/case_validator.py | Extends toolchain-side validation for hypoelasticity + new flags. |
| tests/F31EAABF/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/DA44D68D/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/D41FFB94/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/CF11AA56/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/C7B686C0/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/B76C4F04/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/AE3ECF01/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/AB56B056/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/A26B7E00/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/9AEC024A/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/891C8626/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/879C490D/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/6AF90F3C/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/6736AFD0/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/6103FA4F/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/5D405BF9/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/4FBA4023/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/4912EFF1/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/481CA4B4/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/43CADBB8/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/34F3999B/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/345BC486/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/34336A1F/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/17E2C6D1/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/29C5D458/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/2097140E/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/19981E38/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/0DDE8A87/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/0648E422/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/03EB2617/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| tests/026AA23F/golden-metadata.txt | Adds new golden metadata file for regression test artifacts. |
| src/simulation/m_riemann_state.fpp | Adds new state buffers (flux hat-R + nc_iface_vel) and stress tensor permutation support for elasticity. |
| src/simulation/m_riemann_solvers.fpp | Adds hypoelastic HLLD solver dispatch, alloc/dealloc for new buffers, and dual-pass early-return path. |
| src/simulation/m_riemann_solver_lf.fpp | Updates hypoelastic elastic-energy contribution computation with optional guard and safe denominators. |
| src/simulation/m_global_parameters.fpp | Adds derived NC modes, adv_src_mode selection, and use_nc_iface_vel flag; wires new parameters. |
| src/simulation/m_data_output.fpp | Fixes pressure computation call site to pass energy by correct index. |
| src/simulation/m_checker.fpp | Adds runtime input validation for new HLL/Hypo NC branches and solver compatibility rules. |
| src/pre_process/m_global_parameters.fpp | Establishes default for hypo_energy_guard in pre_process. |
| src/post_process/m_global_parameters.fpp | Establishes default for hypo_energy_guard in post_process. |
| src/common/m_variables_conversion.fpp | Adds guarded elastic-energy (add/subtract) treatment to avoid division-by-near-zero G. |
| src/common/m_global_parameters_common.fpp | Exposes new parameters to device via GPU_DECLARE. |
| src/common/include/shared_parallel_macros.fpp | Improves folding of long GPU directives by also splitting oversized clauses at commas. |
| examples/3D_hypo_hlld/case.py | Adds a 3D hypoelastic HLLD example configuration. |
| examples/2D_hypo_hlld/case.py | Adds a 2D hypoelastic HLLD example configuration. |
| examples/2D_axisym_hypo_hlld/case.py | Adds a 2D axisymmetric hypoelastic HLLD example configuration. |
| docs/module_categories.json | Registers the new hypoelastic HLLD solver module in documentation categories. |
| docs/documentation/equations.md | Documents hypoelastic HLLD and NC-term developer notes references. |
| docs/documentation/case.md | Documents new user-facing parameters and updated solver availability for hypoelasticity. |
Comments suppressed due to low confidence (1)
tests/F31EAABF/golden-metadata.txt:1
- These golden metadata files embed ephemeral environment details (timestamp, local branch name, and a '(dirty)' marker). If any CI/test logic compares these files byte-for-byte, this will be brittle across regeneration and developer environments. Consider either excluding such metadata from golden comparisons (e.g., ignore sections/lines) or stripping volatile fields when generating the golden-metadata.txt files.
…2 (1-fluid is valid)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1414 +/- ##
==========================================
+ Coverage 60.43% 60.63% +0.20%
==========================================
Files 83 84 +1
Lines 19868 20702 +834
Branches 2956 3064 +108
==========================================
+ Hits 12007 12553 +546
- Misses 5860 6054 +194
- Partials 2001 2095 +94 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sbryngelson
left a comment
sbryngelson
left a comment
There was a problem hiding this comment.
High-level code-quality pass
This PR is large (~12k additions), but most of that is auto-generated tests/*/golden-metadata.txt fixtures (~40 new test dirs) — the hand-written src/ diff is closer to ~2,850 lines. Overall the new code is well-documented for its complexity (good WHY-comments, sensible naming, the dev-notes markdown files are legitimate cross-file architecture docs, not compensating for opaque code). The points below are meant to help trim the remaining LOC/complexity without touching the numerics, plus one real correctness gap.
1. Biggest lever: s_hypo_hlld_riemann_solver doesn't use the codebase's own decomposition tool
src/simulation/m_riemann_solver_hypo_hlld.fpp:21-965 is a single ~945-line subroutine with an ~800-line GPU-parallel loop body (147 privatized scalars) — over contributing.md's own soft guideline (≤500 lines/subroutine, ≤1000/file; this file is 1090). MFC already has a working, GPU-offload-safe pattern for factoring per-cell math out of parallel loops: $:GPU_ROUTINE(parallelism='[seq]') helper subroutines (used for s_compute_speed_of_sound, s_compute_pressure, etc.), and this file already calls s_compute_speed_of_sound that way (lines 301-304) — but never applies the same pattern to its own star-state algebra (~lines 556-630) or output-permutation logic (~lines 822-880). Extracting those into [seq]-parallel helpers would roughly halve the loop body with no offload-capability cost.
(Context: this isn't a new anti-pattern — s_hllc_riemann_solver is already ~1970 lines pre-existing — so this PR extends existing tech debt rather than introducing a new category of it. But it's the single biggest opportunity to cut LOC here.)
2. ~150 lines of duplicated small physics formulas — safe, mechanical extraction
Confirmed identical across 3-4 files:
- Hypoelastic energy-correction floor/guard formula:
m_riemann_solver_hll.fpp:347-360,m_riemann_solver_hllc.fpp:308-320and:1416-1428,m_riemann_solver_hypo_hlld.fpp:284-294 - Wave-speed estimate (Rodriguez et al. 2019 formula):
m_riemann_solver_hll.fpp:396-403,m_riemann_solver_hllc.fpp:378-389and:1489-1500,m_riemann_solver_hypo_hlld.fpp:306-309 s_finalize_riemann_solver_hatR(m_riemann_solver_hypo_hlld.fpp:1030-1088) duplicates the existing generic finalizer inm_riemann_state.fpp:1034-1183almost verbatim, instead of using the fypp-templating approach this same file already uses for the analogouss_finalize_nc_iface_vel${SUFFIX}$(lines 974-1020).
Pulling these into shared helper functions/a template would save ~100-150 lines with no design risk.
3. Real gap, not just style: mhd .and. hypoelasticity .and. riemann_solver==4 is never rejected
m_checker.fpp only prohibits HLLD (riemann_solver==4) when neither mhd nor hypoelasticity is set — never when both are set simultaneously. In that combination, hypo_nc_dual_pass becomes true and m_riemann_solvers.fpp unconditionally routes to s_hypo_hlld_riemann_solver, which has no reference to mhd/Bx/By/Bz anywhere — magnetic-field physics is silently dropped with no error, and the existing MHD HLLD path is simply never reached. Suggest adding @:PROHIBIT(riemann_solver == 4 .and. mhd .and. hypoelasticity, "HLLD does not support combined MHD and hypoelasticity") alongside the existing checks in s_check_inputs_hypo_branch.
Lower priority / follow-up material
m_rhs.fppthreads the newadv_src_modethrough 6 separate subroutines (s_initialize_rhs_module,s_compute_rhs,s_compute_directional_rhs,s_compute_advection_source_term,s_add_directional_advection_source_terms,s_finalize_rhs_module) rather than resolving the mode once and passing it down — some shotgun surgery, though consistent with that file's existing (non-templated) style.m_riemann_solvers.fpp's own comment (lines 42-49) states hypoelasticity now enters the Riemann layer in "three distinct code shapes" (HLL/HLLC inline branches vs. HLLD's separate fused-dual-pass module). A future 4th hypoelastic solver variant would face the same scattered integration choice — worth unifying behind one seam before that happens, not a blocker for this PR.- The three mutually-exclusive hypo-mode booleans derived in
m_global_parameters.fpp(hypo_nc_finite_diff/hypo_nc_interface/hypo_nc_dual_pass) could be one enum, matching theadv_src_modepattern already used elsewhere in the same file — inconsistent style for an equivalent problem, not a bug.
None of the above is a blocker — the code is correct and well-tested per the PR description. These are opportunities to reduce the diff's LOC/maintenance footprint using patterns already established elsewhere in this codebase, plus one validation gap worth closing before merge.
Generated with the help of Claude Code.
|
And thanks @sbryngelson for the suggestions! I addressed the following cleanup items:
Also fixed the spelling CI failure: |
|
@sbryngelson If this looks good after the CI rerun, would it be possible to merge this PR soon? I’m happy to continue improving the code in follow-up PRs after this lands. The branch has already gone through several upstream re-merges and CI/review cleanup rounds, so merging the core implementation now would make future cleanup work smaller and easier to review. Thank you! |
|
I'm no longer merging messy/bloated/WIP code on the promise that it will be fixed later. I've been burned on this far too many times (perhaps not by you), and it only ratchets up, not down. Please clean your code of smells, refactor into nice helpers, and minimize SLOC while maintaining speed if you want the PR merged. Thanks. |
|
Okay. I'll refactor the solver into helpers according to your reviews, and minimize SLOC as you requested. Will post the changes here soon. |
…(identical codegen)
…e (identical codegen)
…hint on f_hlld_wave_zone
|
I've done the refactor. Here is what changed:
This removes 95 lines across the touched files. What is left in the main subroutine is dominated by the per-cell star-state algebra, which appears only once, so there is no duplication left to remove. Extracting it behind an interface would take ~50 scalar arguments or regrouping the privatized locals into derived types, and either would change register allocation on the GPU kernel. Since the goal was fewer lines at unchanged speed, that block stays inline, and the helper extractions went where the interfaces are narrow. I've taken care to ensure that most changes do not affect the generated code, and that they have minimal impact on performance. Also merged in the current master. |
|
One note on CI. The last two runs each failed a single Frontier job at the Fetch Dependencies step, before anything gets built or tested (gpu-omp 1/2, then gpu-acc 2/2). Both failures show the same error on the same runner, frontier-5: Maybe a corrupted uv cache entry on that runner's node? Could you help me re-run the one failed job? All the tests should pass. |
Description
Adds:
Key Design Choices
Separate HLLD Riemann Solvers
At a glance it might be tempting to combine HLLD MHD with dual-pass hypoelasticity HLLD, but keeping them separate makes the code cleaner and much easier to maintain because:
Riemann Source Terms
For the non-conservative terms, unlike the usual governing equations that only need div U i.e. du/dx, dv/dy, dw/dz (alpha div U, K div U, etc.), Hypoelasticity has cross terms like du/dy, so we must also pass those Riemann-consistent traces from Riemann solver to the rhs. (The old Hypoelasticity code with the HLL Riemann solver uses finite difference for non-conservative rhs, which provides enough stability given that HLL smears the interface immediately, so there wasn't a need to pass the du/dy traces before this PR. But that does not work for HLLC/HLLD for Hypoelasticity.)
Also grouped/named the condition branches (with lots of comments within the code):
adv_src_alpha_ifaceflux_src_n(dir)%vf(j_adv)= per-fluidnc_iface_vel_n(dir)%vf(1)adv_src_vel_ifaceflux_src_n(dir)%vf(adv\%beg)= sharedflux_src_nslot (alreadyadv_src_noneThe derivations, meanings, and usage of the Riemann source variables are not straightforward. I've added some hopefully very helpful notes in
misc/dev_notesfor future developers (or AI agents; directing them to my notes should help them make fewer mistakes with the source terms) in terms of the understanding and derivations for the HLL/HLLC non-conservative fluxes, and their variable mapping for Riemann solvers and RHS.Backwards Compatibiilty
Type of change
Testing
All tests passed locally on CPU and Nvidia GPU, and on Frontier.
Smooth Eigenmode Convergence
Checklist
AI code reviews
Reviews are not triggered automatically. To request a review, comment on the PR:
@coderabbitai review— incremental review (new changes only)@coderabbitai full review— full review from scratch/review— Qodo review/improve— Qodo code suggestions@claude full review— Claude full review (also triggers on PR open/reopen/ready)claude-full-review— Claude full review via label