Add a per-fluid equation-of-state selector - #1700
Conversation
Introduce fluid_pp(:)%eos, a stable five-value enumeration (stiffened_gas, ideal_gas_mixture, mie_gruneisen, jwl, table) in place of implicit backend selection. Only stiffened_gas (default) and ideal_gas_mixture (chemistry, Pyrometheus) are backed by an adapter; the remaining values are reserved and rejected explicitly. The default resolves from the compile-time chemistry flag, so every existing case is unchanged and all goldens stay bit-identical, and no runtime dispatch enters the hot per-cell path. Case files accept readable names while the integer representation stays internal. The Fortran and Python checkers reject unsupported values, ideal_gas_mixture without a chemistry build, and intra-cell EOS mixing (every fluid must share one family). The five enum constants are hand-written in m_constants.fpp; the constant generator skips compound registry keys so the per-fluid CONSTRAINTS entries drive only readable-name resolution and validation.
State plainly that a plain ideal gas without chemistry is stiffened_gas with pi_inf = 0, and that ideal_gas_mixture is the Pyrometheus mixture backend valid only in a chemistry build. This keeps users from reaching for ideal_gas_mixture on a non-chemistry build, where it is rejected.
18786a8 to
7ee3cd3
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1700 +/- ##
==========================================
+ Coverage 61.24% 61.25% +0.01%
==========================================
Files 83 83
Lines 20700 20708 +8
Branches 3072 3072
==========================================
+ Hits 12677 12685 +8
Misses 5969 5969
Partials 2054 2054 ☔ 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.
Thanks for this. The plumbing checks out — _r(f"{px}eos", INT, ...) registration means the MPI broadcast is auto-generated correctly as MPI_INTEGER for all three targets, the namelist binds fluid_pp as a derived type so fluid_pp(1)%eos resolves, and the compile-time chemistry parameter makes the pre-namelist-read default correct. The Fortran and Python checkers agree.
Four things to address before merge, mostly around the enum having several hand-synced copies. Details inline.
Separately, worth noting for context rather than as a change request: nothing in src/ currently reads %eos, and both checkers leave exactly one legal value per build since it is fully determined by chemistry. That is defensible as step-2 groundwork, but it is the part I would most want justified given the guidance against config knobs with one correct value.
than hand-sync the eos integer values, and cover the bubbles_euler slot Records the fluid_pp(:)%eos hand-written constants as a "still manual" case in common-pitfalls.md and notes it in the m_constants.fpp comment, since generate_constants_fpp silently skips compound registry keys. check_eos in case_validator.py now reads its two supported values from CONSTRAINTS["fluid_pp(1)%eos"]["names"], the same idiom already used for recon_type, instead of a third hand-synced copy of 1 and 2. check_eos (Python) and s_check_eos (Fortran) both now cover the extra fluid property slot that bubbles_euler uses, matching the existing check_stiffened_eos pattern; previously fluid_pp(num_fluids+1)%eos went unvalidated when bubbles_euler was set. Adds eos to the physical_parameters member list comment in definitions.py, the registry/type drift guard from MFlowCode#1553.
Code reviewFound 1 issue:
This came in with MFC/src/common/m_checker_common.fpp Lines 41 to 43 in 1a7222f MFC/src/common/m_constants.fpp Lines 23 to 25 in 1a7222f 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
7a48930 to
d401bba
Compare
|
I don't understand why we can't just do exactly what is done now, but use ideal gas as an option (leaving chemistry alone) for when \Pi_\infty = 0? |
That's a valid approach. I kept the change small on purpose: eos defaults to stiffened_gas (eos=1) when it isn't set, and that path already covers both stiffened gas and ideal gas. With pi_inf = 0 the stiffened-gas EOS reduces to an ideal gas, with no chemistry involved. eos=2 (ideal_gas_mixture) is the Pyrometheus multi-species mixture specifically, which is the only reason it is tied to chemistry. Happy to do it your way: require an explicit eos in the case file instead of defaulting it, and leave chemistry as its own independent switch. The ideal-gas case then stays exactly as you describe, stiffened_gas with pi_inf = 0, decoupled from chemistry. |
…p slot
Two problems with the selector as it stood.
The enum reserved mie_gruneisen, jwl, and table, but 'choices' listed all five
values, so the auto-generated hint advertised them as valid directly beneath
the error saying they were not implemented:
fluid_pp(1)%eos selects an equation of state that is not yet implemented;
only 'stiffened_gas' and 'ideal_gas_mixture' are available
Valid values: 1 (stiffened-gas), ..., 4 (JWL), 5 (tabulated)
Drop the three reserved values. They cost three hand-synced constants in
m_constants.fpp and bought nothing that adding them alongside their backend
would not; the hint is now consistent with what check_eos accepts.
Both checkers also stopped at num_fluids (+1 for bubbles_euler), but every
fluid_pp slot is default-assigned and MPI-broadcast up to num_fluids_max via
the member loop in _emit_fluid_pp, so a selector left on an unused slot reached
the solver unvalidated. With num_fluids = 2, fluid_pp(3)%eos was accepted
unchecked. Both loops now cover num_fluids_max; validation is input-time only,
so the wider bound costs nothing.
The membership check stays explicit in check_eos rather than deferring to the
'choices' constraint: choices is enforced by validate_constraints, a separate
layer from CaseValidator, so CaseValidator alone would have let an out-of-enum
integer through.
Nothing exercised fluid_pp(i)%eos: no case sets it, so 616/616 bit-identical showed only that the selector changed no behaviour, not that it works. Covers the enum, the readable-name to integer resolution done by Case, and each check_eos branch. test_fortran_and_python_enums_agree is the one that matters most: generate_constants_fpp skips compound registry keys, so the eos_* constants are hand-written in m_constants.fpp and nothing else forced them to match _EOS_NAMES. get_fortran_constants already parses m_constants.fpp, so the guard is a direct comparison rather than a comment asserting the invariant. test_unused_slot_is_validated pins the loop bound fixed in the previous commit. test_value_outside_enum_rejected pins the membership check that CaseValidator must not delegate to the 'choices' layer.
|
I pushed three follow-ups to this branch directly ( Reserved enum values removed. A user reads the second line, sets Both checkers now cover every slot. They stopped at Tests added. Writing those tests caught a mistake in my own first pass: I had dropped the membership check on the assumption that the Verification: On scope: I have left the "one legal value per build" point as a note in the description rather than a change request. It is a real cost — a case parameter that cannot be set to anything other than what it already is — and it is only worth paying if the first backend follows closely. I am fine merging on that basis, but the next PR in the series should be the thing that reads |
Resolves a conflict in src/common/m_global_parameters_common.fpp introduced by MFlowCode#1713 (remove the four-equation model). Both branches edit the same use m_constants line: this branch adds eos_stiffened_gas and eos_ideal_gas_mixture, master removes model_eqns_4eq. The resolution keeps both changes. Verified after the merge: build clean, full suite 627 passed, 0 failed.
Adds
fluid_pp(i)%eos, a stable enumeration for a fluid's equation of state, in place of the implicit backend selection. This is step 2 of the incremental EOS path tracked in #1638 ("add EOS selection and integrate the existing Pyrometheus path"). Note that step 1 — centralizing thermodynamic operations, attempted in #1663 — has not landed, so this selector arrives before the interface it will eventually dispatch through. That ordering is the reason nothing insrc/reads%eosyet; see Scope below.Only the two backends we already support are accepted:
stiffened_gas(default)ideal_gas_mixture(chemistry builds, via Pyrometheus)The enum holds exactly these two. Values for backends that do not exist yet are added alongside their backend rather than reserved up front, so the auto-generated "Valid values" hint never advertises a selector that
check_eoswill reject.ideal_gas_mixtureis the Pyrometheus mixture backend and is only valid in a chemistry build. On a non-chemistry build it is rejected, not silently downgraded. A plain ideal gas without chemistry is already covered bystiffened_gaswithpi_inf = 0, which reduces the stiffened-gas law to the ideal-gas law, so it does not need its own enum value.The default resolves from the compile-time
chemistryflag, so existing cases do not change and the golden suite stays bit-identical (616/616, no goldens regenerated). The selector is validated at input time only, so nothing new enters the per-cell hot path. Case files use the readable names while the integer stays internal. The Fortran and Python checkers both reject unsupported values,ideal_gas_mixtureon a non-chemistry build, and mixing EOS families within one run.Both checkers cover every
fluid_ppslot up tonum_fluids_max, not justnum_fluids: each slot is default-assigned and MPI-broadcast through the_emit_fluid_ppmember loop, so a selector left on an unused slot still reaches the solver.Scope
Nothing in
src/reads%eosyet, and the selector is fully determined by thechemistryflag, so there is exactly one legal value per build. That is deliberate — this PR establishes the interface and its validation, and the first backend to consume it lands next. Reviewers should weigh that tradeoff explicitly rather than treat it as an oversight.Tests
toolchain/mfc/params_tests/test_eos_selector.pycovers the enum, the readable-name to integer resolution, and eachcheck_eosbranch.test_fortran_and_python_enums_agreecompares the hand-writteneos_*constants inm_constants.fppagainst_EOS_NAMES—generate_constants_fppskips compound registry keys, so nothing else forces the two to match.Handover notes
Branch
External contribution (Fahad Nabid). Maintainer commits have been pushed directly to the contributor's branch —
maintainerCanModifyis true. If you push, use--force-with-lease=<branch>:<sha>with an explicit SHA; a bare--force-with-leasefails with "stale info" unless you have fetched the ref in the current clone.Commits added on top of the contributor's work:
Environment
Merge conflict with #1713 — resolved
After #1713 (remove the four-equation model) merged, this PR went
CONFLICTINGin exactly one file,src/common/m_global_parameters_common.fpp. Both branches edit the sameuse m_constantsline: this PR addseos_stiffened_gas, eos_ideal_gas_mixture, master removedmodel_eqns_4eq. Resolution keeps both. Verified after the merge: build clean, full suite 627 passed, 0 failed.If master moves again, that same
useline is the likely collision point.Plumbing that was verified, so it need not be re-derived
_r(f"{px}eos", INT, ...)indefinitions.pyregisters the parameter, so_emit_fluid_ppintoolchain/mfc/params/generators/fortran_gen.pyauto-generates the MPI broadcast asMPI_INTEGERfor all three targets. It reads the datatype from the registry, so no hand-editing ofm_mpi_proxyis needed.chemistryislogical, parameter :: chemistry = .${chemistry}$.(m_global_parameters_common.fpp), i.e. a compile-time fypp parameter. That is what makes the pre-namelist-read defaultmerge(eos_ideal_gas_mixture, eos_stiffened_gas, chemistry)correct and constant-folded.Case.__init__(toolchain/mfc/case.py) normalises readable names to integers beforeCaseValidatorruns, so the validator sees ints. The name-resolution path works end to end.fluid_pphas no GPU declare, so wideningphysical_parametersis free.Design decisions taken during review
The enum holds only implemented backends.
mie_gruneisen,jwlandtablewere removed. With them present,choicesadvertised all five values, so the auto-generated hint contradicted the error:Note
test_names_are_wellformed_and_cover_choicesassertsset(names.values()) == set(choices), sonamesandchoicesmust shrink together.Membership is re-checked in
check_eos. Thechoicesconstraint is enforced byvalidate_constraints, a different layer fromCaseValidator— relying on it lets an out-of-enum integer through. This was found by a test, not by reading.Both checkers loop to
num_fluids_max. Everyfluid_ppslot is default-assigned and MPI-broadcast up tonum_fluids_max, so a selector on an unused slot still reaches the solver. Validation is input-time only, so the wider bound costs nothing.Fortran/Python enum drift guard.
generate_constants_fppsilently skips compound registry keys (fluid_pp(1)%eoswould produce the invalid identifierfluid_pp(1)%eos_stiffened_gas), so theeos_*constants are hand-written inm_constants.fpp.test_eos_selector.py::test_fortran_and_python_enums_agreecompares them against_EOS_NAMESusingget_fortran_constants(), which parsesm_constants.fpp. Without that test nothing forces the two to agree.Open question for reviewers
Nothing in
src/reads%eosyet, and the selector is fully determined by the compile-timechemistryflag, so there is exactly one legal value per build. That is deliberate — this establishes the interface and its validation — but it is a config knob that cannot be turned until a backend consumes it. The next PR in the series should be the thing that reads%eos, not more interface.Series context
Tracking issue #1638 records the plan from #1659:
Note this is step 2 landing before step 1, which is why nothing reads the selector yet.
Related: #1714 (
eos_staterefactor) gives the interface a dispatch seam; #1708 tracks the remaining EOS duplication. A constraint established while investigating #1714:m_thermochem.f90is generated for every build, but non-chemistry builds get a dummyh2o2.yamlmechanism (toolchain/mfc/run/input.py), andnum_speciesfrom it sizesrhoYks(1:num_species)throughout. Soideal_gas_mixtureis a whole-build mode, not a per-fluid peer backend — the fyppchemistrysplit must stay, and any future runtime dispatch covers only the non-chemistry backends.Purpose in the series
Tracking issue #1638 (JWL / multi-EOS). Plan from #1659:
This is step 2 arriving before step 1, which is why nothing in
src/reads%eosyet. It establishes the vocabulary — a stable per-fluid enum, validated at input time — that later backends dispatch on. #1714 supplies the dispatch seam.Do not merge more interface after this. The next PR in the series should be the first thing that actually reads
%eos; otherwise the selector accumulates as a knob nobody can turn.Working conventions and hazards (shared across this series)
Collected from the work that produced #1705, #1709, #1712, #1713, #1714, #1716. Every one of these cost real time or produced a wrong result before being caught.
Testing
A regression test that cannot fail is worse than no test. Always verify the negative: revert the fix, rebuild, confirm the case fails, restore. Two ways this silently broke here:
git stash push -- <file>has nothing to stash once the fix is committed, so the "reverted" run tests the fixed binary and reports a pass identical to a real one. Usegit checkout master -- <file>, rebuild, test, thengit checkout HEAD -- <file>.model_eqns = 3cannot detect a sound-speed defect, because the six-equation branch ofs_compute_speed_of_soundtouches neitherHnorqv.The golden packer discards data.
toolchain/mfc/packer/pack.pytreated every.datunderD/as<x> [<y> <z>] <value>and kept only the last column of each row. Probe and integral output are multi-column time series, so most columns were never compared (#1711, fixed in #1712). Before asserting that a golden covers something, check it is actually ingolden.txt.Case labels are load-bearing. The golden UUID is
crc32(sha1(str(trace)))— the label chain determines the directory name. Renaming a label renames the golden. Avoid!in labels (history expansion in interactive bash).Local suite runs are flaky at high
-j. Non-reproducible failures appeared on several unrelated branches at-j 12–16(chemistry cases, probe cases) that passed individually and in clean reruns. Re-run before investigating.Removing parameters or features
Deregistering a parameter breaks things that are not the source tree. Removing
pref/rhoreffrom the registry broke the entire suite becauseBASE_CFGintoolchain/mfc/test/case.pyset them for every case. Also checkfp_stability.py,params_tests/mutation_tests.py, and lint fixtures that use real parameter names as examples.Grep the generated artifacts, not just the sources. A stale
TYPED_DECLSentry naming a deleted type survived removal and did not break the build only because the parameter had also left every target's namelist vars, so it was never emitted. Checkgenerated_decls.fpp,generated_constants.fpp,SIM_GPU_DECL_VARS, and the MPI broadcast generators.Dead-local tell: after removing a block, a local with exactly one remaining occurrence in its file is almost certainly its own declaration. Two occurrences often means declaration plus a
private()entry.Fortran is case-insensitive. A local
pRefshadowed the module globalprefin the hardcoded-IC files; the read site was spelledprefand looked like a reference to the global. It is not. Confirm scope before concluding a global is live.GPU
A CPU test run cannot catch a missing
private(). It is a silent device race. Audit by hand or by script when adding per-cell state.Do not match
GPU_PARALLEL_LOOPnaively —END_GPU_PARALLEL_LOOPcontains the same substring and will register as a loop start, producing false positives. Exclude it explicitly.Derived-type components cannot have runtime extents.
dimension(num_fluids)in a type fails to compile outside case-optimized builds, wherenum_fluidsis aparameter.Benchmarking
Run
./mfc.sh benchin the foreground on an idle machine. Running it in the background while a pre-commit precheck ran at-j 12produced a bogus +50% regression on a case that executes none of the changed code. Baseline noise here is ±3.5%; run the baseline twice before trusting any delta, and sanity-check that the regressing cases actually execute the modified code.GitHub mechanics
--force-with-leaseneeds an explicit SHA (--force-with-lease=<branch>:<sha>) when the ref has not been fetched in the current clone; the bare form fails with "stale info".gh run view --log-failedcan miss the real output entirely. On the Frontier jobs the failing step carried only a non-zero exit while the actual test output lived in a separatePrint Logsstep that succeeded. Fetch the full log.gh run rerun <id> --failedrefuses while the workflow is still running; retry later.file INSTALL cannot set modification time ... No such file or directory, exit 143). Check for a real error before assuming a code fault.