Extract m_eos and add a single EOS family registry - #1870
Open
sbryngelson wants to merge 21 commits into
Open
Conversation
Task 3 of the m_eos extraction: consumers of the EOS routines now `use m_eos` (or `use m_eos, only: ...`) directly instead of relying on m_variables_conversion's temporary re-export. m_variables_conversion's public list drops the moved EOS names; the m_global_parameters_common arrays it re-exports (gammas, isentrope_n, pi_infs, isentrope_B, cvs, qvs, qvps) are unaffected and stay out of scope. Only use/public statements changed - no executable code was touched.
…metrically Give s_phase_pressure_on_isentrope and s_phase_temperature the function_name= and cray_inline=True attributes carried by every other cross-module GPU_ROUTINE device helper, matching the bare-directive convention already followed by m_eos's private helpers (s_reference_curve, s_eos_coefficients, s_ode_slope, s_rk4, s_phase_c2). Also close an allocation/deallocation asymmetry Task 4 introduced: s_initialize_eos_module (m_eos) allocates eight fluid-property arrays, but s_finalize_variables_conversion_module (m_variables_conversion) was still the one freeing them. Add a public s_finalize_eos_module to m_eos that deallocates exactly those eight arrays, shrink m_variables_conversion's DEALLOCATE call to its own Gs_vc, and call s_finalize_eos_module() immediately after s_finalize_variables_conversion_module() in all three start-ups (pre_process, simulation, post_process).
Task 6 of the m_eos extraction: contributing.md's "How to Add an Equation of State" section and equations.md's key-source-files line still pointed at m_variables_conversion.fpp for the EOS operators themselves. Update both to m_eos.fpp, add a s_reference_curve table row, and note that s_initialize_eos_module resolves constant-coefficient families once at start-up.
Placing it last made "the first six are mechanical... the last two are caloric" wrongly include s_reference_curve, which needs only rho and a fluid index. Moving it to row 1 restores the six/two split and matches "adding a second EOS means supplying these" - it's the one operator a new family must actually add.
- Correct m_global_parameters_common.fpp ownership comment: the material property arrays are initialized by m_eos, not m_variables_conversion. - Remove unused f_bulk_modulus import from m_hypoelastic.fpp. - Move f_c2_from_coefficients to the private/family-layer row in the m_eos extraction design doc's tier table, matching the code and prose. - Fix m_eos.fpp header overclaim: state direct dependencies (and build order) instead of falsely claiming no transitive MFC dependencies.
definitions.py stored the eos_* enum three times (names, labels, choices) and hand-wrote three near-identical mg_*/jwl_*/vinet_* parameter blocks. toolchain/mfc/params/eos_families.py is now the single registry those sites derive from; it also carries each state-dependent family's eos_coeffs field mapping (parameter, Fortran literal, or computed) for later refactor steps to generate Fortran from. test_eos_families.py asserts against values captured from the unmodified tree, not the registry, to prove this is behavior-preserving. Co-authored-by: Claude <noreply@anthropic.com>
…en_a bug R2 of the EOS family registry collapse. The state-dependent families dict, the optional-parameter dict, and the initial-state-inside-EOS dispatch in case_validator.py now derive from EOS_FAMILIES instead of hard-coding the mie_gruneisen/jwl/vinet trio in four places. Rewiring the dispatch through the registry's coefficients_fn exposed a real bug: the validator's Vinet path composed coefficients_from_curve(vinet_reference(...), vinet_gruneisen) directly, silently dropping vinet_gruneisen_a, unlike m_eos.fpp which applies a variable Gruneisen coefficient to every state-dependent family. Added eos.vinet_coefficients (mirroring eos_coefficients) so all three families now resolve to one direct coefficients call.
…is live Review of 712d1a4 found the added vinet_coefficients test called eos.py directly and never exercised case_validator.py's dispatch -- the actual bug site. The only existing validator-level Vinet test sets alpha_rho(1) equal to vinet_rho0, so mu = 0 and gruneisen_a*mu is zero regardless of gruneisen_a, making it blind to a dropped gruneisen_a in the lambda. Added a state at mu = 0.2 with vinet_gruneisen_a = -20.0, chosen so the variable-Gruneisen and constant-Gruneisen paths disagree on accept/reject, not just on the computed coefficients. Verified (then reverted) that temporarily dropping vinet_gruneisen_a from the lambda in _check_initial_states_inside_eos makes this test fail before restoring it.
…writing them fluid_pp(1)%eos gets a "fortran_prefix": "eos" constraint entry; generate_constants_fpp now emits <fortran_prefix>_<name> for a compound key that carries one, instead of skipping it (deduped across the 10 fluid_pp(f)%eos entries that share the prefix). Deletes the hand-written eos_* block from m_constants.fpp, which already #:includes the generated file. test_eos_selector.py's test_fortran_and_python_enums_agree parsed m_constants.fpp textually and would find nothing now that the constants live in the #:include'd generated file; asserting the generator agrees with the registry it generated from would also be circular. Replaced with two non-circular tests: the emitter produces exactly the five expected eos_<suffix> = <value> lines, and every eos_* identifier referenced under src/ is either generated or a known non-family symbol (eos_coeffs, eos_coefficients, eos_rk4_steps).
…m EOS_FAMILIES
New generated_eos.fpp holds four Fypp macros derived from the registry: the two family
predicates (expression-valued, spliced with ${MACRO('i')}$) and the two layers of
s_initialize_eos_module's coefficient assignment (statement-valued, @:MACRO(i)). A Fypp
#:include is textual and adds no use, so m_eos stays a leaf module.
Which layer a field belongs to is derived, not hard-coded: a field more than one family
writes needs a case arm to pick its source, a field one family writes does not. That
yields case = {rho0, t0, gruneisen0, gruneisen_a} and unconditional = {c0, s, s2, s3,
mu_max, a, b, r1, r2, k0, k0p}, reproducing the hand-written split exactly. All three
eos_coeffs source kinds are handled: Param copies, JWL's FortranLiteral gruneisen_a, and
mu_max's Computed call, whose arguments are qualified as fluid_pp(i)%<param> and which
raises rather than guessing if it cannot resolve one.
f_has_isentropic_reference generates only the family half of its condition. The
gruneisen_a == 0._wp conjunct is a runtime value, not a family property, and stays
hand-written in m_eos.fpp; generating it away would change which fluids take the
closed-form isentrope.
The case default arm is not a family property either, so EOS_COEFF_DEFAULT /
EOS_COEFF_DEFAULTS were added to eos_families.py to say explicitly that gruneisen_a
defaults to a live 0._wp where the other dispatched fields default to dflt_real.
Verified by textual equivalence rather than by a test run: m_eos.fpp preprocessed
through Fypp before and after, with cmake's exact flags, leaves 547 statements on each
side and one difference -- case default moves from between case (eos_jwl) and
case (eos_vinet) to last, which Fortran treats identically. Both predicates expand
byte-identically. Details in the R4 report.
Made with Claude Code.
Seven tests in test_eos_families.py close the gap between the registry and the parts of the codebase that stay hand-written by design: every registry parameter has a physical_parameters field, every required parameter is read by s_initialize_eos_module (accounting for generated_eos.fpp), every coefficients_fn resolves in eos.py, every family has a s_reference_curve case, Doxygen math symbols match a hand-verified expected map, and every case-dispatched eos_coeffs field is assigned by every state-dependent family (with EOS_COEFF_DEFAULTS keys checked against that same set). The last check is duplicated as a generation-time assertion in generate_eos_fpp so a future family that skips a case-dispatched field fails the build, not just CI. Each test was verified to fail by breaking its target and observing the failure before reverting.
Task R6 of the m_eos extraction. "How to Add an Equation of State" described the pre-registry world implicitly (only the per-family operators, not how a family's parameters get registered/validated). Add a short paragraph naming eos_families.py as the entry point, what it generates (Fortran eos_* constants, definitions.py registration, validator required/optional sets, both family predicates, both s_initialize_eos_module layers), and what stays hand-written and test-checked (s_reference_curve's case body, eos.py's mirror function, physical_parameters' fields), plus the gruneisen_a runtime caveat in f_has_isentropic_reference. Made with Claude Code.
case.py held the seventeenth family list, `eos_state_dependent = {3, 4, 5}`,
feeding the any_state_dependent_eos case-optimization flag; a new family would
have left it false and aborted every case-optimized run with a "Rebuild"
message pointing at the wrong fix. It now derives from EOS_FAMILIES.
case_validator's initial-state check took only the function name from
coefficients_fn while the argument list stayed hand-written per family, ending
in an unlabelled `else` meaning Vinet. A new family fell into that `else` and
died with a TypeError. EosFamily gains an ordered `coefficients_args`; the call
is built generically, defaulting exactly the suffixes listed in `optional`. The
generated calls are argument-identical to the three they replace.
Widen the init-agreement test to optional parameters: the bug this registry
exists to prevent (Vinet ignoring vinet_gruneisen_a) was an optional one, so a
required-only sweep still missed it.
contributing.md now states the four guarded edits a new family needs and names
the two that remain, marking them as failing loudly at build.
Made with Claude Code.
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Refactors equation-of-state (EOS) plumbing by extracting EOS math into a new Fortran leaf module (m_eos) and centralizing EOS-family metadata in a single Python registry (EOS_FAMILIES) that drives code generation and validation.
Changes:
- Extracts EOS computation/initialization into
src/common/m_eos.fppand updates Fortran call sites to initialize/finalize it. - Introduces
toolchain/mfc/params/eos_families.pyregistry and extends codegen (fortran_gen.py) to emit EOS-related Fypp macros/constants from it. - Updates validator logic and adds tests to ensure registry ↔ generated ↔ hand-written EOS code remain consistent (including the Vinet variable-Gruneisen fix).
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| toolchain/mfc/test_eos_mie_gruneisen.py | Adds regression coverage ensuring Vinet coefficients incorporate variable Grüneisen term. |
| toolchain/mfc/test_case_validator.py | Adds validator regression test ensuring vinet_gruneisen_a participates in initial-state checks. |
| toolchain/mfc/params_tests/test_fortran_gen.py | Updates generator expectations and adds coverage for generated EOS Fypp macros. |
| toolchain/mfc/params_tests/test_eos_selector.py | Switches EOS constant checks to validate generator output and scans src/ for stray eos_* identifiers. |
| toolchain/mfc/params_tests/test_eos_families.py | New test suite pinning observable behavior and cross-checking registry vs hand-written EOS touchpoints. |
| toolchain/mfc/params/generators/fortran_gen.py | Adds fortran_prefix support for compound enum params and generates generated_eos.fpp macros/initializers. |
| toolchain/mfc/params/eos_families.py | Adds the EOS_FAMILIES registry, coefficient-source modeling, and default rules. |
| toolchain/mfc/params/definitions.py | Derives EOS enum names/labels/choices and parameter registration from EOS_FAMILIES; adds fortran_prefix constraint key. |
| toolchain/mfc/eos.py | Adds vinet_coefficients mirroring Fortran’s variable-Grüneisen logic. |
| toolchain/mfc/case_validator.py | Refactors state-dependent EOS dispatch based on EOS_FAMILIES (including initial-state coefficient calls). |
| toolchain/mfc/case.py | Derives EOS_STATE_DEPENDENT_VALUES from registry for case-optimization flagging. |
| src/simulation/m_start_up.fpp | Initializes/finalizes m_eos for simulation target. |
| src/simulation/m_riemann_solver_hypo_hlld.fpp | Imports m_eos for EOS helpers now moved out of m_variables_conversion. |
| src/simulation/m_riemann_solver_hllc.fpp | Imports m_eos for EOS helpers now moved out of m_variables_conversion. |
| src/simulation/m_riemann_solver_hll.fpp | Imports m_eos for EOS helpers now moved out of m_variables_conversion. |
| src/simulation/m_rhs.fpp | Imports m_eos for EOS helpers now moved out of m_variables_conversion. |
| src/simulation/m_reactive_burn.fpp | Switches EOS calls (f_pressure, s_phase_temperature) to m_eos. |
| src/simulation/m_qbmm.fpp | Imports m_eos for EOS helpers now moved out of m_variables_conversion. |
| src/simulation/m_pressure_relaxation.fpp | Switches multiple EOS helpers to m_eos (pressure/coefficients/isentrope helpers). |
| src/simulation/m_igr.fpp | Imports m_eos for EOS helpers now moved out of m_variables_conversion. |
| src/simulation/m_ibm.fpp | Imports m_eos for EOS helpers now moved out of m_variables_conversion. |
| src/simulation/m_hypoelastic.fpp | Switches bulk-modulus helper import to m_eos. |
| src/simulation/m_bubbles_EL.fpp | Imports m_eos for EOS helpers now moved out of m_variables_conversion. |
| src/simulation/m_bubbles_EE.fpp | Imports m_eos for EOS helpers now moved out of m_variables_conversion. |
| src/simulation/m_acoustic_src.fpp | Imports m_eos for EOS helpers now moved out of m_variables_conversion. |
| src/pre_process/m_start_up.fpp | Initializes/finalizes m_eos for pre_process target. |
| src/post_process/m_start_up.fpp | Initializes/finalizes m_eos for post_process target. |
| src/post_process/m_derived_variables.fpp | Imports m_eos for EOS helpers now moved out of m_variables_conversion. |
| src/post_process/m_data_output.fpp | Imports m_eos for EOS helpers now moved out of m_variables_conversion. |
| src/common/m_variables_conversion.fpp | Removes extracted EOS routines and delegates EOS-dependent operations to m_eos. |
| src/common/m_phase_change.fpp | Imports m_eos for EOS helpers now moved out of m_variables_conversion. |
| src/common/m_global_parameters_common.fpp | Updates docs comment to reflect EOS arrays now initialized in m_eos. |
| src/common/m_eos.fpp | New leaf module containing EOS initialization, reference-curve math, and EOS thermo helpers. |
| src/common/m_constants.fpp | Removes hand-written EOS selector constants and documents codegen source. |
| docs/module_categories.json | Registers m_eos under Physics Models for documentation tooling. |
| docs/documentation/equations.md | Updates key-source-file pointers to reflect EOS extraction into m_eos. |
| docs/documentation/contributing.md | Updates “How to Add an EOS” guidance to use EOS family registry as entry point. |
| cmake/ParamsCodegen.cmake | Adds generated_eos.fpp to enumerated generated include outputs (15 → 18). |
Suppressed comments (4)
toolchain/mfc/params/generators/fortran_gen.py:1
- The
emitted_prefixesde-dupe will silently skip subsequent enum constraints that reuse the samefortran_prefix, even if theirnamesmappings differ (only the first encountered gets emitted). Consider adding an explicit consistency check: if a prefix was already emitted, verify the incomingnamesdict is identical (or raise a ValueError) to prevent subtle omissions when additionalfortran_prefixusers are introduced.
toolchain/mfc/params_tests/test_eos_selector.py:1 - This regex only matches identifiers starting with lowercase
eos_. Since Fortran is case-insensitive,EOS_VINET(or mixed-case) would be missed by the scan. Consider compiling withre.IGNORECASEand normalizing matches to lowercase before comparison so the test reliably catches all selector references regardless of casing.
toolchain/mfc/params_tests/test_eos_selector.py:1 - Filtering with
if \"eos_\" in lineis broader than needed and can become brittle if headers/comments ever mentioneos_(or if additional unrelatedeos_*symbols are generated). Consider narrowing the filter to the exact declaration pattern being asserted (e.g., lines starting withinteger, parameter :: eos_) so the test only inspects emitted constants.
toolchain/mfc/params/generators/fortran_gen.py:1 - If
familiesis empty, this returns an empty string, which would produce an invalid macro body ingenerated_eos.fpp(blank expression). Consider returning a valid Fortran logical literal such as.false.whenfamiliesis empty, so the generator remains robust if a category ever legitimately has zero families.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| args = [] | ||
| for suffix in family.coefficients_args: | ||
| value = self.get(f"fluid_pp({i})%{family.prefix}_{suffix}") | ||
| args.append((value or 0.0) if suffix in optional else value) |
|
Claude Code Review Head SHA: ab34a1a Files changed:
Findings:
|
Lines of Code
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1870 +/- ##
==========================================
- Coverage 61.26% 61.22% -0.04%
==========================================
Files 84 85 +1
Lines 22330 22321 -9
Branches 3265 3266 +1
==========================================
- Hits 13680 13666 -14
- Misses 6207 6211 +4
- Partials 2443 2444 +1 ☔ 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.
Made with Claude Code.
Summary
s_reference_curve,f_pressure,s_compute_energy,f_bulk_modulus,s_initialize_eos_module, and friends) out ofsrc/common/m_variables_conversion.fppinto a new leaf modulesrc/common/m_eos.fpp(526 lines), separating per-family EOS math from the mixture-closure rules (Wood's law, six-equation mean, bubbly branch) that remain inm_variables_conversion.fpp(now 1493 lines).toolchain/mfc/params/eos_families.py: a singleEOS_FAMILIESregistry (oneEosFamilyentry per family — stiffened-gas, ideal-gas, Mie-Gruneisen, JWL, Vinet). From that entry, these are now generated instead of hand-maintained at each of ~16 separate sites: the Fortraneos_*constants, thephysical_parametersparameter registration indefinitions.py, the case validator's per-family required/optional parameter sets and initial-state coefficient calls, bothf_is_state_dependent/f_has_isentropic_referencefamily predicates, both layers ofs_initialize_eos_module, and the case-optimizationany_state_dependent_eosflag.s_reference_curve's per-familycasebody (the actual mathematics), the Python mirror function intoolchain/mfc/eos.py, and the family's fields onphysical_parametersinsrc/common/m_derived_types.fpp.docs/documentation/contributing.md's "How to Add an Equation of State" section to describe the registry as the entry point.docs/superpowers/for reference/history.This is a refactor: it does not change any CFD result. Golden test files under
tests/are untouched (git diff --stat upstream/master -- tests/is empty).Test plan
./mfc.sh build -j 16— exit 0build/venv/bin/python -m pytest toolchain/mfc/ -q— all passing./mfc.sh test -j 16— 713 passed, 0 failed, 39 skipped, 752 total (bit-for-bit identical to pre-refactor goldens)git diff --stat upstream/master -- tests/— empty./mfc.sh precheck -j 12— all 7 checks passpython3 toolchain/mfc/lint_docs.py— cleanContribution Policy
We do not accept pull requests generated primarily by AI without genuine understanding or real-world usage context.
All contributions are expected to demonstrate:
If these expectations are not met, we would prefer to implement the changes ourselves rather than spend time reviewing low-effort submissions.
Acknowledgement
PR template credit: junegunn