Skip to content

Anchored Dual-pass HLLD for Hypoelasticity (+ HLLC and interface-consistent HLL)#1414

Open
ChrisZYJ wants to merge 102 commits into
MFlowCode:masterfrom
ChrisZYJ:hypo_hlld
Open

Anchored Dual-pass HLLD for Hypoelasticity (+ HLLC and interface-consistent HLL)#1414
ChrisZYJ wants to merge 102 commits into
MFlowCode:masterfrom
ChrisZYJ:hypo_hlld

Conversation

@ChrisZYJ

@ChrisZYJ ChrisZYJ commented May 9, 2026

Copy link
Copy Markdown
Contributor

Description

Adds:

  1. Hypoelasticity: Anchored Dual-pass HLLD
  2. Hypoelasticity: HLLC
  3. Hypoelasticity: HLL option (interface-consistent)
  4. HLL option (alpha div U) so non-conservative treatment aligns with HLLC

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:

  1. Unlike HLL or HLLC, HLLD is a class of HLLD-type solvers, with formulas and states dependent on the eigenstructure of the governing equations, so the inner states' equations are completely different for MHD vs Hypoelasticity.
  2. HLLD hypoelasticity has a newly developed dual-pass anchored form, making it different from any convenional HLLD Riemann solver. The anchored forms are necessary for the non-conservative hypoelasticity terms, which MHD does not have.
  3. MHD and Hypoelasticity deal with completely different physical regimes with different governing equations, and any changes or new physical models added in the future will not apply to both modules at once.

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):

Branch Face quantity read RHS formula per $\alpha_k$ K*div(u) velocity source
adv_src_alpha_iface flux_src_n(dir)%vf(j_adv) = per-fluid $\Psi_{\alpha_k}$ $u_\text{cell} \cdot \Delta\Psi_\alpha / \Delta x$ nc_iface_vel_n(dir)%vf(1)
adv_src_vel_iface flux_src_n(dir)%vf(adv\%beg) = shared $\Psi_u$ $\alpha_k \cdot \Delta\Psi_u / \Delta x$ Same flux_src_n slot (already $\Psi_u$)
adv_src_none Skipped (HLLD handles internally)

The derivations, meanings, and usage of the Riemann source variables are not straightforward. I've added some hopefully very helpful notes in misc/dev_notes for 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

  • All default behaviors preserved exactly (newly added features as options)
    • The only exception is the removal of an incorrect ad-hoc fluids-limit guard that affects only Hypoelasticity HLL
  • All existing usage of Riemann and rhs source terms are preserved. No refactor is done to keep the scope of this PR limited (any refactoring would touch most of the existing HLLC functionalities)

Type of change

  • New feature

Testing

  • All tests passed locally on CPU and Nvidia GPU, and on Frontier.

  • Smooth Eigenmode Convergence

image
  • Weak Solution Comparison (Rodriguez & Johnsen (2019) §5.3(b))
image
  • Weak Scaling on Frontier
image

Checklist

  • I added or updated tests for new behavior
  • I updated documentation if user-facing behavior changed
  • GPU results match CPU results
  • Tested on NVIDIA GPU or AMD GPU

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)
  • Add label claude-full-review — Claude full review via label

@sbryngelson

Copy link
Copy Markdown
Member

@ChrisZYJ - pushed two correctness fixes to this branch (bb0090e), both flagged by the Claude review:

  1. m_riemann_solver_hllc.fpp, the NORM_DIR == 2 cyl + hypoelastic geometric-source block: the eqn_idx%cont%end + dir_idx(1) flux read used literal flux_rsx_vf(j, k, l, ...) while the LHS and every sibling assignment use ${SF('')}$. For the y-sweep SF('') expands to (k, j, l), so the literal indices read a transposed element - wrong geometric-source momentum flux on 2D/axisymmetric hypoelastic HLLC runs. Switched it to flux_rsx_vf(${SF('')}$, ...).

  2. m_checker.fpp: G1_eff/G2_eff use fluid_pp(1:2) for all hypoelastic Riemann solvers, but only HLLD (riemann_solver == 4) had the num_fluids == 2 guard. Added the matching guard for HLL/HLLC (riemann_solver == 1 .or. == 2) so num_fluids > 2 is rejected rather than silently dropping fluids 3+ from the bulk modulus.

Both compile.

@ChrisZYJ

ChrisZYJ commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

@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.

@sbryngelson sbryngelson marked this pull request as ready for review June 24, 2026 22:43
Copilot AI review requested due to automatic review settings June 24, 2026 22:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/simulation/m_riemann_solver_lf.fpp
Comment thread src/simulation/m_checker.fpp Outdated
Comment thread src/simulation/m_global_parameters.fpp
@codecov

codecov Bot commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 68.18966% with 369 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.63%. Comparing base (66254db) to head (6737464).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/simulation/m_riemann_solver_hypo_hlld.fpp 60.97% 129 Missing and 31 partials ⚠️
src/simulation/m_rhs.fpp 69.78% 56 Missing and 41 partials ⚠️
src/simulation/m_riemann_solver_hllc.fpp 69.18% 28 Missing and 25 partials ⚠️
src/simulation/m_riemann_solver_hll.fpp 48.48% 9 Missing and 8 partials ⚠️
src/simulation/m_riemann_solvers.fpp 10.52% 8 Missing and 9 partials ⚠️
src/simulation/m_hypoelastic.fpp 90.81% 3 Missing and 6 partials ⚠️
src/simulation/m_riemann_solver_lf.fpp 0.00% 5 Missing ⚠️
src/simulation/m_global_parameters.fpp 84.00% 0 Missing and 4 partials ⚠️
src/simulation/m_riemann_state.fpp 95.16% 0 Missing and 3 partials ⚠️
src/common/m_variables_conversion.fpp 66.66% 0 Missing and 2 partials ⚠️
... and 2 more
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sbryngelson
sbryngelson previously approved these changes Jun 27, 2026

@sbryngelson sbryngelson left a comment

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.

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-320 and :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-389 and :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 in m_riemann_state.fpp:1034-1183 almost verbatim, instead of using the fypp-templating approach this same file already uses for the analogous s_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.fpp threads the new adv_src_mode through 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 the adv_src_mode pattern 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.

@ChrisZYJ

ChrisZYJ commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

And thanks @sbryngelson for the suggestions! I addressed the following cleanup items:

  • Added checker + Python validator guard for mhd + hypoelasticity + HLLD.
  • Deduped repeated hypo elastic-energy and HLL/HLLC wave-speed formulas via Fypp inline macros.
  • Fixed an HLLC 2D-axisym elastic-energy bug: HLLC now doubles only shear_indices, matching m_variables_conversion.
  • Kept the intrusive s_hypo_hlld_riemann_solver helper extraction as follow-up scope. The solve kernel has been optimized for Frontier, and prior profiling showed it is register/occupancy-sensitive, so moving large blocks behind helpers could be done separately with performance profiling.

Also fixed the spelling CI failure: docs/documentation/case.md now uses patch(es) instead of patche(s). I think
this was exposed by a newer typos version on CI.

@ChrisZYJ

ChrisZYJ commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

@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!

@sbryngelson

Copy link
Copy Markdown
Member

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.

@ChrisZYJ

ChrisZYJ commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Okay. I'll refactor the solver into helpers according to your reviews, and minimize SLOC as you requested. Will post the changes here soon.

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

Labels

enhancement New feature or request

Development

Successfully merging this pull request may close these issues.

3 participants