Remove the four-equation model (model_eqns = 4) - #1713
Open
sbryngelson wants to merge 1 commit into
Open
Conversation
The four-equation Tait model had no test, example, or benchmark exercising it:
the suite only ever sets model_eqns to 2 or 3, and no case anywhere set the
rhoref/pref pair it required. Its code paths were therefore unverifiable in
either direction, and its presence forced a model_eqns branch through the
pressure inversion, the energy assembly, the primitive recovery, the HLLC
solver, the immersed-boundary and acoustic-source updates, and both
initialisation paths.
Removed everywhere, including the logic that only existed to route around it:
- the eqn_idx/sys_size branch and its nested bubbles_euler block
- the Tait inversion in s_compute_pressure and the zero-energy assignment
in the prim->cons path, collapsing both to the five-equation forms
- the /= 4eq guards in the primitive recovery, IBM, acoustic source, and
the pre-process patch assignment
- the 209-line HLLC four-equation branch
- the sound-speed branch condition, now just five-equation with
bubbles_euler
- the 'rho' vs 'alpha_rho' variable naming split in post-process output
model_eqns_4eq is generated from the names dict in definitions.py, so dropping
'4eq' removes the constant without touching m_constants.fpp.
Variables that die with it:
- rhoref: every read was a Tait relation. Removed entirely, including its
registry entry, GPU declaration, MPI broadcast, and BASE_CFG default.
- pref: same. Note the hardcoded ICs are unaffected -- 2d/3dHardcodedIC
declare their own local pRef and set it to 1e5, which shadowed the global
because Fortran is case-insensitive. Their references are renamed to the
declared spelling so the locality is visible.
- lit_gamma, and the pi_inf/gamma pairs feeding it, in four
m_icpp_patches subroutines and one m_assign_variables subroutine: all
were assigned solely for the Tait density relation.
- the pref overwrite in s_add_bubbles (#1710), whose last reachable
consumer was the Tait branch.
Validator rules, the model_eqns choices/labels/names, the bubbles_euler
requires_value, the model_eqns=4 dependency block, and the docs entries are
updated to match. Two toolchain unit tests used pref/rhoref as example
registry-bound scalars; they now use poly_sigma, which is still registered in
all three targets.
Verified: builds clean (GNU 15.2, MPI); full suite 627 passed, 0 failed, with
no golden regenerated -- removing the model changes no result anywhere, which
is the evidence it was dead.
|
Claude Code Review Head SHA: 575d19c Files changed:
Findings:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request removes the previously supported four-equation Tait model (model_eqns = 4) across MFC (toolchain validation/parameter registry, solver code paths in pre_process/simulation/post_process, and user-facing documentation), simplifying multiple core branches that were effectively untestable.
Changes:
- Removed
model_eqns = 4as a valid option from the toolchain parameter definitions and case validation (including droppingpref/rhorefparameters that existed to support the Tait inversion path). - Deleted solver branches that were specific to the four-equation model (EOS pressure inversion/energy handling, HLLC branch, IBM/acoustic/ICPP special guards, and GPU update plumbing).
- Updated documentation and toolchain tests to reflect the supported
model_eqnsset and remaining parameters.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| toolchain/mfc/test/case.py | Removes pref/rhoref from test case defaults now that 4-equation support is gone. |
| toolchain/mfc/test_lint_source.py | Updates lint-source tests to use poly_sigma as the example registry scalar instead of removed pref/rhoref. |
| toolchain/mfc/params/generators/fortran_gen.py | Drops pref/rhoref from generator scalar lists so generated Fortran bindings/bcasts no longer include them. |
| toolchain/mfc/params/descriptions.py | Removes user-facing descriptions for pref/rhoref. |
| toolchain/mfc/params/definitions.py | Removes model_eqns=4 choice/name/label and unregisters pref/rhoref. |
| toolchain/mfc/params_tests/test_fortran_gen.py | Adjusts generator tests to assert poly_sigma broadcast presence across targets. |
| toolchain/mfc/params_tests/mutation_tests.py | Removes invalid-value mutation cases for pref/rhoref parameters that no longer exist. |
| toolchain/mfc/fp_stability.py | Removes pref/rhoref from floating-point stability case parameter sets. |
| toolchain/mfc/case_validator.py | Updates validation rules to only allow model_eqns in {1,2,3} and removes 4-equation-specific bubble checks. |
| src/simulation/m_riemann_solver_hllc.fpp | Deletes the 4-equation HLLC flux branch and removes the constant import. |
| src/simulation/m_ibm.fpp | Removes the model_eqns /= model_eqns_4eq gating so IBM always uses the mixture conversion path (since 4eq is removed). |
| src/simulation/m_global_parameters.fpp | Removes 4-equation QBMM indexing block and stops GPU-updating removed pref/rhoref. |
| src/simulation/m_data_output.fpp | Removes 4-equation-specific lit_gamma assignment path in output/probe logic. |
| src/simulation/m_bubbles_EL.fpp | Removes assignment that overwrote global pref during Lagrangian bubble initialization. |
| src/simulation/m_acoustic_src.fpp | Removes 4-equation guard that previously suppressed energy source updates. |
| src/pre_process/m_icpp_patches.fpp | Removes 4-equation-dependent EOS parameter placeholders and the Tait-based density reassignment block. |
| src/pre_process/m_global_parameters.fpp | Removes 4-equation QBMM indexing/initialization block (including the old stop path). |
| src/pre_process/m_assign_variables.fpp | Removes 4-equation density-from-pressure logic and unifies alpha_rho assignment. |
| src/post_process/m_start_up.fpp | Removes 4-equation constant import and simplifies output naming to always use alpha_rho* for 5eq/6eq. |
| src/post_process/m_global_parameters.fpp | Removes 4-equation QBMM indexing/initialization block (including the old stop path). |
| src/common/m_variables_conversion.fpp | Removes 4-equation EOS inversion and related special-casing in pressure/energy/primitive recovery/sound speed logic. |
| src/common/m_global_parameters_common.fpp | Removes 4-equation eqn-index/system-size layout branch and drops defaults for pref/rhoref. |
| src/common/include/3dHardcodedIC.fpp | Renames pref references to pRef to make local shadowing explicit (no behavior change intended). |
| src/common/include/2dHardcodedIC.fpp | Same pref → pRef visibility rename for the 2D hardcoded IC. |
| docs/documentation/equations.md | Removes model_eqns = 4 from equations documentation. |
| docs/documentation/contributing.md | Updates contributing guidance to remove references to the removed model’s EOS branch. |
| docs/documentation/case.md | Removes pref/rhoref entries and updates model_eqns description to only list 1–3. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1713 +/- ##
==========================================
+ Coverage 60.77% 61.24% +0.46%
==========================================
Files 83 83
Lines 20872 20700 -172
Branches 3101 3072 -29
==========================================
- Hits 12685 12677 -8
+ Misses 6121 5969 -152
+ Partials 2066 2054 -12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Removes the four-equation Tait model (
model_eqns = 4) from the codebase, together with the logic and variables that existed only to serve it.Fixes #1710 — the
prefoverwrite ins_add_bubblesis deleted here, since the global it wrote no longer exists.Why
Nothing exercised it. The test suite only ever sets
model_eqnsto 2 or 3; no example or benchmark sets 4; and no case anywhere set therhoref/prefpair the model required. Its code paths were unverifiable in either direction — a change breaking them would have gone unnoticed, and so would a change fixing them.Meanwhile its presence forced a
model_eqnsbranch through the pressure inversion, the energy assembly, the primitive recovery, the HLLC solver, the immersed-boundary and acoustic-source updates, and both initialisation paths.This surfaced while tracing
preffor #1710.What was removed
Structure
eqn_idx/sys_sizebranch inm_global_parameters_commonand its nestedbubbles_eulerblock, plus the same block in all threem_global_parameters.fpps_compute_pressureand the zero-energy assignment in theprim -> conspath; both collapse to the five-equation formsm_riemann_solver_hllc/= 4eqguards in primitive recovery, IBM, acoustic source, and pre-process patch assignmentbubbles_eulerrhovsalpha_rhovariable-naming split in post-process outputmodel_eqns_4eqis generated from thenamesdict indefinitions.py, so dropping"4eq"removes the constant without touchingm_constants.fpp.Variables that die with it
rhorefBASE_CFGdefault all removedpreflit_gamma(+ thepi_inf/gammapairs feeding it)m_icpp_patchessubroutines and one inm_assign_variablespref = gas_p(...)ins_add_bubblesA note on the hardcoded ICs. They appear to read the global
pref, but do not.2dHardcodedIC.fppand3dHardcodedIC.fppeach declare their own localpRefin the variables macro and set it to1e5; because Fortran is case-insensitive, that local shadows the module global. Their references are renamed to the declared spelling so the shadowing is visible rather than implied. No behaviour change.Toolchain and docs
Validator rules, the
model_eqnschoices/labels/names, thebubbles_eulerrequires_value, themodel_eqns = 4dependency block, and thecase.md/equations.md/contributing.mdentries are updated. A case settingmodel_eqns = 4now fails validation with the standard out-of-range message.Two toolchain unit tests used
pref/rhorefas example registry-bound scalars; they now usepoly_sigma, which is still registered in all three targets.Verification
No golden moved. That is the substantive result: removing the model changes no result anywhere, which is precisely the evidence that it was dead.