From 891c45442fc63ef8c9fce1ed14eb0e675ebc4442 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 9 Aug 2026 15:23:28 -0400 Subject: [PATCH 1/8] Move input-only simulation checks from Fortran to case_validator The Python validator and src/simulation/m_checker.fpp had drifted into double entry: reactive_burn (#1670) added nine constraints to both in the same PR, and ib_state_wrt, chemistry+bubbles_euler, and dt <= 0 were each enforced twice. Delete the Fortran copies of the checks that depend only on input parameters, and complete their Python counterparts: - reactive_burn num_fluids / gamma / pi_inf / qv pairing (new in Python) - chemistry operator-split sub-stepping, 4 checks (new in Python) - many_ib_patch_parallelism requires ib (new in Python) - bf_spatial_support 2D-only (new in Python) - dt <= 0, ib_state_wrt, chemistry + bubbles_euler/qbmm (already in Python; drop the Fortran duplicates) s_check_inputs_time_stepping had no remaining body and is removed. Fortran defaults are materialized where an unset value is meaningful: chem_params%reaction_substeps{,_max} and fluid_pp(i)%qv all default to 0, so Python treats unset as 0 to match. gamma and pi_inf default to the dflt_real sentinel, which f_approx_equal reports as equal, so Python skips the comparison when either is unset. nv_uvm_igr_temps_on_gpu stays in Fortran. Its default is 3 and the check is "== 3 .and. igr_iter_solver == 2", so it fires precisely when the user has not set the parameter; in Python an unset value reads as None and the check would never fire. It is also inside #ifdef __NVCOMPILER_GPU_UNIFIED_MEM, which the validator cannot see. The remaining Fortran checks depend on runtime or compiler state that is unavailable at validation time: MPI decomposition (s_check_total_cells, s_check_inputs_fft), per-rank m/n/p (WENO/MUSCL stencil widths), and compiler conditionals (s_check_amd, rdma_mpi). Add toolchain/mfc/test_case_validator.py. case_validator.py had no pytest coverage of its constraint checks, and validating the example cases only exercises configurations meant to pass, so a check that stopped firing would go unnoticed -- which now matters more, since these no longer have a Fortran backstop. Reverting case_validator.py fails 15 of the 24 tests. src/simulation/m_checker.fpp: 225 -> 162 lines. --- src/simulation/m_checker.fpp | 63 --------- toolchain/mfc/case_validator.py | 54 ++++++++ toolchain/mfc/test_case_validator.py | 197 +++++++++++++++++++++++++++ 3 files changed, 251 insertions(+), 63 deletions(-) create mode 100644 toolchain/mfc/test_case_validator.py diff --git a/src/simulation/m_checker.fpp b/src/simulation/m_checker.fpp index 3d752f1c3..7c5cc45df 100644 --- a/src/simulation/m_checker.fpp +++ b/src/simulation/m_checker.fpp @@ -35,60 +35,6 @@ contains end if end if - call s_check_inputs_time_stepping - - @:PROHIBIT(chemistry .and. chem_params%reaction_substeps < 0, & - & "chem_params%reaction_substeps must be >= 0 (0 = reaction source in the flow RHS; > 0 = operator-split sub-stepping)") - - @:PROHIBIT(chemistry .and. igr .and. chem_params%reaction_substeps > 0, & - & "operator-split reaction sub-stepping (reaction_substeps > 0) is not supported with igr: the reactor reads the post-flow (rho, e, T) state, which the IGR update path does not guarantee") - - @:PROHIBIT(chemistry .and. chem_params%adap_substeps .and. chem_params%reaction_substeps < 1, & - & "chem_params%adap_substeps requires reaction_substeps >= 1 (the operator-split floor)") - - @:PROHIBIT(chemistry .and. chem_params%adap_substeps & - & .and. chem_params%reaction_substeps_max < chem_params%reaction_substeps, & - & "chem_params%reaction_substeps_max must be >= reaction_substeps when adap_substeps = T") - - ! Chemistry with Euler bubbles is not currently supported: the IBM image-point - ! interpolation selects the bubbles/QBMM branch before the chemistry branch, so the - ! species state (Ys_IP) is not carried when both are enabled. Disallow until implemented. - @:PROHIBIT(chemistry .and. (bubbles_euler .or. qbmm), & - & "chemistry is not currently supported with Euler bubbles (bubbles_euler/qbmm)") - - @:PROHIBIT(ib_state_wrt .and. .not. ib, "ib_state_wrt requires ib to be enabled") - @:PROHIBIT(many_ib_patch_parallelism .and. .not. ib, "many_ib_patch_parallelism requires ib to be enabled") - - @:PROHIBIT(bf_spatial_support .and. (n == 0 .or. p /= 0), & - & "bf_spatial_support is implemented for 2D only (it forces mom%beg and mom%beg+1)") - - ! Condensed-phase reactive burn assumes exactly two fluids (reactant=1, product=2) that share the - ! stiffened-gas EOS and differ only in qv; violating these silently corrupts the mass/energy balance. - @:PROHIBIT(reactive_burn .and. num_fluids /= 2, "reactive_burn requires num_fluids = 2 (reactant then product)") - @:PROHIBIT(reactive_burn .and. .not. f_approx_equal(fluid_pp(1)%gamma, fluid_pp(2)%gamma), & - & "reactive_burn requires fluid_pp(1)%gamma == fluid_pp(2)%gamma (reactant and product share the EOS)") - @:PROHIBIT(reactive_burn .and. .not. f_approx_equal(fluid_pp(1)%pi_inf, fluid_pp(2)%pi_inf), & - & "reactive_burn requires fluid_pp(1)%pi_inf == fluid_pp(2)%pi_inf (reactant and product share the EOS)") - @:PROHIBIT(reactive_burn .and. fluid_pp(1)%qv <= fluid_pp(2)%qv, & - & "reactive_burn requires fluid_pp(1)%qv > fluid_pp(2)%qv (reactant releases energy on conversion to product)") - @:PROHIBIT(reactive_burn .and. rburn%pref <= 0._wp, & - & "reactive_burn requires rburn%pref > 0 (it normalizes the pressure drive (p - rburn%pign)/rburn%pref and is used as a divisor)") - ! The rate uses rburn%k, rburn%pign, rburn%n directly; each defaults to the sentinel dflt_real, - ! so an unset value silently produces spurious ignition (pign), NaN via drive**n (n), or a - ! backward reaction (k). Require each to be set to a physical value. - @:PROHIBIT(reactive_burn .and. rburn%k <= 0._wp, & - & "reactive_burn requires rburn%k > 0 (rate coefficient [1/s]; unset defaults to a negative sentinel that runs the reaction backward)") - @:PROHIBIT(reactive_burn .and. f_is_default(rburn%pign), & - & "reactive_burn requires rburn%pign to be set (ignition pressure threshold [Pa]; unset defaults to a negative sentinel, so the reactant ignites everywhere from t = 0)") - @:PROHIBIT(reactive_burn .and. rburn%n < 0._wp, & - & "reactive_burn requires rburn%n >= 0 (pressure-drive exponent; unset defaults to a negative sentinel, so drive**n overflows to Inf and the field goes NaN)") - @:PROHIBIT(reactive_burn .and. model_eqns /= 2 .and. model_eqns /= 3, & - & "reactive_burn requires model_eqns = 2 or 3 (the 5-equation pressure-equilibrium or 6-equation multi-fluid model)") - @:PROHIBIT(reactive_burn .and. rburn%ta < 0._wp, & - & "reactive_burn requires rburn%ta >= 0 (activation temperature [K]; 0 disables the Arrhenius factor)") - @:PROHIBIT(reactive_burn .and. rburn%ta > 0._wp .and. fluid_pp(1)%cv <= 0._wp, & - & "reactive_burn with rburn%ta > 0 requires fluid_pp(1)%cv > 0 (the reactant temperature T = (p + pi_inf)/((gamma - 1) cv rho) needs a physical heat capacity; cv = 0 silently disables the Arrhenius factor)") - if (ib .and. chemistry) then call s_check_inputs_ib_injection end if @@ -148,15 +94,6 @@ contains end subroutine s_check_inputs_muscl - !> Checks constraints on time stepping parameters - impure subroutine s_check_inputs_time_stepping - - if (.not. cfl_dt) then - @:PROHIBIT(dt <= 0) - end if - - end subroutine s_check_inputs_time_stepping - !> Validate NVIDIA unified virtual memory configuration parameters impure subroutine s_check_inputs_nvidia_uvm diff --git a/toolchain/mfc/case_validator.py b/toolchain/mfc/case_validator.py index e85336a9f..a806eb40a 100644 --- a/toolchain/mfc/case_validator.py +++ b/toolchain/mfc/case_validator.py @@ -12,6 +12,7 @@ - src/post_process/m_checker.fpp """ +import math import re from functools import lru_cache from typing import Any, Dict, List, Set @@ -611,6 +612,7 @@ def check_ibm(self): num_particle_clouds = self.get("num_particle_clouds", 0) or 0 ib_state_wrt = self.get("ib_state_wrt", "F") == "T" + many_ib_patch_parallelism = self.get("many_ib_patch_parallelism", "F") == "T" fd_order = self.get("fd_order") self.prohibit(ib and fd_order is None, "fd_order must be specified for ib") @@ -627,6 +629,7 @@ def check_ibm(self): ) self.prohibit(not ib and num_ibs > 0, "num_ibs is set, but ib is not enabled") self.prohibit(ib_state_wrt and not ib, "ib_state_wrt requires ib to be enabled") + self.prohibit(many_ib_patch_parallelism and not ib, "many_ib_patch_parallelism requires ib to be enabled") for i in range(1, num_particle_clouds + 1): packing_method = self.get(f"particle_cloud({i})%packing_method", None) @@ -940,6 +943,13 @@ def check_bubbles_euler_simulation(self): def check_body_forces(self): """Checks constraints on body forces parameters""" + # Spatially supported forcing writes mom%beg and mom%beg+1 directly, so it is + # only defined for a 2D domain (n > 0 with p == 0). + bf_spatial_support = self.get("bf_spatial_support", "F") == "T" + n = self.get("n", 0) or 0 + p = self.get("p", 0) or 0 + self.prohibit(bf_spatial_support and (n == 0 or p != 0), "bf_spatial_support is implemented for 2D only (it forces mom%beg and mom%beg+1)") + for dir in ["x", "y", "z"]: bf = self.get(f"bf_{dir}", "F") == "T" @@ -1516,6 +1526,31 @@ def check_chemistry(self): qbmm = self.get("qbmm", "F") == "T" self.prohibit(chemistry and (bubbles_euler or qbmm), "chemistry is not currently supported with Euler bubbles (bubbles_euler / qbmm)") + # Operator-split reaction sub-stepping. The Fortran defaults are + # reaction_substeps = reaction_substeps_max = 0 and adap_substeps = F, so an + # unset value is treated as 0 here to match. + igr = self.get("igr", "F") == "T" + adap_substeps = self.get("chem_params%adap_substeps", "F") == "T" + reaction_substeps = self.get("chem_params%reaction_substeps", 0) or 0 + reaction_substeps_max = self.get("chem_params%reaction_substeps_max", 0) or 0 + + self.prohibit( + chemistry and reaction_substeps < 0, + "chem_params%reaction_substeps must be >= 0 (0 = reaction source in the flow RHS; > 0 = operator-split sub-stepping)", + ) + self.prohibit( + chemistry and igr and reaction_substeps > 0, + "operator-split reaction sub-stepping (reaction_substeps > 0) is not supported with igr: the reactor reads the post-flow (rho, e, T) state, which the IGR update path does not guarantee", + ) + self.prohibit( + chemistry and adap_substeps and reaction_substeps < 1, + "chem_params%adap_substeps requires reaction_substeps >= 1 (the operator-split floor)", + ) + self.prohibit( + chemistry and adap_substeps and reaction_substeps_max < reaction_substeps, + "chem_params%reaction_substeps_max must be >= reaction_substeps when adap_substeps = T", + ) + # Define what constitutes a wall (-15 for slip, -16 for no-slip) wall_bcs = [-15, -16] @@ -1559,6 +1594,25 @@ def check_reactive_burn(self): model_eqns = self.get("model_eqns") # Supported on the 5-equation (pressure-equilibrium) and 6-equation multi-fluid models. self.prohibit(model_eqns is not None and model_eqns not in (2, 3), "reactive_burn requires model_eqns = 2 or 3 (5- or 6-equation multi-fluid model)") + + # Exactly two fluids (reactant = 1, product = 2) sharing the stiffened-gas EOS and + # differing only in qv; violating these silently corrupts the mass/energy balance. + num_fluids = self.get("num_fluids") + self.prohibit(num_fluids is not None and num_fluids != 2, "reactive_burn requires num_fluids = 2 (reactant then product)") + for prop in ("gamma", "pi_inf"): + v1 = self.get(f"fluid_pp(1)%{prop}") + v2 = self.get(f"fluid_pp(2)%{prop}") + self.prohibit( + self._is_numeric(v1) and self._is_numeric(v2) and not math.isclose(v1, v2, rel_tol=1e-10), + f"reactive_burn requires fluid_pp(1)%{prop} == fluid_pp(2)%{prop} (reactant and product share the EOS)", + ) + # qv defaults to 0 in the Fortran, so an unset value is treated as 0 here to match. + qv1 = self.get("fluid_pp(1)%qv", 0.0) + qv2 = self.get("fluid_pp(2)%qv", 0.0) + self.prohibit( + self._is_numeric(qv1) and self._is_numeric(qv2) and qv1 <= qv2, + "reactive_burn requires fluid_pp(1)%qv > fluid_pp(2)%qv (reactant releases energy on conversion to product)", + ) # The rate uses rburn%k, %pign, %pref, %n directly; an unset value defaults to a negative # sentinel in the solver and silently corrupts the burn, so require each to be set. rk = self.get("rburn%k") diff --git a/toolchain/mfc/test_case_validator.py b/toolchain/mfc/test_case_validator.py new file mode 100644 index 000000000..876eed656 --- /dev/null +++ b/toolchain/mfc/test_case_validator.py @@ -0,0 +1,197 @@ +""" +Unit tests for case_validator.py constraint checks. + +These cover constraints that are enforced only in Python: the Fortran +m_checker*.fpp counterparts were removed, so a check that silently stops +firing would otherwise go unnoticed (validating the example cases only +exercises configurations that are meant to pass). +""" + +import unittest + +from .case_validator import CaseValidator + +# A minimal 1D case that passes simulation validation. +BASE = { + "m": 50, + "n": 0, + "p": 0, + "model_eqns": 2, + "num_fluids": 1, + "num_patches": 1, + "t_step_start": 0, + "t_step_stop": 100, + "t_step_save": 10, + "dt": 1e-6, + "weno_order": 5, + "weno_eps": 1e-6, + "riemann_solver": 2, + "wave_speeds": 1, + "avg_state": 2, + "bc_x%beg": -1, + "bc_x%end": -1, + "x_domain%beg": 0.0, + "x_domain%end": 1.0, + "patch_icpp(1)%geometry": 1, + "patch_icpp(1)%x_centroid": 0.5, + "patch_icpp(1)%length_x": 1.0, + "patch_icpp(1)%vel(1)": 0.0, + "patch_icpp(1)%pres": 1.0, + "patch_icpp(1)%alpha_rho(1)": 1.0, + "patch_icpp(1)%alpha(1)": 1.0, + "fluid_pp(1)%gamma": 0.4, + "fluid_pp(1)%pi_inf": 0.0, +} + +BASE_2D = { + **BASE, + "n": 50, + "bc_y%beg": -1, + "bc_y%end": -1, + "y_domain%beg": 0.0, + "y_domain%end": 1.0, + "patch_icpp(1)%y_centroid": 0.5, + "patch_icpp(1)%length_y": 1.0, + "patch_icpp(1)%vel(2)": 0.0, +} + +# A reactive-burn case satisfying every rburn constraint. +REACTIVE_BURN = { + **BASE, + "num_fluids": 2, + "reactive_burn": "T", + "rburn%k": 1.0e6, + "rburn%pign": 1.0e8, + "rburn%pref": 1.0e8, + "rburn%n": 1.0, + "rburn%ta": 0.0, + "fluid_pp(1)%qv": 1.0e6, + "fluid_pp(2)%gamma": 0.4, + "fluid_pp(2)%pi_inf": 0.0, + "fluid_pp(2)%qv": 0.0, + "patch_icpp(1)%alpha_rho(2)": 0.0, + "patch_icpp(1)%alpha(2)": 0.0, +} + +CHEMISTRY = {**BASE, "chemistry": "T", "cantera_file": "h2o2.yaml"} + + +class ConstraintTestCase(unittest.TestCase): + """Base class providing assertions over simulation-stage validation.""" + + def errors_for(self, params) -> str: + """Return all simulation-stage validation errors for params, joined.""" + validator = CaseValidator(dict(params)) + try: + validator.validate("simulation") + except Exception as exc: # CaseConstraintError + return str(exc) + return "" + + def assertRejects(self, params, expected: str): + """params must fail validation with expected in the message.""" + errors = self.errors_for(params) + self.assertIn(expected, errors) + + def assertAccepts(self, params, unexpected: str): + """params must not trip the check identified by unexpected.""" + errors = self.errors_for(params) + self.assertNotIn(unexpected, errors) + + +class TestImmersedBoundaryFlags(ConstraintTestCase): + MSG = "many_ib_patch_parallelism requires ib" + + def test_requires_ib(self): + self.assertRejects({**BASE, "many_ib_patch_parallelism": "T"}, self.MSG) + + def test_not_tripped_when_disabled(self): + self.assertAccepts(BASE, self.MSG) + + +class TestBodyForceSpatialSupport(ConstraintTestCase): + MSG = "bf_spatial_support is implemented for 2D only" + + def test_rejects_1d(self): + self.assertRejects({**BASE, "bf_spatial_support": "T"}, self.MSG) + + def test_rejects_3d(self): + self.assertRejects({**BASE_2D, "p": 50, "bf_spatial_support": "T"}, self.MSG) + + def test_accepts_2d(self): + self.assertAccepts({**BASE_2D, "bf_spatial_support": "T"}, self.MSG) + + +class TestChemistrySubstepping(ConstraintTestCase): + def test_rejects_negative_substeps(self): + self.assertRejects({**CHEMISTRY, "chem_params%reaction_substeps": -1}, "reaction_substeps must be >= 0") + + def test_rejects_substepping_with_igr(self): + self.assertRejects({**CHEMISTRY, "igr": "T", "chem_params%reaction_substeps": 2}, "not supported with igr") + + def test_rejects_adaptive_without_substeps(self): + """adap_substeps with reaction_substeps unset: the Fortran default is 0, below the floor of 1.""" + self.assertRejects({**CHEMISTRY, "chem_params%adap_substeps": "T"}, "requires reaction_substeps >= 1") + + def test_rejects_adaptive_with_zero_substeps(self): + self.assertRejects({**CHEMISTRY, "chem_params%adap_substeps": "T", "chem_params%reaction_substeps": 0}, "requires reaction_substeps >= 1") + + def test_rejects_max_below_floor(self): + params = {**CHEMISTRY, "chem_params%adap_substeps": "T", "chem_params%reaction_substeps": 5, "chem_params%reaction_substeps_max": 2} + self.assertRejects(params, "reaction_substeps_max must be >=") + + def test_rejects_max_unset_below_floor(self): + """reaction_substeps_max unset defaults to 0 in the Fortran, below a floor of 5.""" + self.assertRejects({**CHEMISTRY, "chem_params%adap_substeps": "T", "chem_params%reaction_substeps": 5}, "reaction_substeps_max must be >=") + + def test_accepts_no_substepping(self): + self.assertAccepts(CHEMISTRY, "reaction_substeps") + + def test_accepts_valid_adaptive_substepping(self): + params = {**CHEMISTRY, "chem_params%adap_substeps": "T", "chem_params%reaction_substeps": 2, "chem_params%reaction_substeps_max": 8} + self.assertAccepts(params, "reaction_substeps") + + def test_accepts_igr_without_substepping(self): + self.assertAccepts({**CHEMISTRY, "igr": "T", "chem_params%reaction_substeps": 0}, "not supported with igr") + + +class TestReactiveBurnFluidPairing(ConstraintTestCase): + def test_rejects_wrong_num_fluids(self): + self.assertRejects({**REACTIVE_BURN, "num_fluids": 3}, "reactive_burn requires num_fluids = 2") + + def test_rejects_gamma_mismatch(self): + self.assertRejects({**REACTIVE_BURN, "fluid_pp(2)%gamma": 0.5}, "fluid_pp(1)%gamma == fluid_pp(2)%gamma") + + def test_rejects_pi_inf_mismatch(self): + self.assertRejects({**REACTIVE_BURN, "fluid_pp(2)%pi_inf": 1.0e5}, "fluid_pp(1)%pi_inf == fluid_pp(2)%pi_inf") + + def test_rejects_equal_qv(self): + self.assertRejects({**REACTIVE_BURN, "fluid_pp(1)%qv": 0.0}, "fluid_pp(1)%qv > fluid_pp(2)%qv") + + def test_rejects_inverted_qv(self): + self.assertRejects({**REACTIVE_BURN, "fluid_pp(1)%qv": 0.0, "fluid_pp(2)%qv": 1.0e6}, "fluid_pp(1)%qv > fluid_pp(2)%qv") + + def test_rejects_unset_qv(self): + """qv defaults to 0 in the Fortran, so leaving both unset means no energy release.""" + params = {k: v for k, v in REACTIVE_BURN.items() if not k.endswith("%qv")} + self.assertRejects(params, "fluid_pp(1)%qv > fluid_pp(2)%qv") + + def test_accepts_valid_configuration(self): + self.assertEqual(self.errors_for(REACTIVE_BURN), "") + + +class TestTimeStepPositivity(ConstraintTestCase): + MSG = "dt must be positive" + + def test_rejects_negative_dt(self): + self.assertRejects({**BASE, "dt": -1.0}, self.MSG) + + def test_rejects_zero_dt(self): + self.assertRejects({**BASE, "dt": 0.0}, self.MSG) + + def test_accepts_positive_dt(self): + self.assertAccepts(BASE, self.MSG) + + +if __name__ == "__main__": + unittest.main() From 7c6cfbcf9af7cac2224c93705c0f176d3e22fd92 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 9 Aug 2026 15:35:55 -0400 Subject: [PATCH 2/8] Finish the checker migration, delete dead params_tests, enforce the split Three follow-ons to the previous commit, which moved the input-only simulation checks into case_validator. Migrate the last input-only Fortran checks ------------------------------------------ - particle_cloud packing_method (2 checks) and the muscl_order/int_comp check were already duplicated in case_validator; drop the Fortran. - synthetic-turbulence forcing zones (3 checks) had no Python counterpart; add check_synthetic_turbulence, covering num_turbulent_sources bounds and per-zone turb_pos / synth_L over d = 1..num_dims, matching the Fortran loop so trailing components of a lower-dimensional case stay optional. s_check_inputs_particle_clouds and s_check_inputs_synthetic_turbulence are removed along with their call sites, and m_helper_basic and muscl_order_first_order are no longer used by m_checker. src/simulation/m_checker.fpp: 162 -> 113 lines (225 before this branch). Delete the params_tests CLI island ---------------------------------- coverage.py, inventory.py, mutation_tests.py, negative_tests.py, runner.py, and snapshot.py: 1651 lines that collect zero pytest tests. Each is imported only by runner.py, which nothing imports and which appears in no workflow, script, doc, or CMake file. Its entire history is incidental edits from unrelated feature PRs -- #1713 had to update mutation_tests.py to drop model_eqns = 4 earlier today. It could not be promoted as-is: negative_tests decides whether the right error fired with a "half the key terms appear" substring heuristic, and runner verify needs a data/ baseline that is gitignored. The sibling test_*.py files, which supply all 171 real tests in the package, stay. Enforce the split going forward ------------------------------- Add check_checker_input_constraints to lint_source. A @:PROHIBIT in src/**/m_checker*.fpp is allowed only inside a subroutine listed in RUNTIME_CHECKER_SUBROUTINES -- the ones that depend on the MPI decomposition, per-rank grid extents, the active compiler, or a Cantera-populated value -- or on a line marked "! lint: runtime-check ". This is what stops the next feature PR from re-creating the double entry this branch removed: reactive_burn added nine constraints to both languages in a single PR. --- src/simulation/m_checker.fpp | 52 +- toolchain/mfc/case_validator.py | 41 ++ toolchain/mfc/lint_source.py | 72 +++ toolchain/mfc/params_tests/.gitignore | 3 - toolchain/mfc/params_tests/__init__.py | 5 +- toolchain/mfc/params_tests/coverage.py | 280 ----------- toolchain/mfc/params_tests/inventory.py | 147 ------ toolchain/mfc/params_tests/mutation_tests.py | 257 ---------- toolchain/mfc/params_tests/negative_tests.py | 470 ------------------- toolchain/mfc/params_tests/runner.py | 234 --------- toolchain/mfc/params_tests/snapshot.py | 263 ----------- toolchain/mfc/test_case_validator.py | 43 ++ 12 files changed, 158 insertions(+), 1709 deletions(-) delete mode 100644 toolchain/mfc/params_tests/coverage.py delete mode 100644 toolchain/mfc/params_tests/inventory.py delete mode 100644 toolchain/mfc/params_tests/mutation_tests.py delete mode 100644 toolchain/mfc/params_tests/negative_tests.py delete mode 100644 toolchain/mfc/params_tests/runner.py delete mode 100644 toolchain/mfc/params_tests/snapshot.py diff --git a/src/simulation/m_checker.fpp b/src/simulation/m_checker.fpp index 7c5cc45df..0c6324b2e 100644 --- a/src/simulation/m_checker.fpp +++ b/src/simulation/m_checker.fpp @@ -11,8 +11,7 @@ module m_checker use m_global_parameters use m_mpi_proxy use m_helper - use m_helper_basic - use m_constants, only: recon_type_weno, recon_type_muscl, muscl_order_first_order + use m_constants, only: recon_type_weno, recon_type_muscl implicit none @@ -39,14 +38,6 @@ contains call s_check_inputs_ib_injection end if - if (num_particle_clouds > 0) then - call s_check_inputs_particle_clouds - end if - - if (synthetic_turbulence) then - call s_check_inputs_synthetic_turbulence - end if - end subroutine s_check_inputs !> Checks constraints on compiler options @@ -89,8 +80,6 @@ contains @:PROHIBIT(p + 1 < min(1, p)*num_stcls_min*muscl_order, & & "For 3D simulation, p must be greater than or equal to (num_stcls_min*muscl_order - 1), whose value is " & & // trim(numStr)) - @:PROHIBIT(muscl_order == muscl_order_first_order .and. int_comp > 0, & - & "int_comp requires muscl_order >= 2 (muscl_order=1 leaves the reconstruction workspace uninitialised)") end subroutine s_check_inputs_muscl @@ -120,43 +109,4 @@ contains end subroutine s_check_inputs_ib_injection - !> Checks that each active particle cloud has a valid packing_method specified - impure subroutine s_check_inputs_particle_clouds - - integer :: i - character(len=5) :: idxStr - - do i = 1, num_particle_clouds - call s_int_to_str(i, idxStr) - @:PROHIBIT(particle_cloud(i)%packing_method == dflt_int, & - & "particle_cloud("//trim(idxStr) & - & //")%packing_method must be specified (1 = rejection sampling, 2 = lattice)") - @:PROHIBIT(particle_cloud(i)%packing_method /= 1 .and. particle_cloud(i)%packing_method /= 2, & - & "particle_cloud("//trim(idxStr) //")%packing_method must be 1 (rejection sampling) or 2 (lattice)") - end do - - end subroutine s_check_inputs_particle_clouds - - !> Checks that each active synthetic-turbulence forcing zone has a fully specified position and a positive size in every active - !! dimension - impure subroutine s_check_inputs_synthetic_turbulence - - integer :: i, d - character(len=5) :: idxStr - - @:PROHIBIT(num_turbulent_sources <= 0, "num_turbulent_sources must be > 0 when synthetic_turbulence is enabled") - - do i = 1, num_turbulent_sources - call s_int_to_str(i, idxStr) - do d = 1, num_dims - @:PROHIBIT(f_is_default(turb_pos(i, d)), & - & "turb_pos("//trim(idxStr) & - & //",:) must be specified for all num_dims when synthetic_turbulence is enabled") - @:PROHIBIT(f_is_default(synth_L(i, d)) .or. synth_L(i, d) <= 0._wp, & - & "synth_L("//trim(idxStr)//",:) must be positive for all num_dims when synthetic_turbulence is enabled") - end do - end do - - end subroutine s_check_inputs_synthetic_turbulence - end module m_checker diff --git a/toolchain/mfc/case_validator.py b/toolchain/mfc/case_validator.py index a806eb40a..45ddbb643 100644 --- a/toolchain/mfc/case_validator.py +++ b/toolchain/mfc/case_validator.py @@ -205,6 +205,16 @@ "references": ["Papanastasiou87"], "docs_section": "sec-non-newtonian", }, + # Forcing + "check_synthetic_turbulence": { + "title": "Synthetic Turbulence Forcing", + "category": "Feature Compatibility", + "explanation": ( + "num_turbulent_sources must be > 0 and <= num_turb_sources_max. Each active forcing zone i needs " + "turb_pos(i,d) set and synth_L(i,d) > 0 for every active dimension d (d = 1..num_dims); components " + "beyond num_dims are unused and not required." + ), + }, # Acoustic Sources "check_acoustic_source": { "title": "Acoustic Sources", @@ -941,6 +951,36 @@ def check_bubbles_euler_simulation(self): self.prohibit(avg_state is not None and avg_state != 2, "Bubble modeling requires arithmetic average (avg_state = 2)") self.prohibit(model_eqns == 2 and bubble_model == 1, "The 5-equation bubbly flow model does not support bubble_model = 1 (Gilmore)") + def check_synthetic_turbulence(self): + """Checks constraints on synthetic-turbulence forcing zones (simulation)""" + if self.get("synthetic_turbulence", "F") != "T": + return + + num_turbulent_sources = self.get("num_turbulent_sources", 0) or 0 + self.prohibit(num_turbulent_sources <= 0, "num_turbulent_sources must be > 0 when synthetic_turbulence is enabled") + + num_turb_sources_max = get_fortran_constants().get("num_turb_sources_max", 10) + self.prohibit( + num_turbulent_sources > num_turb_sources_max, + f"num_turbulent_sources must be <= {num_turb_sources_max} (num_turb_sources_max in m_constants.fpp)", + ) + + # Each active zone needs a position and a positive extent in every active + # dimension; the Fortran loops d = 1, num_dims, so trailing components of a + # lower-dimensional case are not required. + num_dims = 3 if (self.get("p", 0) or 0) > 0 else (2 if (self.get("n", 0) or 0) > 0 else 1) + for i in range(1, min(num_turbulent_sources, num_turb_sources_max) + 1): + for d in range(1, num_dims + 1): + self.prohibit( + not self.is_set(f"turb_pos({i},{d})"), + f"turb_pos({i},{d}) must be specified for all num_dims when synthetic_turbulence is enabled", + ) + synth_l = self.get(f"synth_L({i},{d})") + self.prohibit( + synth_l is None or (self._is_numeric(synth_l) and synth_l <= 0), + f"synth_L({i},{d}) must be positive for all num_dims when synthetic_turbulence is enabled", + ) + def check_body_forces(self): """Checks constraints on body forces parameters""" # Spatially supported forcing writes mom%beg and mom%beg+1 directly, so it is @@ -2373,6 +2413,7 @@ def validate_simulation(self): self.check_model_eqns_simulation() self.check_bubbles_euler_simulation() self.check_body_forces() + self.check_synthetic_turbulence() self.check_viscosity() self.check_non_newtonian() self.check_mhd_simulation() diff --git a/toolchain/mfc/lint_source.py b/toolchain/mfc/lint_source.py index 535119207..b3ca0d90f 100644 --- a/toolchain/mfc/lint_source.py +++ b/toolchain/mfc/lint_source.py @@ -33,6 +33,29 @@ # MPI proxy source directory -> params-registry target key MPI_PROXY_TARGETS = {"pre_process": "pre", "simulation": "sim", "post_process": "post"} +# Checker subroutines allowed to hold @:PROHIBIT. Every one of these depends on +# state the Python validator cannot see at case-validation time: the MPI +# decomposition, per-rank grid extents, the active compiler, or a value Cantera +# fills in at runtime. Constraints between input parameters belong in +# toolchain/mfc/case_validator.py instead -- see check_checker_input_constraints. +RUNTIME_CHECKER_SUBROUTINES = { + # Compiler conditionals (#ifdef / #if guarded). + "s_check_amd", + "s_check_inputs_compilers", + "s_check_inputs_nvidia_uvm", + # MPI decomposition: n_global, num_procs_y/z. + "s_check_total_cells", + "s_check_inputs_fft", + # Per-rank grid extents m/n/p, which differ from the case-file values. + "s_check_inputs_weno", + "s_check_inputs_muscl", + # num_species is populated by Cantera at runtime. + "s_check_inputs_ib_injection", +} + +# Opt out of check_checker_input_constraints for a single @:PROHIBIT. +RUNTIME_CHECK_MARKER = "lint: runtime-check" + def _is_comment_or_blank(stripped: str) -> bool: """True if stripped line is blank, a Fortran comment, or a Fypp directive.""" @@ -437,6 +460,54 @@ def check_manual_registry_bcasts(repo_root: Path) -> list[str]: return errors +def check_checker_input_constraints(repo_root: Path) -> list[str]: + """Keep input-only constraints out of the Fortran m_checker files. + + Constraints between case-file parameters are enforced in + toolchain/mfc/case_validator.py, which runs before any binary is invoked. + Adding them to Fortran as well means the same rule is written twice, in two + languages, and the copies drift. + + A @:PROHIBIT is allowed only inside a subroutine in + RUNTIME_CHECKER_SUBROUTINES, or on a line preceded by a + "! lint: runtime-check " comment. + """ + errors = [] + + for path in sorted((repo_root / SRC_DIR).rglob("m_checker*.fpp")): + rel = path.relative_to(repo_root) + subroutine = None + exempt_next = False + + for lineno, line in enumerate(path.read_text().splitlines(), 1): + stripped = line.strip() + + match = re.match(r"(?:impure\s+|pure\s+)?subroutine\s+(\w+)", stripped) + if match: + subroutine = match.group(1) + elif stripped.startswith("end subroutine"): + subroutine = None + + if stripped.startswith("!"): + exempt_next = RUNTIME_CHECK_MARKER in stripped + continue + + if "@:PROHIBIT" in stripped and not exempt_next: + if subroutine not in RUNTIME_CHECKER_SUBROUTINES: + where = f"in {subroutine}" if subroutine else "at module scope" + errors.append( + f"{rel}:{lineno}: @:PROHIBIT {where} looks like an input-only constraint. " + f"Add it to a check_* method in toolchain/mfc/case_validator.py instead. " + f"If it genuinely needs runtime or compiler state, add {subroutine!r} to " + f"RUNTIME_CHECKER_SUBROUTINES in {Path(__file__).name}, or mark the line with " + f"'! {RUNTIME_CHECK_MARKER} '." + ) + + exempt_next = False + + return errors + + def main(): repo_root = Path(__file__).resolve().parents[2] @@ -450,6 +521,7 @@ def main(): all_errors.extend(check_duplicate_lines(repo_root)) all_errors.extend(check_hardcoded_byte_size(repo_root)) all_errors.extend(check_manual_registry_bcasts(repo_root)) + all_errors.extend(check_checker_input_constraints(repo_root)) if all_errors: print("Source lint failed:") diff --git a/toolchain/mfc/params_tests/.gitignore b/toolchain/mfc/params_tests/.gitignore index 588fff811..345c6b741 100644 --- a/toolchain/mfc/params_tests/.gitignore +++ b/toolchain/mfc/params_tests/.gitignore @@ -1,5 +1,2 @@ -# Generated data files - recreate with: python -m mfc.params_tests.runner build -data/ - # Python cache __pycache__/ diff --git a/toolchain/mfc/params_tests/__init__.py b/toolchain/mfc/params_tests/__init__.py index 3e6982f8e..a9fab8710 100644 --- a/toolchain/mfc/params_tests/__init__.py +++ b/toolchain/mfc/params_tests/__init__.py @@ -1,8 +1,5 @@ """ Parameter Validation Test Infrastructure. -This package provides tools for: -- Exporting parameter inventory -- Capturing validation snapshots -- Comparing validation behavior across refactoring +Unit tests for the parameter registry, schema, generators, and validation. """ diff --git a/toolchain/mfc/params_tests/coverage.py b/toolchain/mfc/params_tests/coverage.py deleted file mode 100644 index 47b8ab733..000000000 --- a/toolchain/mfc/params_tests/coverage.py +++ /dev/null @@ -1,280 +0,0 @@ -""" -Constraint Coverage Analysis Tool. - -Analyzes which validation constraints are exercised by the test cases. -This helps identify gaps in test coverage before refactoring. -""" - -import ast -import json -from dataclasses import dataclass -from pathlib import Path -from typing import Any, Dict, List - - -@dataclass -class ConstraintInfo: - """ - Information about a single constraint. - - Attributes: - method: Name of the check_* method containing this constraint. - line_number: Line number of the prohibit() call start (1-indexed). - For multi-line calls, this is the first line. - message: Error message shown when constraint is violated. - condition_code: Unparsed source code of the condition expression. - """ - - method: str - line_number: int - message: str - condition_code: str - - -def _extract_message(msg_node: ast.expr) -> str: - """Extract message string from AST node.""" - if isinstance(msg_node, ast.Constant): - return msg_node.value - if isinstance(msg_node, ast.JoinedStr): - # f-string - extract the static parts - return "".join(p.value if isinstance(p, ast.Constant) else "{...}" for p in msg_node.values) - return "" - - -def _is_prohibit_call(node: ast.AST) -> bool: - """Check if node is a self.prohibit() call with enough arguments.""" - if not isinstance(node, ast.Call): - return False - if not isinstance(node.func, ast.Attribute): - return False - return node.func.attr == "prohibit" and len(node.args) >= 2 - - -def _find_case_validator_class(tree: ast.Module) -> ast.ClassDef: - """Find the CaseValidator class in the AST.""" - for node in tree.body: - if isinstance(node, ast.ClassDef) and node.name == "CaseValidator": - return node - return None - - -def extract_constraints_from_validator() -> List[ConstraintInfo]: - """Parse case_validator.py and extract all prohibit() calls.""" - validator_path = Path(__file__).parent.parent / "case_validator.py" - - with open(validator_path, "r", encoding="utf-8") as f: - source = f.read() - - tree = ast.parse(source) - validator_class = _find_case_validator_class(tree) - if validator_class is None: - return [] - - constraints: List[ConstraintInfo] = [] - - # Iterate through each method in the class - for item in validator_class.body: - if not isinstance(item, ast.FunctionDef): - continue - - # Walk only within this method to find prohibit() calls - for node in ast.walk(item): - if not _is_prohibit_call(node): - continue - - message = _extract_message(node.args[1]) - try: - condition_code = ast.unparse(node.args[0]) - except (ValueError, TypeError, AttributeError): - # ast.unparse can fail on malformed AST or missing attributes - condition_code = "" - - # Note: node.lineno points to the start of the prohibit() call. - # For multi-line calls, this is the first line, not where the - # condition or message appears. - constraints.append(ConstraintInfo(method=item.name, line_number=node.lineno, message=message, condition_code=condition_code)) - - return constraints - - -def _count_prohibit_calls(func_node: ast.FunctionDef) -> int: - """Count self.prohibit() calls in a function.""" - count = 0 - for subnode in ast.walk(func_node): - if _is_prohibit_call(subnode): - count += 1 - return count - - -def extract_check_methods() -> Dict[str, Dict[str, Any]]: - """Extract all check_* methods from validator with their stage.""" - validator_path = Path(__file__).parent.parent / "case_validator.py" - - with open(validator_path, "r", encoding="utf-8") as f: - source = f.read() - - methods = {} - tree = ast.parse(source) - validator_class = _find_case_validator_class(tree) - - if validator_class is None: - return methods - - for item in validator_class.body: - if not isinstance(item, ast.FunctionDef): - continue - if not item.name.startswith("check_"): - continue - - docstring = ast.get_docstring(item) or "" - methods[item.name] = { - "line_number": item.lineno, - "docstring": docstring.split("\n")[0] if docstring else "", - "prohibit_count": _count_prohibit_calls(item), - } - - return methods - - -_VALIDATE_METHOD_TO_STAGE = { - "validate_common": "common", - "validate_pre_process": "pre_process", - "validate_simulation": "simulation", - "validate_post_process": "post_process", -} - - -def _find_check_calls(func_node: ast.FunctionDef) -> List[str]: - """Find all self.check_* method calls in a function.""" - calls = [] - for subnode in ast.walk(func_node): - if not isinstance(subnode, ast.Call): - continue - if not isinstance(subnode.func, ast.Attribute): - continue - if subnode.func.attr.startswith("check_"): - calls.append(subnode.func.attr) - return calls - - -def extract_validate_dispatch() -> Dict[str, List[str]]: - """Extract which check methods are called for each stage.""" - validator_path = Path(__file__).parent.parent / "case_validator.py" - - with open(validator_path, "r", encoding="utf-8") as f: - source = f.read() - - dispatch = {stage: [] for stage in _VALIDATE_METHOD_TO_STAGE.values()} - tree = ast.parse(source) - validator_class = _find_case_validator_class(tree) - - if validator_class is None: - return dispatch - - for item in validator_class.body: - if not isinstance(item, ast.FunctionDef): - continue - stage = _VALIDATE_METHOD_TO_STAGE.get(item.name) - if stage is None: - continue - dispatch[stage].extend(_find_check_calls(item)) - - return dispatch - - -def generate_coverage_report() -> Dict[str, Any]: - """Generate a comprehensive coverage report.""" - constraints = extract_constraints_from_validator() - methods = extract_check_methods() - dispatch = extract_validate_dispatch() - - # Group constraints by method - by_method = {} - for c in constraints: - if c.method not in by_method: - by_method[c.method] = [] - by_method[c.method].append({"line": c.line_number, "message": c.message, "condition": c.condition_code[:80] + "..." if len(c.condition_code) > 80 else c.condition_code}) - - # Calculate coverage per stage - stage_coverage = {} - for stage, check_methods in dispatch.items(): - total_constraints = 0 - for method_name in check_methods: - if method_name in methods: - total_constraints += methods[method_name]["prohibit_count"] - stage_coverage[stage] = { - "methods": check_methods, - "method_count": len(check_methods), - "constraint_count": total_constraints, - } - - # Add common constraints to all stages - common_constraints = stage_coverage.get("common", {}).get("constraint_count", 0) - for stage in ["pre_process", "simulation", "post_process"]: - if stage in stage_coverage: - stage_coverage[stage]["total_with_common"] = stage_coverage[stage]["constraint_count"] + common_constraints - - return { - "summary": { - "total_constraints": len(constraints), - "total_check_methods": len(methods), - "methods_with_most_constraints": sorted([(name, info["prohibit_count"]) for name, info in methods.items()], key=lambda x: -x[1])[:10], - }, - "stage_coverage": stage_coverage, - "methods": methods, - "constraints_by_method": by_method, - } - - -def print_coverage_report(): - """Print coverage report to console.""" - report = generate_coverage_report() - - print("=" * 70) - print("MFC Case Validator Constraint Coverage Report") - print("=" * 70) - - print(f"\nTotal constraints (self.prohibit calls): {report['summary']['total_constraints']}") - print(f"Total check methods: {report['summary']['total_check_methods']}") - - print("\nMethods with most constraints:") - for method, count in report["summary"]["methods_with_most_constraints"]: - print(f" {method}: {count} constraints") - - print("\nConstraints by stage:") - for stage, info in report["stage_coverage"].items(): - total = info.get("total_with_common", info["constraint_count"]) - print(f" {stage}:") - print(f" Methods: {info['method_count']}") - print(f" Constraints: {info['constraint_count']} (+ common = {total})") - - print("\n" + "=" * 70) - print("Detailed constraint listing (top methods):") - print("=" * 70) - - for method, count in report["summary"]["methods_with_most_constraints"][:5]: - print(f"\n{method} ({count} constraints):") - if method in report["constraints_by_method"]: - for c in report["constraints_by_method"][method][:5]: - print(f" L{c['line']}: {c['message'][:60]}") - if len(report["constraints_by_method"][method]) > 5: - print(f" ... and {len(report['constraints_by_method'][method]) - 5} more") - - -def save_coverage_report(output_path: Path = None): - """Save coverage report to JSON file.""" - if output_path is None: - output_path = Path(__file__).parent / "constraint_coverage.json" - - report = generate_coverage_report() - - with open(output_path, "w", encoding="utf-8") as f: - json.dump(report, f, indent=2) - - return output_path - - -if __name__ == "__main__": - print_coverage_report() - path = save_coverage_report() - print(f"\nReport saved to: {path}") diff --git a/toolchain/mfc/params_tests/inventory.py b/toolchain/mfc/params_tests/inventory.py deleted file mode 100644 index 6f139571c..000000000 --- a/toolchain/mfc/params_tests/inventory.py +++ /dev/null @@ -1,147 +0,0 @@ -""" -Parameter Inventory Export Tool. - -Exports all MFC parameters with their types and tags to JSON for analysis. -""" - -import json -import re -from pathlib import Path -from typing import Any, Dict - -from ..params import REGISTRY -from ..params.schema import ParamType -from ..run.case_dicts import ALL - - -def get_param_type_name(param_type) -> str: - """Convert ParamType to string name.""" - if isinstance(param_type, ParamType): - return param_type.name - return "UNKNOWN" - - -def export_parameter_inventory() -> Dict[str, Any]: - """Export complete parameter inventory with metadata.""" - # Count by type - by_type = { - "INT": [], - "REAL": [], - "LOG": [], - "STR": [], - "ANALYTIC_INT": [], - "ANALYTIC_REAL": [], - } - - # Count by tag - by_tag = {} - for tag in REGISTRY.get_all_tags(): - by_tag[tag] = [] - - inventory = { - "metadata": { - "total_parameters": len(ALL), - }, - "parameters": {}, - "by_type": by_type, - "by_tag": by_tag, - } - - for param_name, param_type in sorted(ALL.items()): - type_name = get_param_type_name(param_type) - param = REGISTRY.all_params.get(param_name) - - param_info = { - "type": type_name, - "tags": sorted(param.tags) if param else [], - } - - # Detect pattern-based parameters - if "(" in param_name: - # Extract pattern (e.g., "patch_icpp(1)%x_centroid" -> "patch_icpp({id})%x_centroid") - param_pattern = re.sub(r"\((\d+)\)", r"({id})", param_name) - param_pattern = re.sub(r"\((\d+),\s*(\d+)\)", r"({id1}, {id2})", param_pattern) - param_info["pattern"] = param_pattern - - inventory["parameters"][param_name] = param_info - - # Categorize by type - if type_name in by_type: - by_type[type_name].append(param_name) - - # Categorize by tag - if param: - for tag in param.tags: - if tag in by_tag: - by_tag[tag].append(param_name) - - return inventory - - -def export_parameter_patterns() -> Dict[str, Any]: - """Extract unique parameter patterns (for dynamic parameters).""" - patterns = {} - for param_name, param_type in ALL.items(): - if "(" not in param_name: - continue - - # Normalize the pattern - normalized = re.sub(r"\((\d+)\)", r"({N})", param_name) - normalized = re.sub(r"\((\d+),\s*(\d+)\)", r"({N}, {M})", normalized) - - if normalized not in patterns: - patterns[normalized] = {"examples": [], "type": get_param_type_name(param_type), "count": 0} - patterns[normalized]["examples"].append(param_name) - patterns[normalized]["count"] += 1 - - # Trim examples to max 3 - for pattern_data in patterns.values(): - pattern_data["examples"] = pattern_data["examples"][:3] - - return patterns - - -def save_inventory(output_path: Path = None): - """Save parameter inventory to JSON file.""" - if output_path is None: - output_path = Path(__file__).parent / "param_inventory.json" - - inventory = export_parameter_inventory() - inventory["patterns"] = export_parameter_patterns() - - with open(output_path, "w", encoding="utf-8") as f: - json.dump(inventory, f, indent=2) - - return output_path - - -def print_inventory_summary(): - """Print a summary of the parameter inventory.""" - inventory = export_parameter_inventory() - patterns = export_parameter_patterns() - - print("=" * 60) - print("MFC Parameter Inventory Summary") - print("=" * 60) - print(f"Total parameters: {inventory['metadata']['total_parameters']}") - print() - print("By type:") - for type_name, params in inventory["by_type"].items(): - print(f" - {type_name}: {len(params)}") - print() - print("By feature tag:") - for tag, params in sorted(inventory["by_tag"].items()): - if params: - print(f" - {tag}: {len(params)}") - print() - print(f"Dynamic parameter patterns: {len(patterns)}") - print("Top patterns:") - sorted_patterns = sorted(patterns.items(), key=lambda x: -x[1]["count"])[:10] - for pattern, info in sorted_patterns: - print(f" - {pattern}: {info['count']} instances ({info['type']})") - - -if __name__ == "__main__": - print_inventory_summary() - path = save_inventory() - print(f"\nInventory saved to: {path}") diff --git a/toolchain/mfc/params_tests/mutation_tests.py b/toolchain/mfc/params_tests/mutation_tests.py deleted file mode 100644 index d25eb0a0a..000000000 --- a/toolchain/mfc/params_tests/mutation_tests.py +++ /dev/null @@ -1,257 +0,0 @@ -""" -Mutation Testing for Validator Coverage. - -Takes valid example cases and systematically mutates parameters -to verify the validator catches invalid configurations. -""" - -import json -import subprocess -from dataclasses import dataclass -from pathlib import Path -from typing import Any, Dict, List, Tuple - -from ..case_validator import CaseValidator - - -@dataclass -class MutationResult: - """Result of a mutation test.""" - - case_name: str - param_name: str - original_value: Any - mutated_value: Any - validator_caught: bool - errors: List[str] - - -# Mutations to apply to parameters -MUTATIONS = { - # BASIC NUMERIC PARAMETERS - "m": [0, -1, None], - "n": [-1, -10], - "p": [-1, -5], - "dt": [0, -1e-6, None], - "t_step_start": [-1], - "t_step_stop": [-1], - "t_step_save": [0, -1], - "num_fluids": [0, -1], - "num_patches": [0, -1], - "model_eqns": [0, 5, 10, -1], - "weno_order": [0, 2, 4, 6, 8], - "time_stepper": [0, 6, -1], - "riemann_solver": [0, 10, -1], - # BOOLEAN PARAMETERS (Fortran logicals) - "bubbles_euler": ["X", "yes", "1"], - "mpp_lim": ["X", "yes"], - "cyl_coord": ["X", "maybe"], - # BOUNDARY CONDITIONS - "bc_x%beg": [None, 100, -100], - "bc_x%end": [None, 100, -100], - "bc_y%beg": [100, -100], - "bc_y%end": [100, -100], - # DOMAIN PARAMETERS - "x_domain%beg": [None], - "x_domain%end": [None], - # PHYSICS: THERMODYNAMICS - # gamma must be > 1 for physical gases (gamma = Cp/Cv) - # In MFC, fluid_pp(i)%gamma stores 1/(gamma-1), so it must be > 0 - "fluid_pp(1)%gamma": [0, -1, -0.5], - # pi_inf (stiffness) must be >= 0 for stiffened gas EOS - "fluid_pp(1)%pi_inf": [-1, -1e6], - # PHYSICS: PATCH INITIAL CONDITIONS - # Pressure must be positive - "patch_icpp(1)%pres": [0, -1, -1e5], - # Density (alpha_rho) must be non-negative (0 allowed for vacuum) - "patch_icpp(1)%alpha_rho(1)": [-1, -1000], - # Volume fraction must be in [0, 1] - "patch_icpp(1)%alpha(1)": [-0.1, 1.5, 2.0], - # PHYSICS: GEOMETRY - # Patch dimensions must be positive - "patch_icpp(1)%length_x": [0, -1, -10], - "patch_icpp(1)%length_y": [0, -1], - "patch_icpp(1)%length_z": [0, -1], - "patch_icpp(1)%radius": [0, -1], - # PHYSICS: BUBBLES - # Bubble radius must be positive - "patch_icpp(1)%r0": [0, -1], - # Number of bubble bins must be positive - "nb": [0, -1], - # Bubble reference parameters must be positive - "bub_pp%R0ref": [0, -1], - "bub_pp%p0ref": [0, -1], - "bub_pp%rho0ref": [0, -1], - "bub_pp%T0ref": [0, -1], - # Bubble viscosities must be non-negative - "bub_pp%mu_l": [-1, -1e-3], - "bub_pp%mu_g": [-1, -1e-3], - # Surface tension must be non-negative - "bub_pp%ss": [-1, -0.01], - # PHYSICS: ACOUSTICS - # Frequency/wavelength must be positive - "acoustic(1)%frequency": [0, -1], - "acoustic(1)%wavelength": [0, -1], - "acoustic(1)%gauss_sigma_time": [0, -1], - "acoustic(1)%gauss_sigma_dist": [0, -1], - # NUMERICS - # CFL target should be in (0, 1] - "cfl_target": [-0.1, 0, 1.5, 2.0], - # WENO epsilon must be positive (small regularization) - "weno_eps": [0, -1e-6], -} - - -def load_example_case(case_path: Path) -> Dict[str, Any]: - """Load parameters from an example case file.""" - result = subprocess.run(["python3", str(case_path)], capture_output=True, text=True, cwd=case_path.parent, timeout=30, check=False) - if result.returncode != 0: - return None - return json.loads(result.stdout.strip()) - - -def run_mutation(params: Dict[str, Any], param_name: str, mutated_value: Any) -> Tuple[bool, List[str]]: - """Apply mutation and check if validator catches it.""" - mutated_params = params.copy() - - if mutated_value is None: - # Remove the parameter - mutated_params.pop(param_name, None) - else: - mutated_params[param_name] = mutated_value - - validator = CaseValidator(mutated_params) - - try: - validator.validate_pre_process() - except Exception: - pass - - try: - validator.validate_simulation() - except Exception: - pass - - try: - validator.validate_post_process() - except Exception: - pass - - return len(validator.errors) > 0, validator.errors - - -def run_mutations_on_case(case_name: str, params: Dict[str, Any]) -> List[MutationResult]: - """Run all applicable mutations on a case.""" - results = [] - - for param_name, mutations in MUTATIONS.items(): - if param_name not in params: - continue - - original = params[param_name] - - for mutated_value in mutations: - # Skip if mutation is same as original - if mutated_value == original: - continue - - caught, errors = run_mutation(params, param_name, mutated_value) - - results.append( - MutationResult( - case_name=case_name, - param_name=param_name, - original_value=original, - mutated_value=mutated_value, - validator_caught=caught, - errors=errors[:3], # Limit for memory - ) - ) - - return results - - -def run_mutation_tests(max_cases: int = 10) -> Dict[str, Any]: - """Run mutation tests on example cases.""" - examples_dir = Path(__file__).parent.parent.parent.parent / "examples" - case_files = sorted(examples_dir.glob("**/case.py"))[:max_cases] - - all_results = [] - cases_tested = 0 - - for case_file in case_files: - case_name = str(case_file.relative_to(examples_dir).parent) - params = load_example_case(case_file) - - if params is None: - continue - - cases_tested += 1 - results = run_mutations_on_case(case_name, params) - all_results.extend(results) - - # Summarize - total = len(all_results) - caught = sum(1 for r in all_results if r.validator_caught) - missed = sum(1 for r in all_results if not r.validator_caught) - - # Group by parameter - by_param = {} - for r in all_results: - if r.param_name not in by_param: - by_param[r.param_name] = {"caught": 0, "missed": 0} - if r.validator_caught: - by_param[r.param_name]["caught"] += 1 - else: - by_param[r.param_name]["missed"] += 1 - - return { - "cases_tested": cases_tested, - "total_mutations": total, - "caught": caught, - "missed": missed, - "catch_rate": caught / total * 100 if total > 0 else 0, - "by_param": by_param, - "missed_details": [r for r in all_results if not r.validator_caught][:20], - } - - -def print_mutation_report(): - """Print mutation test results.""" - print("Running mutation tests on example cases...") - print("(This tests that the validator catches invalid parameter values)") - print() - - results = run_mutation_tests(max_cases=20) - - print("=" * 70) - print("MUTATION TEST RESULTS") - print("=" * 70) - print(f"\nCases tested: {results['cases_tested']}") - print(f"Total mutations: {results['total_mutations']}") - print(f"Caught by validator: {results['caught']}") - print(f"Missed by validator: {results['missed']}") - print(f"Catch rate: {results['catch_rate']:.1f}%") - - print("\n" + "-" * 70) - print("BY PARAMETER:") - print("-" * 70) - for param, data in sorted(results["by_param"].items(), key=lambda x: -x[1]["missed"]): - total = data["caught"] + data["missed"] - rate = data["caught"] / total * 100 if total > 0 else 0 - status = "OK" if data["missed"] == 0 else "GAPS" - print(f" {param}: {data['caught']}/{total} caught ({rate:.0f}%) [{status}]") - - if results["missed_details"]: - print("\n" + "-" * 70) - print("SAMPLE OF UNCAUGHT MUTATIONS (potential validator gaps):") - print("-" * 70) - for r in results["missed_details"][:10]: - print(f" {r.case_name}") - print(f" {r.param_name}: {r.original_value} -> {r.mutated_value}") - print(" No validation error raised!") - print() - - -if __name__ == "__main__": - print_mutation_report() diff --git a/toolchain/mfc/params_tests/negative_tests.py b/toolchain/mfc/params_tests/negative_tests.py deleted file mode 100644 index fbd40c912..000000000 --- a/toolchain/mfc/params_tests/negative_tests.py +++ /dev/null @@ -1,470 +0,0 @@ -""" -Negative Test Case Generator. - -Generates test cases that intentionally violate validator constraints -to ensure each constraint is properly enforced. -""" - -from dataclasses import dataclass -from typing import Any, Dict, List - -from ..case_validator import CaseValidator - - -@dataclass -class ConstraintTest: - """A test case for a specific constraint.""" - - method: str - line_number: int - message: str - condition: str - test_params: Dict[str, Any] - should_trigger: bool = True - - -# Base valid case - starts from a known-good configuration -BASE_CASE = { - "m": 50, - "n": 0, - "p": 0, - "model_eqns": 2, - "num_fluids": 1, - "num_patches": 1, - "t_step_start": 0, - "t_step_stop": 100, - "t_step_save": 10, - "dt": 1e-6, - "weno_order": 5, - "bc_x%beg": -1, - "bc_x%end": -1, - "x_domain%beg": 0.0, - "x_domain%end": 1.0, - "patch_icpp(1)%geometry": 1, - "patch_icpp(1)%x_centroid": 0.5, - "patch_icpp(1)%length_x": 1.0, - "patch_icpp(1)%vel(1)": 0.0, - "patch_icpp(1)%pres": 1.0, - "patch_icpp(1)%alpha_rho(1)": 1.0, - "patch_icpp(1)%alpha(1)": 1.0, - "fluid_pp(1)%gamma": 0.4, - "fluid_pp(1)%pi_inf": 0.0, -} - - -def generate_constraint_tests() -> List[ConstraintTest]: - """Generate test cases for each constraint in case_validator.py.""" - tests = [] - - # check_simulation_domain constraints - tests.extend( - [ - ConstraintTest( - method="check_simulation_domain", - line_number=56, - message="m must be set", - condition="m is None", - test_params={**BASE_CASE, "m": None}, - ), - ConstraintTest( - method="check_simulation_domain", - line_number=57, - message="m must be positive", - condition="m <= 0", - test_params={**BASE_CASE, "m": 0}, - ), - ConstraintTest( - method="check_simulation_domain", - line_number=57, - message="m must be positive", - condition="m <= 0", - test_params={**BASE_CASE, "m": -5}, - ), - ConstraintTest( - method="check_simulation_domain", - line_number=58, - message="n must be non-negative", - condition="n < 0", - test_params={**BASE_CASE, "n": -1}, - ), - ConstraintTest( - method="check_simulation_domain", - line_number=59, - message="p must be non-negative", - condition="p < 0", - test_params={**BASE_CASE, "p": -1}, - ), - ConstraintTest( - method="check_simulation_domain", - line_number=60, - message="p must be odd for cylindrical coordinates", - condition="cyl_coord and p > 0 and p % 2 == 0", - test_params={**BASE_CASE, "cyl_coord": "T", "n": 10, "p": 2}, - ), - ConstraintTest( - method="check_simulation_domain", - line_number=62, - message="p must be 0 if n = 0", - condition="n == 0 and p > 0", - test_params={**BASE_CASE, "n": 0, "p": 5}, - ), - ] - ) - - # check_model_eqns_and_num_fluids constraints - tests.extend( - [ - ConstraintTest( - method="check_model_eqns_and_num_fluids", - line_number=73, - message="model_eqns must be 1, 2, 3, or 4", - condition="model_eqns not in [1, 2, 3, 4]", - test_params={**BASE_CASE, "model_eqns": 5}, - ), - ConstraintTest( - method="check_model_eqns_and_num_fluids", - line_number=75, - message="num_fluids must be positive", - condition="num_fluids < 1", - test_params={**BASE_CASE, "num_fluids": 0}, - ), - ConstraintTest( - method="check_model_eqns_and_num_fluids", - line_number=85, - message="model_eqns = 1 does not support mpp_lim", - condition="model_eqns == 1 and mpp_lim", - test_params={**BASE_CASE, "model_eqns": 1, "num_fluids": None, "mpp_lim": "T"}, - ), - ConstraintTest( - method="check_model_eqns_and_num_fluids", - line_number=87, - message="num_fluids = 1 does not support mpp_lim", - condition="num_fluids == 1 and mpp_lim", - test_params={**BASE_CASE, "num_fluids": 1, "mpp_lim": "T"}, - ), - ] - ) - - # check_time_stepping constraints - tests.extend( - [ - ConstraintTest( - method="check_time_stepping", - line_number=0, # Will be determined - message="dt must be positive", - condition="dt <= 0", - test_params={**BASE_CASE, "dt": 0}, - ), - ConstraintTest( - method="check_time_stepping", - line_number=0, - message="dt must be positive", - condition="dt <= 0", - test_params={**BASE_CASE, "dt": -1e-6}, - ), - ConstraintTest( - method="check_time_stepping", - line_number=0, - message="t_step_stop must be >= t_step_start", - condition="t_step_stop < t_step_start", - test_params={**BASE_CASE, "t_step_start": 100, "t_step_stop": 50}, - ), - ] - ) - - # check_weno constraints - tests.extend( - [ - ConstraintTest( - method="check_weno_simulation", - line_number=0, - message="weno_order must be 1, 3, 5, or 7", - condition="weno_order not in [1, 3, 5, 7]", - test_params={**BASE_CASE, "weno_order": 4}, - ), - ConstraintTest( - method="check_weno_simulation", - line_number=0, - message="weno_order must be 1, 3, 5, or 7", - condition="weno_order not in [1, 3, 5, 7]", - test_params={**BASE_CASE, "weno_order": 9}, - ), - ] - ) - - # check_boundary_conditions constraints - tests.extend( - [ - ConstraintTest( - method="check_boundary_conditions", - line_number=0, - message="bc_x%beg must be set", - condition="bc_x%beg is None", - test_params={**BASE_CASE, "bc_x%beg": None}, - ), - ConstraintTest( - method="check_boundary_conditions", - line_number=0, - message="bc_x%end must be set", - condition="bc_x%end is None", - test_params={**BASE_CASE, "bc_x%end": None}, - ), - ] - ) - - # check_bubbles constraints - bubble_case = {**BASE_CASE, "bubbles_euler": "T", "bubble_model": 2, "nb": 1} - tests.extend( - [ - ConstraintTest( - method="check_bubbles_euler", - line_number=0, - message="nb must be >= 1", - condition="bubbles_euler and nb < 1", - test_params={**bubble_case, "nb": 0}, - ), - ] - ) - - # check_acoustic_source constraints (the biggest method) - tests.extend( - [ - ConstraintTest( - method="check_acoustic_source", - line_number=0, - message="num_source must be positive when acoustic_source is enabled", - condition="acoustic_source and num_source < 1", - test_params={**BASE_CASE, "acoustic_source": "T", "num_source": 0}, - ), - ] - ) - - tests.extend( - [ - ConstraintTest( - method="check_interface_compression", - line_number=0, - message="int_comp > 0 is not supported with model_eqns = 3", - condition="int_comp != 0 and model_eqns == 3", - test_params={ - **BASE_CASE, - "n": 10, - "bc_y%beg": -3, - "bc_y%end": -3, - "y_domain%beg": 0.0, - "y_domain%end": 1.0, - "num_fluids": 2, - "model_eqns": 3, - "int_comp": 1, - "fluid_pp(2)%gamma": 2.5, - "fluid_pp(2)%pi_inf": 0.0, - "patch_icpp(1)%alpha_rho(2)": 0.0, - "patch_icpp(1)%alpha(2)": 0.0, - }, - ), - ] - ) - - # check_non_newtonian constraints - # A valid-except-for-the-violation non-Newtonian fluid case. - nn_case = { - **BASE_CASE, - "viscous": "T", - "riemann_solver": 2, - "wave_speeds": 1, - "avg_state": 2, - "fluid_pp(1)%Re(1)": 50.0, - "fluid_pp(1)%non_newtonian": "T", - "fluid_pp(1)%K": 2e-2, - "fluid_pp(1)%nn": 0.7, - "fluid_pp(1)%mu_min": 1e-6, - "fluid_pp(1)%mu_max": 10.0, - } - tests.extend( - [ - ConstraintTest( - method="check_non_newtonian", - line_number=0, - message="non_newtonian requires viscous = T", - condition="non_newtonian and not viscous", - test_params={**nn_case, "viscous": "F", "fluid_pp(1)%Re(1)": None}, - ), - ConstraintTest( - method="check_non_newtonian", - line_number=0, - message="tau0 > 0 requires hb_m to be set", - condition="non_newtonian and tau0 > 0 and hb_m is None", - test_params={**nn_case, "fluid_pp(1)%tau0": 0.1}, - ), - ConstraintTest( - method="check_non_newtonian", - line_number=0, - message="mu_max must exceed mu_min", - condition="non_newtonian and mu_max <= mu_min", - test_params={**nn_case, "fluid_pp(1)%mu_min": 10.0, "fluid_pp(1)%mu_max": 1.0}, - ), - ConstraintTest( - method="check_non_newtonian", - line_number=0, - message="mu_bulk is not yet supported", - condition="non_newtonian and mu_bulk is set", - test_params={**nn_case, "fluid_pp(1)%mu_bulk": 1.0}, - ), - ConstraintTest( - method="check_non_newtonian", - line_number=0, - message="Re(2) is not supported for non-Newtonian fluids", - condition="non_newtonian and Re(2) is set", - test_params={**nn_case, "fluid_pp(1)%Re(2)": 50.0}, - ), - ConstraintTest( - method="check_non_newtonian", - line_number=0, - message="mu_max must be positive", - condition="non_newtonian and mu_max <= 0", - test_params={**nn_case, "fluid_pp(1)%mu_min": None, "fluid_pp(1)%mu_max": -1.0}, - ), - ConstraintTest( - method="check_non_newtonian", - line_number=0, - message="non_newtonian requires riemann_solver 1 or 2", - condition="non_newtonian and riemann_solver not in (1, 2)", - test_params={**nn_case, "riemann_solver": 5}, - ), - ConstraintTest( - method="check_non_newtonian", - line_number=0, - message="K is set, but non_newtonian is not enabled", - condition="HB parameter set and non_newtonian not enabled", - test_params={**BASE_CASE, "fluid_pp(1)%K": 2e-2}, - ), - ConstraintTest( - method="check_non_newtonian", - line_number=0, - message="non_newtonian requires model_eqns 2 or 3", - condition="non_newtonian and model_eqns == 1 with num_fluids omitted", - test_params={**nn_case, "model_eqns": 1, "num_fluids": None}, - ), - ] - ) - - return tests - - -def _message_matches(expected: str, actual_errors: List[str]) -> bool: - """Check if expected message matches any actual error (fuzzy).""" - expected_lower = expected.lower() - # Extract key terms from expected message - key_terms = [w for w in expected_lower.split() if len(w) > 3] - - for err in actual_errors: - err_lower = err.lower() - # Check if most key terms appear in the error - matches = sum(1 for term in key_terms if term in err_lower) - if matches >= len(key_terms) * 0.5: # 50% of terms match - return True - return False - - -def run_constraint_tests() -> Dict[str, Any]: - """Run all constraint tests and return results.""" - tests = generate_constraint_tests() - results = { - "total": len(tests), - "passed": 0, - "failed": 0, - "errors_triggered": 0, - "details": [], - } - - for test in tests: - validator = CaseValidator(test.test_params) - - # Run validation for all stages - try: - validator.validate_pre_process() - except Exception: - pass - - try: - validator.validate_simulation() - except Exception: - pass - - # Check if any error was triggered (the key metric) - any_error = len(validator.errors) > 0 - message_matched = _message_matches(test.message, validator.errors) - - if any_error: - results["errors_triggered"] += 1 - - if any_error == test.should_trigger: - results["passed"] += 1 - status = "PASS" - else: - results["failed"] += 1 - status = "FAIL" - - results["details"].append( - { - "method": test.method, - "message": test.message, - "status": status, - "expected_trigger": test.should_trigger, - "any_error": any_error, - "message_matched": message_matched, - "all_errors": validator.errors[:3], # Limit for display - } - ) - - return results - - -def print_test_report(): - """Print test results to console.""" - results = run_constraint_tests() - - print("=" * 70) - print("Constraint Validation Negative Tests") - print("=" * 70) - print(f"\nTotal tests: {results['total']}") - print(f"Errors triggered: {results['errors_triggered']}/{results['total']}") - print(f"Passed: {results['passed']}") - print(f"Failed: {results['failed']}") - - # Group by method - by_method = {} - for detail in results["details"]: - method = detail["method"] - if method not in by_method: - by_method[method] = {"passed": 0, "failed": 0, "tests": []} - if detail["status"] == "PASS": - by_method[method]["passed"] += 1 - else: - by_method[method]["failed"] += 1 - by_method[method]["tests"].append(detail) - - print("\nResults by method:") - for method, data in sorted(by_method.items()): - status = "OK" if data["failed"] == 0 else "ISSUES" - print(f" {method}: {data['passed']}/{data['passed'] + data['failed']} [{status}]") - - if results["failed"] > 0: - print("\nFailed tests (constraint not triggering as expected):") - for detail in results["details"]: - if detail["status"] == "FAIL": - print(f"\n {detail['method']}") - print(f" Expected: {detail['message']}") - print(f" Got errors: {detail['any_error']}") - if detail["all_errors"]: - for err in detail["all_errors"][:2]: - print(f" - {err[:60]}...") - - print("\n" + "=" * 70) - error_rate = results["errors_triggered"] / results["total"] * 100 - print(f"Error trigger rate: {error_rate:.1f}% ({results['errors_triggered']}/{results['total']} tests triggered errors)") - print("=" * 70) - - -if __name__ == "__main__": - print_test_report() diff --git a/toolchain/mfc/params_tests/runner.py b/toolchain/mfc/params_tests/runner.py deleted file mode 100644 index aa89c87bc..000000000 --- a/toolchain/mfc/params_tests/runner.py +++ /dev/null @@ -1,234 +0,0 @@ -""" -Test Safety Net Runner. - -Main entry point for building and verifying the parameter validation test suite. -""" - -import argparse -import json -import sys -from pathlib import Path - -from .coverage import generate_coverage_report, print_coverage_report, save_coverage_report -from .inventory import export_parameter_inventory, print_inventory_summary, save_inventory -from .snapshot import capture_all_examples, compare_snapshots, load_snapshots, print_comparison_report, save_snapshots - - -def get_data_dir() -> Path: - """Get the directory for storing test data.""" - data_dir = Path(__file__).parent / "data" - data_dir.mkdir(exist_ok=True) - return data_dir - - -def build_safety_net(verbose: bool = True): - """ - Build the complete test safety net. - - This captures: - 1. Parameter inventory - 2. Validation snapshots from all examples - 3. Constraint coverage analysis - """ - data_dir = get_data_dir() - - if verbose: - print("=" * 70) - print("Building Parameter Validation Safety Net") - print("=" * 70) - - # 1. Parameter inventory - if verbose: - print("\n[1/3] Exporting parameter inventory...") - inventory_path = data_dir / "param_inventory.json" - save_inventory(inventory_path) - inventory = export_parameter_inventory() - if verbose: - print(f" Total parameters: {inventory['metadata']['total_parameters']}") - print(f" Saved to: {inventory_path}") - - # 2. Validation snapshots - if verbose: - print("\n[2/3] Capturing validation snapshots from examples...") - snapshots = capture_all_examples() - snapshots_path = data_dir / "validation_snapshots.json" - save_snapshots(snapshots, snapshots_path) - load_errors = sum(1 for s in snapshots.values() if s.load_error) - if verbose: - print(f" Total cases: {len(snapshots)}") - print(f" Load errors: {load_errors}") - print(f" Saved to: {snapshots_path}") - - # 3. Constraint coverage - if verbose: - print("\n[3/3] Analyzing constraint coverage...") - coverage_path = data_dir / "constraint_coverage.json" - save_coverage_report(coverage_path) - coverage = generate_coverage_report() - if verbose: - print(f" Total constraints: {coverage['summary']['total_constraints']}") - print(f" Check methods: {coverage['summary']['total_check_methods']}") - print(f" Saved to: {coverage_path}") - - if verbose: - print("\n" + "=" * 70) - print("Safety net built successfully!") - print("=" * 70) - print(f"\nData stored in: {data_dir}") - print("\nFiles created:") - print(f" - param_inventory.json ({inventory['metadata']['total_parameters']} params)") - print(f" - validation_snapshots.json ({len(snapshots)} cases)") - print(f" - constraint_coverage.json ({coverage['summary']['total_constraints']} constraints)") - - return { - "inventory": inventory, - "snapshots": snapshots, - "coverage": coverage, - } - - -def _print_if(verbose: bool, *args, **kwargs): - """Print only if verbose mode is enabled.""" - if verbose: - print(*args, **kwargs) - - -def _print_changes_report(differences: dict, verbose: bool): - """Print report when validation has changed.""" - if not verbose: - return - print("\n" + "=" * 70) - print("VALIDATION CHANGED!") - print("=" * 70) - if differences["changed_validation"]: - print(f" {len(differences['changed_validation'])} cases have different validation results") - if differences["removed_cases"]: - print(f" {len(differences['removed_cases'])} cases were removed") - print("\nIf this is expected, run 'build' to update the safety net.") - - -def verify_safety_net(verbose: bool = True) -> bool: - """ - Verify that current validation matches the captured safety net. - - Returns True if validation is unchanged, False if there are differences. - """ - data_dir = get_data_dir() - snapshots_path = data_dir / "validation_snapshots.json" - - if not snapshots_path.exists(): - _print_if(verbose, "ERROR: Safety net not found. Run 'build' first.") - return False - - _print_if(verbose, "=" * 70) - _print_if(verbose, "Verifying Parameter Validation Against Safety Net") - _print_if(verbose, "=" * 70) - - _print_if(verbose, "\nLoading saved snapshots...") - old_snapshots = load_snapshots(snapshots_path) - _print_if(verbose, f" Loaded {len(old_snapshots.get('snapshots', {}))} cases") - - _print_if(verbose, "\nCapturing current validation results...") - new_snapshots = capture_all_examples() - _print_if(verbose, f" Captured {len(new_snapshots)} cases") - - _print_if(verbose, "\nComparing results...") - differences = compare_snapshots(old_snapshots, new_snapshots) - - if verbose: - print_comparison_report(differences) - - has_changes = bool(differences["changed_validation"] or differences["removed_cases"]) - if has_changes: - _print_changes_report(differences, verbose) - return False - - _print_if(verbose, "\n" + "=" * 70) - _print_if(verbose, "VALIDATION UNCHANGED - All tests pass!") - _print_if(verbose, "=" * 70) - - return True - - -def show_summary(): - """Show summary of captured safety net data.""" - data_dir = get_data_dir() - - print("=" * 70) - print("Parameter Validation Safety Net Summary") - print("=" * 70) - - # Inventory - inventory_path = data_dir / "param_inventory.json" - if inventory_path.exists(): - with open(inventory_path) as f: - inventory = json.load(f) - print("\nParameter Inventory:") - print(f" Total parameters: {inventory['metadata']['total_parameters']}") - print(" By stage:") - print(f" Common: {inventory['metadata']['common_count']}") - print(f" Pre-process: {inventory['metadata']['pre_process_count']}") - print(f" Simulation: {inventory['metadata']['simulation_count']}") - print(f" Post-process: {inventory['metadata']['post_process_count']}") - else: - print("\nParameter Inventory: NOT FOUND") - - # Snapshots - snapshots_path = data_dir / "validation_snapshots.json" - if snapshots_path.exists(): - with open(snapshots_path) as f: - snapshots = json.load(f) - print("\nValidation Snapshots:") - print(f" Total cases: {snapshots['metadata']['total_cases']}") - print(f" Load errors: {snapshots['metadata']['load_errors']}") - print(f" Validation errors: {snapshots['metadata']['validation_errors']}") - else: - print("\nValidation Snapshots: NOT FOUND") - - # Coverage - coverage_path = data_dir / "constraint_coverage.json" - if coverage_path.exists(): - with open(coverage_path) as f: - coverage = json.load(f) - print("\nConstraint Coverage:") - print(f" Total constraints: {coverage['summary']['total_constraints']}") - print(f" Check methods: {coverage['summary']['total_check_methods']}") - print(" Top methods by constraint count:") - for method, count in coverage["summary"]["methods_with_most_constraints"][:5]: - print(f" {method}: {count}") - else: - print("\nConstraint Coverage: NOT FOUND") - - -def main(): - """Main entry point for command-line usage.""" - parser = argparse.ArgumentParser(description="Parameter Validation Test Safety Net") - parser.add_argument("command", choices=["build", "verify", "summary", "inventory", "coverage", "negative", "mutation"], help="Command to run") - parser.add_argument("-q", "--quiet", action="store_true", help="Reduce output verbosity") - - args = parser.parse_args() - verbose = not args.quiet - - if args.command == "build": - build_safety_net(verbose=verbose) - elif args.command == "verify": - success = verify_safety_net(verbose=verbose) - sys.exit(0 if success else 1) - elif args.command == "summary": - show_summary() - elif args.command == "inventory": - print_inventory_summary() - elif args.command == "coverage": - print_coverage_report() - elif args.command == "negative": - from .negative_tests import print_test_report - - print_test_report() - elif args.command == "mutation": - from .mutation_tests import print_mutation_report - - print_mutation_report() - - -if __name__ == "__main__": - main() diff --git a/toolchain/mfc/params_tests/snapshot.py b/toolchain/mfc/params_tests/snapshot.py deleted file mode 100644 index 3a36f3800..000000000 --- a/toolchain/mfc/params_tests/snapshot.py +++ /dev/null @@ -1,263 +0,0 @@ -""" -Validation Snapshot Tool. - -Captures validation results from case files for regression testing. -This allows us to verify that refactoring doesn't change validation behavior. -""" - -import hashlib -import json -import subprocess -from dataclasses import asdict, dataclass -from pathlib import Path -from typing import Any, Dict, List, Optional - -from ..case_validator import CaseValidator - - -@dataclass -class ValidationResult: - """Result of validating a single case file for a single stage.""" - - case_path: str - stage: str - success: bool - errors: List[str] - param_hash: str # Hash of parameters for change detection - error_count: int - - -@dataclass -class CaseSnapshot: - """Complete validation snapshot for a case file.""" - - case_path: str - param_count: int - param_hash: str - stages: Dict[str, ValidationResult] - load_error: Optional[str] = None - - -def hash_params(params: Dict[str, Any]) -> str: - """Create a hash of parameters for change detection.""" - # Sort keys for consistent hashing - sorted_items = sorted(params.items(), key=lambda x: x[0]) - param_str = json.dumps(sorted_items, sort_keys=True, default=str) - return hashlib.md5(param_str.encode()).hexdigest()[:12] - - -def validate_case_for_stage(params: Dict[str, Any], stage: str) -> ValidationResult: - """Run validation for a specific stage and capture results.""" - validator = CaseValidator(params) - - try: - if stage == "pre_process": - validator.validate_pre_process() - elif stage == "simulation": - validator.validate_simulation() - elif stage == "post_process": - validator.validate_post_process() - else: - raise ValueError(f"Unknown stage: {stage}") - - return ValidationResult( - case_path="", # Will be filled in by caller - stage=stage, - success=len(validator.errors) == 0, - errors=validator.errors.copy(), - param_hash=hash_params(params), - error_count=len(validator.errors), - ) - except (ValueError, KeyError, TypeError, AttributeError) as e: - # Catch expected validation errors, not programming bugs like SystemExit - return ValidationResult(case_path="", stage=stage, success=False, errors=[f"Exception during validation: {type(e).__name__}: {str(e)}"], param_hash=hash_params(params), error_count=1) - - -def load_case_params(case_path: Path) -> Dict[str, Any]: - """Load parameters from a case file by running it and capturing JSON output.""" - # MFC case files print JSON to stdout when run - result = subprocess.run(["python3", str(case_path)], capture_output=True, text=True, cwd=case_path.parent, timeout=30, check=False) - - if result.returncode != 0: - raise ValueError(f"Case file failed: {result.stderr[:200]}") - - # Parse the JSON output - output = result.stdout.strip() - if not output: - raise ValueError("Case file produced no output") - - return json.loads(output) - - -def capture_case_snapshot(case_path: Path) -> CaseSnapshot: - """Capture complete validation snapshot for a case file.""" - case_path = Path(case_path) - - try: - params = load_case_params(case_path) - except (ValueError, json.JSONDecodeError, subprocess.TimeoutExpired, subprocess.SubprocessError, OSError, FileNotFoundError) as e: - # Catch expected case loading errors, not programming bugs - return CaseSnapshot(case_path=str(case_path), param_count=0, param_hash="", stages={}, load_error=f"{type(e).__name__}: {str(e)}") - - stages = {} - for stage in ["pre_process", "simulation", "post_process"]: - result = validate_case_for_stage(params, stage) - result.case_path = str(case_path) - stages[stage] = result - - return CaseSnapshot(case_path=str(case_path), param_count=len(params), param_hash=hash_params(params), stages=stages) - - -def capture_all_examples(examples_dir: Path = None) -> Dict[str, CaseSnapshot]: - """Capture validation snapshots for all example cases.""" - if examples_dir is None: - examples_dir = Path(__file__).parent.parent.parent.parent / "examples" - - snapshots = {} - - # Find all case.py files - case_files = sorted(examples_dir.glob("**/case.py")) - - for case_file in case_files: - relative_path = case_file.relative_to(examples_dir) - case_name = str(relative_path.parent) - - print(f" Capturing: {case_name}...", end=" ", flush=True) - try: - snapshot = capture_case_snapshot(case_file) - snapshots[case_name] = snapshot - - if snapshot.load_error: - print(f"LOAD ERROR: {snapshot.load_error[:50]}") - else: - errors = sum(s.error_count for s in snapshot.stages.values()) - if errors > 0: - print(f"ERRORS: {errors}") - else: - print("OK") - except (ValueError, KeyError, TypeError, OSError, json.JSONDecodeError) as e: - # Catch expected errors during capture, not programming bugs - print(f"EXCEPTION: {e}") - snapshots[case_name] = CaseSnapshot(case_path=str(case_file), param_count=0, param_hash="", stages={}, load_error=f"Capture exception: {type(e).__name__}: {str(e)}") - - return snapshots - - -def snapshot_to_dict(snapshot: CaseSnapshot) -> Dict[str, Any]: - """Convert snapshot to JSON-serializable dict.""" - result = asdict(snapshot) - # Convert ValidationResult objects in stages - result["stages"] = {stage: asdict(vr) for stage, vr in snapshot.stages.items()} - return result - - -def save_snapshots(snapshots: Dict[str, CaseSnapshot], output_path: Path = None): - """Save snapshots to JSON file.""" - if output_path is None: - output_path = Path(__file__).parent / "validation_snapshots.json" - - data = { - "metadata": { - "total_cases": len(snapshots), - "load_errors": sum(1 for s in snapshots.values() if s.load_error), - "validation_errors": sum(sum(stage.error_count for stage in s.stages.values()) for s in snapshots.values() if not s.load_error), - }, - "snapshots": {name: snapshot_to_dict(snapshot) for name, snapshot in snapshots.items()}, - } - - with open(output_path, "w", encoding="utf-8") as f: - json.dump(data, f, indent=2) - - return output_path - - -def load_snapshots(input_path: Path) -> Dict[str, Any]: - """Load snapshots from JSON file.""" - with open(input_path, "r", encoding="utf-8") as f: - return json.load(f) - - -def compare_snapshots(old_snapshots: Dict[str, Any], new_snapshots: Dict[str, CaseSnapshot]) -> Dict[str, Any]: - """Compare old and new snapshots, report differences.""" - differences = { - "new_cases": [], - "removed_cases": [], - "changed_validation": [], - "unchanged": [], - } - - old_cases = set(old_snapshots.get("snapshots", {}).keys()) - new_cases = set(new_snapshots.keys()) - - differences["new_cases"] = sorted(new_cases - old_cases) - differences["removed_cases"] = sorted(old_cases - new_cases) - - for case_name in sorted(old_cases & new_cases): - old_snap = old_snapshots["snapshots"][case_name] - new_snap = snapshot_to_dict(new_snapshots[case_name]) - - # Compare validation results - changed = False - changes = [] - - for stage in ["pre_process", "simulation", "post_process"]: - old_stage = old_snap.get("stages", {}).get(stage, {}) - new_stage = new_snap.get("stages", {}).get(stage, {}) - - old_errors = set(old_stage.get("errors", [])) - new_errors = set(new_stage.get("errors", [])) - - if old_errors != new_errors: - changed = True - changes.append( - { - "stage": stage, - "old_error_count": len(old_errors), - "new_error_count": len(new_errors), - "added_errors": sorted(new_errors - old_errors), - "removed_errors": sorted(old_errors - new_errors), - } - ) - - if changed: - differences["changed_validation"].append({"case": case_name, "changes": changes}) - else: - differences["unchanged"].append(case_name) - - return differences - - -def print_comparison_report(differences: Dict[str, Any]): - """Print a human-readable comparison report.""" - print("=" * 60) - print("Validation Comparison Report") - print("=" * 60) - - print(f"\nNew cases: {len(differences['new_cases'])}") - for case in differences["new_cases"][:5]: - print(f" + {case}") - if len(differences["new_cases"]) > 5: - print(f" ... and {len(differences['new_cases']) - 5} more") - - print(f"\nRemoved cases: {len(differences['removed_cases'])}") - for case in differences["removed_cases"][:5]: - print(f" - {case}") - - print(f"\nChanged validation: {len(differences['changed_validation'])}") - for item in differences["changed_validation"][:10]: - print(f"\n {item['case']}:") - for change in item["changes"]: - print(f" [{change['stage']}] {change['old_error_count']} -> {change['new_error_count']} errors") - for err in change["added_errors"][:2]: - print(f" + {err[:60]}...") - for err in change["removed_errors"][:2]: - print(f" - {err[:60]}...") - - print(f"\nUnchanged: {len(differences['unchanged'])}") - - -if __name__ == "__main__": - print("Capturing validation snapshots for all examples...") - all_snapshots = capture_all_examples() - path = save_snapshots(all_snapshots) - print(f"\nSnapshots saved to: {path}") diff --git a/toolchain/mfc/test_case_validator.py b/toolchain/mfc/test_case_validator.py index 876eed656..31478cd7c 100644 --- a/toolchain/mfc/test_case_validator.py +++ b/toolchain/mfc/test_case_validator.py @@ -180,6 +180,49 @@ def test_accepts_valid_configuration(self): self.assertEqual(self.errors_for(REACTIVE_BURN), "") +class TestSyntheticTurbulence(ConstraintTestCase): + """A 2D case with one fully specified forcing zone.""" + + ENABLED = { + **BASE_2D, + "synthetic_turbulence": "T", + "num_turbulent_sources": 1, + "turb_pos(1,1)": 0.5, + "turb_pos(1,2)": 0.5, + "synth_L(1,1)": 1.0, + "synth_L(1,2)": 1.0, + } + + def test_rejects_zero_sources(self): + self.assertRejects({**self.ENABLED, "num_turbulent_sources": 0}, "num_turbulent_sources must be > 0") + + def test_rejects_unset_sources(self): + params = {k: v for k, v in self.ENABLED.items() if k != "num_turbulent_sources"} + self.assertRejects(params, "num_turbulent_sources must be > 0") + + def test_rejects_missing_position(self): + params = {k: v for k, v in self.ENABLED.items() if k != "turb_pos(1,2)"} + self.assertRejects(params, "turb_pos(1,2) must be specified") + + def test_rejects_missing_extent(self): + params = {k: v for k, v in self.ENABLED.items() if k != "synth_L(1,2)"} + self.assertRejects(params, "synth_L(1,2) must be positive") + + def test_rejects_nonpositive_extent(self): + self.assertRejects({**self.ENABLED, "synth_L(1,2)": 0.0}, "synth_L(1,2) must be positive") + + def test_accepts_fully_specified_zone(self): + self.assertEqual(self.errors_for(self.ENABLED), "") + + def test_third_dimension_not_required_in_2d(self): + """The Fortran loops d = 1, num_dims, so a 2D case needs no z components.""" + self.assertAccepts(self.ENABLED, "turb_pos(1,3)") + self.assertAccepts(self.ENABLED, "synth_L(1,3)") + + def test_not_checked_when_disabled(self): + self.assertAccepts(BASE_2D, "num_turbulent_sources") + + class TestTimeStepPositivity(ConstraintTestCase): MSG = "dt must be positive" From 097ddbc074cc5af1143b788610bd2a21b4965b4b Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 9 Aug 2026 15:46:11 -0400 Subject: [PATCH 3/8] Stop rewriting .vscode/settings.json on every mfc.sh invocation Every ./mfc.sh run called ensure_vscode_settings(), which edited .vscode/settings.json in the user's checkout whether or not they use VS Code. It did so by string surgery -- rfind("}"), then splice in a block and guess whether a comma is needed -- on a file that is hand-maintained, committed, and legally contains comments, so a malformed result was a plausible outcome. Remove toolchain/mfc/ide.py entirely. Its two functions, ensure_vscode_settings and update_vscode_settings, were near-duplicates of each other, called from main.py (every invocation) and from generate --json-schema respectively. Also drop generate_vscode_settings() from json_schema_gen.py: a third copy of the same settings blob, called by nothing, and disagreeing with ide.py on both the fileMatch patterns and the schema URL. .vscode/settings.json keeps all 32 of its settings, including the json.schemas/yaml.schemas association -- it is a useful set of defaults for anyone working on MFC. The block is now plain committed config rather than a region the toolchain rewrites, so the "auto-generated, do not edit" markers are replaced with a comment pointing at ./mfc.sh generate, which still writes toolchain/mfc-case-schema.json. --- .vscode/settings.json | 4 +- toolchain/main.py | 5 - toolchain/mfc/generate.py | 4 - toolchain/mfc/ide.py | 122 ------------------ .../mfc/params/generators/json_schema_gen.py | 5 - 5 files changed, 2 insertions(+), 138 deletions(-) delete mode 100644 toolchain/mfc/ide.py diff --git a/.vscode/settings.json b/.vscode/settings.json index 0dd1f28f9..ab634e612 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -74,7 +74,8 @@ "problems.showOn": "toggle", "problems.defaultViewMode": "list", - // MFC-SCHEMA-CONFIG-BEGIN (auto-generated, do not edit) + // Case-file schema association. Run ./mfc.sh generate to (re)build + // toolchain/mfc-case-schema.json, which these entries point at. "json.schemas": [ { "fileMatch": ["**/case.json", "**/input.json", "**/mfc-case.json", "**/mfc.json"], @@ -84,5 +85,4 @@ "yaml.schemas": { "./toolchain/mfc-case-schema.json": ["**/case.yaml", "**/case.yml", "**/input.yaml", "**/input.yml", "**/mfc-case.yaml", "**/mfc.yaml"] } - // MFC-SCHEMA-CONFIG-END } diff --git a/toolchain/main.py b/toolchain/main.py index f7b1fa9e7..94ab11036 100644 --- a/toolchain/main.py +++ b/toolchain/main.py @@ -227,11 +227,6 @@ def __run(): lock.switch(state.MFCConfig.from_dict(state.gARG)) - # Ensure IDE configuration is up to date (lightweight check) - from mfc.ide import ensure_vscode_settings - - ensure_vscode_settings() - # Auto-regenerate completion scripts if source files changed __ensure_generated_files() diff --git a/toolchain/mfc/generate.py b/toolchain/mfc/generate.py index a6a6273c3..9b602b0bd 100644 --- a/toolchain/mfc/generate.py +++ b/toolchain/mfc/generate.py @@ -91,7 +91,6 @@ def generate(): def _generate_json_schema(): """Generate JSON Schema and parameter documentation (standalone mode).""" - from .ide import update_vscode_settings from .params.generators.docs_gen import generate_parameter_docs from .params.generators.json_schema_gen import generate_json_schema, get_schema_stats @@ -105,9 +104,6 @@ def _generate_json_schema(): docs_path = Path(MFC_ROOT_DIR) / "docs" / "documentation" / "parameters.md" docs_path.write_text(generate_parameter_docs()) - # Update VS Code settings - update_vscode_settings() - stats = get_schema_stats() cons.print(f"[green]Generated[/green] {schema_path}") diff --git a/toolchain/mfc/ide.py b/toolchain/mfc/ide.py deleted file mode 100644 index 59279f103..000000000 --- a/toolchain/mfc/ide.py +++ /dev/null @@ -1,122 +0,0 @@ -""" -IDE Configuration Module. - -Automatically configures IDE settings (VS Code, etc.) for MFC development. -""" - -import re -from pathlib import Path - -from .common import MFC_ROOT_DIR - -# Marker comments for the auto-generated section -_VSCODE_MARKER_BEGIN = "// MFC-SCHEMA-CONFIG-BEGIN (auto-generated, do not edit)" -_VSCODE_MARKER_END = "// MFC-SCHEMA-CONFIG-END" - -# The MFC schema configuration to insert -# Matches common case file names - users get auto-completion for JSON/YAML case files -_VSCODE_MFC_CONFIG = """\ - "json.schemas": [ - { - "fileMatch": ["**/case.json", "**/input.json", "**/mfc-case.json", "**/mfc.json"], - "url": "./toolchain/mfc-case-schema.json" - } - ], - "yaml.schemas": { - "./toolchain/mfc-case-schema.json": ["**/case.yaml", "**/case.yml", "**/input.yaml", "**/input.yml", "**/mfc-case.yaml", "**/mfc.yaml"] - }""" - - -def ensure_vscode_settings() -> bool: - """ - Ensure VS Code settings include MFC schema configuration. - - This is called on every mfc.sh invocation but is very lightweight: - - Only reads/writes if the marker section is missing - - Does not regenerate the schema (that's done via generate --json-schema) - - Returns: - True if settings were updated, False if already configured - """ - vscode_dir = Path(MFC_ROOT_DIR) / ".vscode" - settings_path = vscode_dir / "settings.json" - - # Check if schema file exists (it should be committed to repo) - schema_path = Path(MFC_ROOT_DIR) / "toolchain" / "mfc-case-schema.json" - if not schema_path.exists(): - # Schema not generated yet - skip configuration - return False - - # Build the marked config block - marked_config = f"{_VSCODE_MARKER_BEGIN}\n{_VSCODE_MFC_CONFIG}\n {_VSCODE_MARKER_END}" - - if settings_path.exists(): - content = settings_path.read_text() - - # Check if our markers already exist - if so, nothing to do - if _VSCODE_MARKER_BEGIN in content: - return False - - # Insert before the final closing brace - last_brace = content.rfind("}") - if last_brace != -1: - # Check if we need a comma - before_brace = content[:last_brace].rstrip() - needs_comma = before_brace and not before_brace.endswith("{") and not before_brace.endswith(",") - comma = "," if needs_comma else "" - new_content = content[:last_brace].rstrip() + comma + "\n\n " + marked_config + "\n" + content[last_brace:] - else: - # Malformed JSON, just append - new_content = content + "\n" + marked_config - else: - # Ensure .vscode directory exists - vscode_dir.mkdir(exist_ok=True) - # Create new settings file with just our config - new_content = f"{{\n {marked_config}\n}}\n" - - settings_path.write_text(new_content) - return True - - -def update_vscode_settings() -> None: - """ - Force update VS Code settings with MFC schema configuration. - - Unlike ensure_vscode_settings(), this always updates the marked section, - even if it already exists. Used by `generate --json-schema`. - """ - from .printer import cons - - vscode_dir = Path(MFC_ROOT_DIR) / ".vscode" - settings_path = vscode_dir / "settings.json" - - # Ensure .vscode directory exists - vscode_dir.mkdir(exist_ok=True) - - # Build the marked config block - marked_config = f"{_VSCODE_MARKER_BEGIN}\n{_VSCODE_MFC_CONFIG}\n {_VSCODE_MARKER_END}" - - if settings_path.exists(): - content = settings_path.read_text() - - # Check if our markers already exist - marker_pattern = re.compile(rf"{re.escape(_VSCODE_MARKER_BEGIN)}.*?{re.escape(_VSCODE_MARKER_END)}", re.DOTALL) - - if marker_pattern.search(content): - # Replace existing marked section - new_content = marker_pattern.sub(marked_config, content) - else: - # Insert before the final closing brace - last_brace = content.rfind("}") - if last_brace != -1: - before_brace = content[:last_brace].rstrip() - needs_comma = before_brace and not before_brace.endswith("{") and not before_brace.endswith(",") - comma = "," if needs_comma else "" - new_content = content[:last_brace].rstrip() + comma + "\n\n " + marked_config + "\n" + content[last_brace:] - else: - new_content = content + "\n" + marked_config - else: - new_content = f"{{\n {marked_config}\n}}\n" - - settings_path.write_text(new_content) - cons.print(f"[green]Updated[/green] {settings_path}") diff --git a/toolchain/mfc/params/generators/json_schema_gen.py b/toolchain/mfc/params/generators/json_schema_gen.py index 5533f512f..1e6c31210 100644 --- a/toolchain/mfc/params/generators/json_schema_gen.py +++ b/toolchain/mfc/params/generators/json_schema_gen.py @@ -78,11 +78,6 @@ def generate_json_schema(include_descriptions: bool = True) -> Dict[str, Any]: return schema -def generate_vscode_settings() -> Dict[str, Any]: - """Generate VS Code settings snippet for JSON Schema association.""" - return {"json.schemas": [{"fileMatch": ["case.py", "**/case.py"], "url": "./mfc-case-schema.json"}], "yaml.schemas": {"./mfc-case-schema.json": ["case.yaml", "**/case.yaml"]}} - - def write_json_schema(output_path: str, include_descriptions: bool = True) -> None: """ Write JSON Schema to file. From 948fce01c39b7f35ed3407c9990acbde0101f328 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 9 Aug 2026 15:54:01 -0400 Subject: [PATCH 4/8] Remove interactive mode, contextual Tips, and the markdown help scraper user_guide.py: 710 -> 315 lines. interactive_mode (~145 lines) ./mfc.sh interactive presented a numbered menu whose seven handlers each shelled back out to ./mfc.sh new/validate/build/run/test/clean. The command is removed from the CLI schema, main.py, and the README. Tips (~78 lines) A class of five contextual hint methods. Only after_build_failure was ever called (twice, from build.py); after_case_error, after_test_failure, after_run_failure, and suggest_validate had no callers at all. Markdown help scraper (~173 lines) MARKDOWN_HELP_FILES, _extract_markdown_section, _load_markdown_help, and _generate_markdown_help re-read docs/documentation/*.md at runtime, stripped Doxygen syntax with regexes, and re-rendered four topics (gpu, batch, debugging, performance) in the terminal -- a second, lossy presentation of pages that already exist on the docs site. With those four topics gone, HELP_TOPICS held a single dynamic entry, so the topic-dispatch layer (print_topic_help, print_help_topics, and the `topic` positional on the help command) collapses into print_clusters_help. ./mfc.sh help now prints the cluster table directly, through the same Panel it always rendered in. Kept: the cluster table itself, which derives from toolchain/modules and so cannot go stale, and print_help / print_command_help, which back ./mfc.sh --help and ./mfc.sh --help. --- README.md | 2 +- toolchain/main.py | 4 - toolchain/mfc/args.py | 9 +- toolchain/mfc/build.py | 3 - toolchain/mfc/cli/commands.py | 22 +- toolchain/mfc/cli/docs_gen.py | 2 +- toolchain/mfc/user_guide.py | 413 +--------------------------------- 7 files changed, 11 insertions(+), 444 deletions(-) diff --git a/README.md b/README.md index 3de12374e..d57972f20 100644 --- a/README.md +++ b/README.md @@ -109,7 +109,7 @@ And a high-amplitude acoustic wave reflecting and emerging through a circular or | `./mfc.sh validate case.py` | Check a case file for errors before running | | `./mfc.sh new my_case` | Create a new case from a template | | `./mfc.sh clean` | Remove build artifacts | -| `./mfc.sh interactive` | Launch interactive menu-driven interface | +| `./mfc.sh help` | List the HPC clusters MFC ships module sets for | Run `./mfc.sh --help` for detailed options, or see the [full documentation](https://mflowcode.github.io/documentation/index.html). Tab completion for bash and zsh is auto-installed after you have run `./mfc.sh generate` (or any non-`new` command) at least once. Play with the examples in `examples/` ([showcased here](https://mflowcode.github.io/documentation/examples.html)). diff --git a/toolchain/main.py b/toolchain/main.py index 94ab11036..a2a0783fc 100644 --- a/toolchain/main.py +++ b/toolchain/main.py @@ -178,10 +178,6 @@ def __run(): from mfc import init init.init() - elif cmd == "interactive": - from mfc.user_guide import interactive_mode - - interactive_mode() elif cmd == "completion": from mfc import completion diff --git a/toolchain/mfc/args.py b/toolchain/mfc/args.py index 5c68f35ef..613d0d56d 100644 --- a/toolchain/mfc/args.py +++ b/toolchain/mfc/args.py @@ -15,10 +15,9 @@ from .state import MFCConfig from .user_guide import ( is_first_time_user, + print_clusters_help, print_command_help, print_help, - print_help_topics, - print_topic_help, print_welcome, ) @@ -98,11 +97,7 @@ def custom_error(message): # Handle 'help' command if args["command"] == "help": - topic = args.get("topic") - if topic: - print_topic_help(topic) - else: - print_help_topics() + print_clusters_help() sys.exit(0) # Resolve command aliases diff --git a/toolchain/mfc/build.py b/toolchain/mfc/build.py index fa0bcc982..eff5decaa 100644 --- a/toolchain/mfc/build.py +++ b/toolchain/mfc/build.py @@ -18,7 +18,6 @@ from .printer import cons from .run import input from .state import ARG, CFG, gpuConfigOptions -from .user_guide import Tips # Regex to parse build progress # Ninja format: [42/156] Building Fortran object ... @@ -506,7 +505,6 @@ def configure(self, case: Case): cons.print(f" [bold red]✗[/bold red] Configuration failed for [magenta]{self.name}[/magenta]") if verbosity < 2: _show_build_error(result, "Configuration") - Tips.after_build_failure() raise MFCException(f"Failed to configure the [bold magenta]{self.name}[/bold magenta] target.") cons.print(f" [bold green]✓[/bold green] Configured [magenta]{self.name}[/magenta]") @@ -553,7 +551,6 @@ def build(self, case: input.MFCInputFile): cons.print(f" [bold red]✗[/bold red] Build failed for [magenta]{self.name}[/magenta]") if verbosity < 2: _show_build_error(result, "Build") - Tips.after_build_failure() raise MFCException(f"Failed to build the [bold magenta]{self.name}[/bold magenta] target.") cons.print(f" [bold green]✓[/bold green] Built [magenta]{self.name}[/magenta]") diff --git a/toolchain/mfc/cli/commands.py b/toolchain/mfc/cli/commands.py index ca60b53ee..f3752aa73 100644 --- a/toolchain/mfc/cli/commands.py +++ b/toolchain/mfc/cli/commands.py @@ -721,20 +721,7 @@ HELP_COMMAND = Command( name="help", - help="Show help on a topic.", - positionals=[ - Positional( - name="topic", - help="Help topic: gpu, clusters, batch, debugging, performance", - nargs="?", - default=None, - choices=["gpu", "clusters", "batch", "debugging", "performance"], - completion=Completion( - type=CompletionType.CHOICES, - choices=["gpu", "clusters", "batch", "debugging", "performance"], - ), - ), - ], + help="List the HPC clusters MFC ships module sets for.", ) # Simple commands (shell scripts, minimal arguments) @@ -815,12 +802,6 @@ ], ) -INTERACTIVE_COMMAND = Command( - name="interactive", - help="Launch interactive menu-driven interface.", - description="Launch an interactive menu for MFC operations.", -) - GENERATE_COMMAND = Command( name="generate", help="Regenerate completion scripts from CLI schema.", @@ -1494,7 +1475,6 @@ FORMAT_COMMAND, SPELLING_COMMAND, PRECHECK_COMMAND, - INTERACTIVE_COMMAND, BENCH_COMMAND, BENCH_DIFF_COMMAND, COUNT_COMMAND, diff --git a/toolchain/mfc/cli/docs_gen.py b/toolchain/mfc/cli/docs_gen.py index 8d7347a0a..a6975f1ba 100644 --- a/toolchain/mfc/cli/docs_gen.py +++ b/toolchain/mfc/cli/docs_gen.py @@ -244,7 +244,7 @@ def generate_cli_reference(schema: CLISchema) -> str: utility_commands = ["new", "viz", "params", "packer", "completion", "generate", "help"] dev_commands = ["lint", "format", "spelling", "precheck", "count", "count_diff"] ci_commands = ["bench", "bench_diff"] - other_commands = ["load", "interactive"] + other_commands = ["load"] # Core workflow commands first (no header, directly under Commands) for cmd in schema.commands: diff --git a/toolchain/mfc/user_guide.py b/toolchain/mfc/user_guide.py index 6dd608bc8..b5103fa06 100644 --- a/toolchain/mfc/user_guide.py +++ b/toolchain/mfc/user_guide.py @@ -1,23 +1,17 @@ """ -User guide, help, tips, and onboarding for MFC toolchain. +Help output and onboarding for the MFC toolchain. This module provides: - Enhanced help output with Rich formatting -- Contextual tips after errors/failures -- Interactive mode with menu +- The cluster table behind ``./mfc.sh help``, derived from toolchain/modules - Onboarding for new users -- Topic-based help system """ import os import re -import subprocess from rich import box -from rich.markdown import Markdown from rich.panel import Panel -from rich.prompt import Prompt -from rich.table import Table # Import command definitions from CLI schema (SINGLE SOURCE OF TRUTH) from .cli.commands import COMMANDS @@ -180,180 +174,10 @@ def _generate_clusters_content(): • CMake 3.18+, Python 3.11+""" -# MARKDOWN-BASED HELP (Single source of truth from docs/) - -# Mapping of help topics to their source markdown files and optional section -# Format: {"topic": ("file_path", "section_heading" or None for full file)} -MARKDOWN_HELP_FILES = { - "debugging": ("docs/documentation/troubleshooting.md", None), # Full file - "gpu": ("docs/documentation/running.md", "Running on GPUs"), # Section only - "batch": ("docs/documentation/running.md", "Batch Execution"), # Section only - "performance": ("docs/documentation/expectedPerformance.md", "Achieving Maximum Performance"), -} - - -def _extract_markdown_section(content: str, section_heading: str) -> str: - """Extract a specific section from markdown content. - - Extracts from the given heading until the next heading of same or higher level, - or until a horizontal rule (---). - """ - # Find the section heading (## or ###) - # Note: In f-strings, literal braces must be doubled: {{1,3}} -> {1,3} - pattern = rf"^(#{{1,3}})\s+{re.escape(section_heading)}\s*$" - match = re.search(pattern, content, re.MULTILINE) - if not match: - return None - - start_pos = match.end() - - # Find the end: horizontal rule (---) which separates major sections - # Note: We use --- instead of heading detection because shell comments - # inside code blocks (# comment) look like markdown headings to regex - end_pattern = r"^---" - end_match = re.search(end_pattern, content[start_pos:], re.MULTILINE) - - if end_match: - section = content[start_pos : start_pos + end_match.start()] - else: - section = content[start_pos:] - - return section.strip() - - -def _load_markdown_help(topic: str) -> str: - """Load help content from a markdown file. - - Can load full file or extract a specific section. - Strips Doxygen-specific syntax and returns clean markdown. - """ - if topic not in MARKDOWN_HELP_FILES: - return None - - file_path, section = MARKDOWN_HELP_FILES[topic] - filepath = os.path.join(MFC_ROOT_DIR, file_path) - - try: - with open(filepath, "r", encoding="utf-8") as f: - content = f.read() - except FileNotFoundError: - return None - - # Extract section if specified - if section: - content = _extract_markdown_section(content, section) - if content is None: - return None - - # Strip Doxygen-specific syntax - # Remove @page directives - content = re.sub(r"^@page\s+\S+\s+.*$", "", content, flags=re.MULTILINE) - # Remove @ref, @see directives (but keep the text after them readable) - content = re.sub(r'@(ref|see)\s+"([^"]+)"', r"\2", content) # @ref "Text" -> Text - content = re.sub(r"@(ref|see)\s+(\S+)", "", content) # @ref name -> (remove) - # Clean up any resulting empty lines at the start - content = content.lstrip("\n") - - return content - - -def _generate_markdown_help(topic: str): - """Generate a function that loads markdown help for a topic.""" - - def loader(): - return _load_markdown_help(topic) - - return loader - - -# HELP TOPICS - -HELP_TOPICS = { - "gpu": { - "title": "Running on GPUs", - # Content loaded from docs/documentation/running.md "Running on GPUs" section - "content": _generate_markdown_help("gpu"), - "markdown": True, - }, - "clusters": { - "title": "Cluster Configuration", - # Content is generated dynamically from toolchain/modules file - "content": _generate_clusters_content, - }, - "batch": { - "title": "Batch Job Submission", - # Content loaded from docs/documentation/running.md "Batch Execution" section - "content": _generate_markdown_help("batch"), - "markdown": True, - }, - "debugging": { - "title": "Debugging & Troubleshooting", - # Content loaded from docs/documentation/troubleshooting.md - "content": _generate_markdown_help("debugging"), - "markdown": True, - }, - "performance": { - "title": "Performance Optimization", - # Content loaded from docs/documentation/expectedPerformance.md "Achieving Maximum Performance" section - "content": _generate_markdown_help("performance"), - "markdown": True, - }, -} - - -def print_topic_help(topic: str): - """Print help for a specific topic.""" - if topic not in HELP_TOPICS: - cons.print(f"[red]Unknown topic: {topic}[/red]") - cons.print() - cons.print("[bold]Available topics:[/bold]") - for t, info in HELP_TOPICS.items(): - cons.print(f" [green]{t:12}[/green] {info['title']}") - cons.print() - cons.print("[dim]Usage: ./mfc.sh help [/dim]") - return - - topic_info = HELP_TOPICS[topic] - # Support callable content for dynamic generation - content = topic_info["content"] - if callable(content): - content = content() - - if content is None: - cons.print(f"[red]Could not load help for topic: {topic}[/red]") - return - - cons.print() - - # Check if content should be rendered as markdown - if topic_info.get("markdown", False): - # Render markdown content directly (no panel - markdown has its own formatting) - cons.print(f"[bold cyan]{topic_info['title']}[/bold cyan]") - cons.print() - cons.raw.print(Markdown(content)) - else: - # Render as Rich markup in a panel - cons.raw.print(Panel(content, title=f"[bold]{topic_info['title']}[/bold]", box=box.ROUNDED, padding=(1, 2))) - cons.print() - - -def print_help_topics(): - """Print list of available help topics.""" - cons.print() - cons.raw.print(Panel("[bold cyan]MFC Help System[/bold cyan]", box=box.ROUNDED, padding=(0, 2))) - cons.print() - - table = Table(box=box.SIMPLE, show_header=False, padding=(0, 2)) - table.add_column("Topic", style="green") - table.add_column("Description") - - for topic, info in HELP_TOPICS.items(): - table.add_row(topic, info["title"]) - - cons.raw.print(table) +def print_clusters_help(): + """Print the cluster configuration table behind ``./mfc.sh help``.""" cons.print() - cons.print("[dim]Usage: [cyan]./mfc.sh help [/cyan][/dim]") - cons.print("[dim]Example: [cyan]./mfc.sh help gpu[/cyan][/dim]") + cons.raw.print(Panel(_generate_clusters_content(), title="[bold]Cluster Configuration[/bold]", box=box.ROUNDED, padding=(1, 2))) cons.print() @@ -447,84 +271,6 @@ def print_command_help(command: str, show_argparse: bool = True): return True -# CONTEXTUAL TIPS - - -class Tips: - """Contextual tips shown after various events.""" - - @staticmethod - def after_build_failure(): - """Show tips after a build failure.""" - cons.print() - cons.raw.print( - Panel( - "[bold yellow]Troubleshooting Tips[/bold yellow]\n\n" - " [cyan]1.[/cyan] Rebuild with [green]--debug[/green] for debug compiler flags and verbose output\n" - " [cyan]2.[/cyan] Check [green]docs/documentation/troubleshooting.md[/green]\n" - " [cyan]3.[/cyan] Ensure required modules are loaded: [green]source ./mfc.sh load -c -m [/green]\n" - " [cyan]4.[/cyan] Try [green]./mfc.sh clean[/green] and rebuild", - box=box.ROUNDED, - border_style="yellow", - padding=(0, 2), - ) - ) - - @staticmethod - def after_case_error(case_path: str = None): - """Show tips after a case file error.""" - msg = "[bold yellow]Tip[/bold yellow]\n\n" - if case_path: - msg += f" Run [green]./mfc.sh validate {case_path}[/green] to check your case file for errors" - else: - msg += " Run [green]./mfc.sh validate [/green] to check your case file for errors" - - cons.print() - cons.raw.print(Panel(msg, box=box.ROUNDED, border_style="yellow", padding=(0, 2))) - - @staticmethod - def after_test_failure(failed_uuids: list = None): - """Show tips after test failures.""" - lines = [ - "[bold yellow]Next Steps[/bold yellow]\n", - " [cyan]1.[/cyan] Check individual test output in [green]tests//[/green]", - " [cyan]2.[/cyan] Run specific test: [green]./mfc.sh test --only [/green]", - " [cyan]3.[/cyan] Update golden files (if changes are intentional): [green]./mfc.sh test --generate[/green]", - ] - - if failed_uuids and len(failed_uuids) <= 3: - lines.append("") - lines.append(" [bold]Failed tests:[/bold]") - for uuid in failed_uuids: - lines.append(f" [red]•[/red] {uuid}") - - cons.print() - cons.raw.print(Panel("\n".join(lines), box=box.ROUNDED, border_style="yellow", padding=(0, 2))) - - @staticmethod - def after_run_failure(): - """Show tips after a run failure.""" - cons.print() - cons.raw.print( - Panel( - "[bold yellow]Troubleshooting Tips[/bold yellow]\n\n" - " [cyan]1.[/cyan] Validate your case: [green]./mfc.sh validate case.py[/green]\n" - " [cyan]2.[/cyan] Check the output in [green]/[/green]\n" - " [cyan]3.[/cyan] Rebuild with [green]--debug[/green] for debug compiler flags\n" - " [cyan]4.[/cyan] Check MFC documentation: [green]docs/[/green]", - box=box.ROUNDED, - border_style="yellow", - padding=(0, 2), - ) - ) - - @staticmethod - def suggest_validate(): - """Generic suggestion to use validate.""" - cons.print() - cons.print("[dim]Tip: Run [cyan]./mfc.sh validate case.py[/cyan] to check for errors before running[/dim]") - - # ONBOARDING FOR NEW USERS @@ -553,7 +299,7 @@ def print_welcome(): "[bold yellow]Optional:[/bold yellow] Enable tab completion for your shell:\n" " [cyan]./mfc.sh completion install[/cyan]\n\n" "[dim]Run [cyan]./mfc.sh --help[/cyan] for all available commands[/dim]\n" - "[dim]Run [cyan]./mfc.sh interactive[/cyan] for a guided menu[/dim]", + "[dim]Run [cyan]./mfc.sh help[/cyan] for the list of supported clusters[/dim]", title="[bold]Getting Started[/bold]", box=box.DOUBLE, border_style="cyan", @@ -561,150 +307,3 @@ def print_welcome(): ) ) cons.print() - - -# INTERACTIVE MODE - - -def interactive_mode(): - """Run interactive menu-driven interface.""" - - while True: - cons.print() - cons.raw.print(Panel("[bold cyan]MFC Interactive Mode[/bold cyan]", box=box.ROUNDED, padding=(0, 2))) - cons.print() - - # Menu options - options = [ - ("1", "Create a new case", "new"), - ("2", "Validate a case file", "validate"), - ("3", "Build MFC", "build"), - ("4", "Run a simulation", "run"), - ("5", "Run tests", "test"), - ("6", "Clean build files", "clean"), - ("7", "Show help", "help"), - ("q", "Quit", None), - ] - - for key, label, _ in options: - if key == "q": - cons.print(f" [red]{key}[/red]) {label}") - else: - cons.print(f" [green]{key}[/green]) {label}") - - cons.print() - choice = Prompt.ask("[bold]Select an option[/bold]", choices=[o[0] for o in options], default="q") - - if choice == "q": - cons.print("[dim]Goodbye![/dim]") - break - - if choice == "7": - print_help() - continue - - # Get the command for the selected option - cmd = next((o[2] for o in options if o[0] == choice), None) - if cmd is None: - continue - - cons.print() - - # Dispatch to handler - handlers = { - "new": _interactive_new, - "validate": _interactive_validate, - "build": _interactive_build, - "run": _interactive_run, - "test": _interactive_test, - "clean": _interactive_clean, - } - if cmd in handlers: - handlers[cmd]() - - -def _run_mfc_command(args: list): - """Run an MFC command safely using subprocess.""" - cmd_str = " ".join(args) - cons.print() - cons.print(f"[dim]Running: {cmd_str}[/dim]") - cons.print() - try: - subprocess.run(args, check=False) - except FileNotFoundError: - cons.print(f"[red]Command not found: {args[0]}[/red]") - - -def _interactive_new(): - """Interactive case creation.""" - cons.print("[bold]Create a New Case[/bold]") - cons.print() - - # Show templates - cons.print("Available templates: [cyan]1D_minimal[/cyan], [cyan]2D_minimal[/cyan], [cyan]3D_minimal[/cyan]") - cons.print("[dim]Or use 'example:' to copy from examples[/dim]") - cons.print() - - name = Prompt.ask("Case name", default="my_case") - template = Prompt.ask("Template", default="1D_minimal") - - _run_mfc_command(["./mfc.sh", "new", name, "-t", template]) - - -def _interactive_validate(): - """Interactive case validation.""" - cons.print("[bold]Validate a Case File[/bold]") - cons.print() - - path = Prompt.ask("Path to case.py") - - _run_mfc_command(["./mfc.sh", "validate", path]) - - -def _interactive_build(): - """Interactive build.""" - cons.print("[bold]Build MFC[/bold]") - cons.print() - - jobs = Prompt.ask("Number of parallel jobs", default="4") - gpu = Prompt.ask("Enable GPU support?", choices=["y", "n"], default="n") - - args = ["./mfc.sh", "build", "-j", jobs] - if gpu == "y": - args.append("--gpu") - - _run_mfc_command(args) - - -def _interactive_run(): - """Interactive run.""" - cons.print("[bold]Run a Simulation[/bold]") - cons.print() - - path = Prompt.ask("Path to case.py") - ranks = Prompt.ask("Number of MPI ranks", default="1") - - _run_mfc_command(["./mfc.sh", "run", path, "-n", ranks]) - - -def _interactive_test(): - """Interactive test.""" - cons.print("[bold]Run Tests[/bold]") - cons.print() - - jobs = Prompt.ask("Number of parallel jobs", default="4") - - _run_mfc_command(["./mfc.sh", "test", "-j", jobs]) - - -def _interactive_clean(): - """Interactive clean.""" - cons.print("[bold]Clean Build Files[/bold]") - cons.print() - - confirm = Prompt.ask("Are you sure you want to clean all build files?", choices=["y", "n"], default="n") - - if confirm == "y": - _run_mfc_command(["./mfc.sh", "clean"]) - else: - cons.print("[dim]Cancelled[/dim]") From 6f0596cf4171129ae8d4edc40c3d9d9000d52c8d Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 9 Aug 2026 16:00:56 -0400 Subject: [PATCH 5/8] Drop the Python cluster table; ./mfc.sh load is the one cluster list The cluster listing existed twice: a hand-written coloured menu in toolchain/bootstrap/modules.sh, shown by ./mfc.sh load, and 163 lines of Python in user_guide.py that re-derived the same list from toolchain/modules for ./mfc.sh help. Keep the shell menu and delete the Python. That removes CLUSTER_ORGS, SLUG_ORG_OVERRIDE, SLUG_NAME_OVERRIDE, ORG_ORDER, ORG_COLORS (which mapped all eight organisations to "yellow"), _parse_modules_file, _get_cluster_short_name, _generate_clusters_content, and print_clusters_help. The cluster table was the only content ./mfc.sh help had left, so the help command goes with it. user_guide.py: 315 -> 144 lines (710 at the start of this branch). Fix the drift the second copy had been masking. The menu offered Summit, which was decommissioned and has no entry in toolchain/modules, so selecting it loaded nothing; and it omitted Phoenix IFX (pifx) and Santis (san), both of which have module sets that users could not discover. Add check_cluster_menu_slugs to lint_source so the hand-written menu cannot drift again: the slugs it advertises must match the cluster definitions in toolchain/modules, in both directions. Module-list lines (-{all,cpu,gpu}[-unload]) are excluded -- p-gpu-unload is an unload list, not a cluster. --- README.md | 1 - docs/documentation/troubleshooting.md | 3 +- toolchain/bootstrap/modules.sh | 7 +- toolchain/mfc/args.py | 6 - toolchain/mfc/cli/commands.py | 6 - toolchain/mfc/cli/docs_gen.py | 2 +- toolchain/mfc/lint_source.py | 43 +++++++ toolchain/mfc/user_guide.py | 170 +------------------------- 8 files changed, 51 insertions(+), 187 deletions(-) diff --git a/README.md b/README.md index d57972f20..831657365 100644 --- a/README.md +++ b/README.md @@ -109,7 +109,6 @@ And a high-amplitude acoustic wave reflecting and emerging through a circular or | `./mfc.sh validate case.py` | Check a case file for errors before running | | `./mfc.sh new my_case` | Create a new case from a template | | `./mfc.sh clean` | Remove build artifacts | -| `./mfc.sh help` | List the HPC clusters MFC ships module sets for | Run `./mfc.sh --help` for detailed options, or see the [full documentation](https://mflowcode.github.io/documentation/index.html). Tab completion for bash and zsh is auto-installed after you have run `./mfc.sh generate` (or any non-`new` command) at least once. Play with the examples in `examples/` ([showcased here](https://mflowcode.github.io/documentation/examples.html)). diff --git a/docs/documentation/troubleshooting.md b/docs/documentation/troubleshooting.md index 8ec148869..ee9758e2f 100644 --- a/docs/documentation/troubleshooting.md +++ b/docs/documentation/troubleshooting.md @@ -539,8 +539,7 @@ If you can't resolve an issue: 4. **Use the CLI help:** ```bash - ./mfc.sh help debugging - ./mfc.sh help gpu + ./mfc.sh --help ./mfc.sh -h ``` diff --git a/toolchain/bootstrap/modules.sh b/toolchain/bootstrap/modules.sh index d68811b2f..737716c4b 100644 --- a/toolchain/bootstrap/modules.sh +++ b/toolchain/bootstrap/modules.sh @@ -40,10 +40,11 @@ done # Get computer (if not supplied in command line) if [ -v $u_c ]; then log "Select a system:" - log "$G""ORNL$W: Ascent (a) | Frontier (f) | Frontier_amd (famd) | Summit (s) | Wombat (w)" + log "$G""ORNL$W: Ascent (a) | Frontier (f) | Frontier_amd (famd) | Wombat (w)" log "$B""LLNL $W: Tuolumne (tuo)" log "$C""ACCESS$W: Bridges2 (b) | Expanse (e) | Delta (d) | DeltaAI (dai)" - log "$Y""Gatech$W: Phoenix (p)" + log "$Y""Gatech$W: Phoenix (p) | Phoenix IFX (pifx)" + log "$C""CSCS$W: Santis (san)" log "$R""Caltech$W: Richardson (r)" log "$BR""Brown$W: Oscar (o)" log "$BR""Purdue$W: Anvil (pa)" @@ -52,7 +53,7 @@ if [ -v $u_c ]; then log "$OR""Florida$W: HiPerGator (h)" log "$C""WPI $W: Turing (t)" log "$R""AMD$W: HPCFund (amdfund)" - log_n "(${G}a${W}/${G}f${W}/${G}s${W}/${G}w${W}/${B}tuo${W}/${C}b${W}/${C}e${CR}/${C}d/${C}dai${CR}/${Y}p${CR}/${R}r${CR}/${B}cc${CR}/${B}c${CR}/${B}n${CR}/${BR}o${CR}/${BR}pa${CR}/${OR}i${CR}/${OR}h${CR}/${C}t${CR}/${R}amdfund${CR}): " + log_n "(${G}a${W}/${G}f${W}/${G}famd${W}/${G}w${W}/${B}tuo${W}/${C}b${W}/${C}e${CR}/${C}d/${C}dai${CR}/${Y}p${CR}/${Y}pifx${CR}/${C}san${CR}/${R}r${CR}/${B}cc${CR}/${B}c${CR}/${B}n${CR}/${BR}o${CR}/${BR}pa${CR}/${OR}i${CR}/${OR}h${CR}/${C}t${CR}/${R}amdfund${CR}): " read u_c log fi diff --git a/toolchain/mfc/args.py b/toolchain/mfc/args.py index 613d0d56d..c72e895f9 100644 --- a/toolchain/mfc/args.py +++ b/toolchain/mfc/args.py @@ -15,7 +15,6 @@ from .state import MFCConfig from .user_guide import ( is_first_time_user, - print_clusters_help, print_command_help, print_help, print_welcome, @@ -95,11 +94,6 @@ def custom_error(message): print_help() sys.exit(0) - # Handle 'help' command - if args["command"] == "help": - print_clusters_help() - sys.exit(0) - # Resolve command aliases if args["command"] in COMMAND_ALIASES: args["command"] = COMMAND_ALIASES[args["command"]] diff --git a/toolchain/mfc/cli/commands.py b/toolchain/mfc/cli/commands.py index f3752aa73..7dbaee849 100644 --- a/toolchain/mfc/cli/commands.py +++ b/toolchain/mfc/cli/commands.py @@ -719,11 +719,6 @@ ], ) -HELP_COMMAND = Command( - name="help", - help="List the HPC clusters MFC ships module sets for.", -) - # Simple commands (shell scripts, minimal arguments) LOAD_COMMAND = Command( name="load", @@ -1468,7 +1463,6 @@ PARAMS_COMMAND, PACKER_COMMAND, COMPLETION_COMMAND, - HELP_COMMAND, GENERATE_COMMAND, LOAD_COMMAND, LINT_COMMAND, diff --git a/toolchain/mfc/cli/docs_gen.py b/toolchain/mfc/cli/docs_gen.py index a6975f1ba..531998d69 100644 --- a/toolchain/mfc/cli/docs_gen.py +++ b/toolchain/mfc/cli/docs_gen.py @@ -241,7 +241,7 @@ def generate_cli_reference(schema: CLISchema) -> str: # Command categories core_commands = ["build", "run", "test", "clean", "validate"] - utility_commands = ["new", "viz", "params", "packer", "completion", "generate", "help"] + utility_commands = ["new", "viz", "params", "packer", "completion", "generate"] dev_commands = ["lint", "format", "spelling", "precheck", "count", "count_diff"] ci_commands = ["bench", "bench_diff"] other_commands = ["load"] diff --git a/toolchain/mfc/lint_source.py b/toolchain/mfc/lint_source.py index b3ca0d90f..d6664e197 100644 --- a/toolchain/mfc/lint_source.py +++ b/toolchain/mfc/lint_source.py @@ -508,6 +508,48 @@ def check_checker_input_constraints(repo_root: Path) -> list[str]: return errors +def check_cluster_menu_slugs(repo_root: Path) -> list[str]: + """Keep the ``./mfc.sh load`` cluster menu in sync with toolchain/modules. + + The menu in toolchain/bootstrap/modules.sh is hand-written so it can be + grouped and coloured by organisation. That is fine, but it drifts: it used + to offer Summit, which has no module set, and omitted Phoenix IFX and + Santis, which do. Compare the advertised slugs against the data file. + """ + modules = repo_root / "toolchain" / "modules" + script = repo_root / "toolchain" / "bootstrap" / "modules.sh" + if not modules.exists() or not script.exists(): + return [] + + # Cluster definitions in toolchain/modules are " ". The + # "-{all,cpu,gpu}[-unload] " lines carry the module lists. + module_list_line = re.compile(r"-(all|cpu|gpu)(-unload)?$") + defined = set() + for raw_line in modules.read_text().splitlines(): + line = raw_line.strip() + if not line or line.startswith("#"): + continue + slug = line.split()[0] + if not module_list_line.search(slug): + defined.add(slug) + + # The menu block runs from "Select a system:" to the answer prompt. + text = script.read_text() + try: + block = text[text.index("Select a system:") : text.index("read u_c")] + except ValueError: + return [f"{script.relative_to(repo_root)}: could not locate the cluster menu block"] + advertised = set(re.findall(r"\((\w[\w-]*)\)", block)) + + errors = [] + rel = script.relative_to(repo_root) + for slug in sorted(advertised - defined): + errors.append(f"{rel}: cluster menu offers '{slug}', which has no entry in toolchain/modules (selecting it loads nothing)") + for slug in sorted(defined - advertised): + errors.append(f"{rel}: toolchain/modules defines '{slug}', but the cluster menu does not offer it (users cannot discover it)") + return errors + + def main(): repo_root = Path(__file__).resolve().parents[2] @@ -522,6 +564,7 @@ def main(): all_errors.extend(check_hardcoded_byte_size(repo_root)) all_errors.extend(check_manual_registry_bcasts(repo_root)) all_errors.extend(check_checker_input_constraints(repo_root)) + all_errors.extend(check_cluster_menu_slugs(repo_root)) if all_errors: print("Source lint failed:") diff --git a/toolchain/mfc/user_guide.py b/toolchain/mfc/user_guide.py index b5103fa06..4bd404943 100644 --- a/toolchain/mfc/user_guide.py +++ b/toolchain/mfc/user_guide.py @@ -3,12 +3,10 @@ This module provides: - Enhanced help output with Rich formatting -- The cluster table behind ``./mfc.sh help``, derived from toolchain/modules - Onboarding for new users """ import os -import re from rich import box from rich.panel import Panel @@ -18,169 +16,6 @@ from .common import MFC_ROOT_DIR from .printer import cons -# DYNAMIC CLUSTER HELP GENERATION - -# Organization mapping based on system name prefixes and known clusters -CLUSTER_ORGS = { - "OLCF": "ORNL", - "LLNL": "LLNL", - "PSC": "ACCESS", - "SDSC": "ACCESS", - "NCSA": "ACCESS", - "GT": "Georgia Tech", - "Brown": "Brown", - "DoD": "DoD", - "Richardson": "Caltech", - "hipergator": "Florida", - "CSCS": "CSCS", -} - -# Explicit slug-to-org overrides (for cases where modules file naming is inconsistent) -SLUG_ORG_OVERRIDE = { - "tuo": "LLNL", # Tuolumne is at LLNL, not ORNL (modules file says "OLCF" incorrectly) -} - -# Display name overrides for clusters -SLUG_NAME_OVERRIDE = { - "h": "HiPerGator", # Proper capitalization -} - -# Display order and colors for organizations -ORG_ORDER = ["ORNL", "LLNL", "ACCESS", "Georgia Tech", "Caltech", "Brown", "DoD", "Florida", "CSCS"] -ORG_COLORS = { - "ORNL": "yellow", - "LLNL": "yellow", - "ACCESS": "yellow", - "Georgia Tech": "yellow", - "Caltech": "yellow", - "Brown": "yellow", - "DoD": "yellow", - "Florida": "yellow", -} - - -def _parse_modules_file(): - """Parse the modules file to extract cluster information. - - Returns a dict: {slug: {"name": full_name, "org": organization}} - """ - modules_path = os.path.join(MFC_ROOT_DIR, "toolchain", "modules") - clusters = {} - - try: - with open(modules_path, "r", encoding="utf-8") as f: - for raw_line in f: - line = raw_line.strip() - # Skip comments and empty lines - if not line or line.startswith("#"): - continue - # Skip lines with -all, -cpu, -gpu (module definitions) - if "-all" in line or "-cpu" in line or "-gpu" in line: - continue - - # Parse cluster definition lines: "slug System Name" - match = re.match(r"^(\S+)\s+(.+)$", line) - if match: - slug = match.group(1) - full_name = match.group(2).strip() - - # Check for explicit org override first - if slug in SLUG_ORG_OVERRIDE: - org = SLUG_ORG_OVERRIDE[slug] - else: - # Determine organization from name - org = "Other" - for prefix, org_name in CLUSTER_ORGS.items(): - if prefix in full_name or full_name.lower() == prefix.lower(): - org = org_name - break - - clusters[slug] = {"name": full_name, "org": org} - except FileNotFoundError: - # Fallback if modules file not found - pass - - return clusters - - -def _get_cluster_short_name(slug, full_name): - """Get display name for a cluster, with overrides and prefix stripping.""" - if slug in SLUG_NAME_OVERRIDE: - return SLUG_NAME_OVERRIDE[slug] - # Strip org prefix if present - for prefix in CLUSTER_ORGS: - if full_name.startswith(prefix + " "): - return full_name[len(prefix) + 1 :] - return full_name - - -def _generate_clusters_content(): - """Generate the clusters help content dynamically from modules file.""" - clusters = _parse_modules_file() - - # Group clusters by organization - org_clusters = {org: [] for org in ORG_ORDER} - org_clusters["Other"] = [] - - for slug, info in clusters.items(): - org = info["org"] - if org not in org_clusters: - org_clusters["Other"].append((slug, info["name"])) - else: - org_clusters[org].append((slug, info["name"])) - - # Build the cluster list section - cluster_lines = [] - for org in ORG_ORDER: - if not org_clusters.get(org): - continue - # Format: " [yellow]ORG:[/yellow] [cyan]slug[/cyan]=Name [cyan]slug2[/cyan]=Name2" - entries = [f"[cyan]{slug}[/cyan]={_get_cluster_short_name(slug, name)}" for slug, name in org_clusters[org]] - color = ORG_COLORS.get(org, "yellow") - cluster_lines.append(f" [{color}]{org}:[/{color}] " + " ".join(entries)) - - # Handle "Other" if any - if org_clusters.get("Other"): - entries = [f"[cyan]{slug}[/cyan]={name}" for slug, name in org_clusters["Other"]] - cluster_lines.append(" [yellow]Other:[/yellow] " + " ".join(entries)) - - cluster_list = "\n".join(cluster_lines) if cluster_lines else " [dim]No clusters found in modules file[/dim]" - - # Return full help content with dynamic cluster list - return f"""\ -[bold cyan]Supported HPC Clusters[/bold cyan] - -MFC includes pre-configured module sets for many clusters. - -[bold]Loading Cluster Modules:[/bold] - [green]source ./mfc.sh load -c -m [/green] - -[bold]Available Clusters:[/bold] -{cluster_list} - -[bold]Modes:[/bold] - [cyan]c[/cyan] or [cyan]cpu[/cyan] - CPU only - [cyan]g[/cyan] or [cyan]gpu[/cyan] - GPU enabled - -[bold]Examples:[/bold] - [green]source ./mfc.sh load -c p -m g[/green] Phoenix with GPU - [green]source ./mfc.sh load -c f -m g[/green] Frontier with GPU (AMD MI250X) - [green]source ./mfc.sh load -c d -m c[/green] Delta CPU-only - -[bold]Custom Clusters:[/bold] - For unlisted clusters, manually load: - • Fortran compiler (gfortran, nvfortran, amdflang, etc.) - • MPI implementation (OpenMPI, MPICH, Cray-MPICH) - • CMake 3.18+, Python 3.11+""" - - -def print_clusters_help(): - """Print the cluster configuration table behind ``./mfc.sh help``.""" - cons.print() - cons.raw.print(Panel(_generate_clusters_content(), title="[bold]Cluster Configuration[/bold]", box=box.ROUNDED, padding=(1, 2))) - cons.print() - - # ENHANCED HELP OUTPUT @@ -215,7 +50,7 @@ def print_help(): cons.print(f" [green]{cmd:9}[/green][dim]{alias_str:4}[/dim] {desc}") # Secondary commands (dimmed) - secondary = ["params", "load", "help"] + secondary = ["params", "load"] for cmd in secondary: if cmd not in COMMANDS: continue @@ -298,8 +133,7 @@ def print_welcome(): " [cyan]./mfc.sh run my_first_case/case.py[/cyan]\n\n" "[bold yellow]Optional:[/bold yellow] Enable tab completion for your shell:\n" " [cyan]./mfc.sh completion install[/cyan]\n\n" - "[dim]Run [cyan]./mfc.sh --help[/cyan] for all available commands[/dim]\n" - "[dim]Run [cyan]./mfc.sh help[/cyan] for the list of supported clusters[/dim]", + "[dim]Run [cyan]./mfc.sh --help[/cyan] for all available commands[/dim]", title="[bold]Getting Started[/bold]", box=box.DOUBLE, border_style="cyan", From 21ca34b399d779d91de8c90fc7fd500391ba80aa Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 9 Aug 2026 16:05:13 -0400 Subject: [PATCH 6/8] Remove the last operational references to Summit (decommissioned) Three places still offered Summit as a machine you could target: - modules.sh show_help listed "Summit (s)". This was a second, staler copy of the cluster list inside the same file -- it also omitted Tuolumne, Santis, Phoenix IFX, Anvil, HiPerGator, and Turing. Replace the list with a pointer to the interactive menu so the file holds one list, the one check_cluster_menu_slugs already validates. - running.md's "Example Runs" passed -c summit. For ./mfc.sh run, -c names a batch template in toolchain/templates, and summit.mako does not exist, so the documented command could not have worked. Point it at Frontier. - running.md described LSF as "e.g., Summit". LSF support stands; drop the example. Historical references are deliberately kept: the V100 weak-scaling results and figure in expectedPerformance.md, the gallery entries in docs/index.html recording where each simulation ran, the allocation acknowledgement and scaling record in the README, and the search keywords. Those describe what was done, not what users can target. --- docs/documentation/running.md | 6 +++--- toolchain/bootstrap/modules.sh | 7 +------ 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/docs/documentation/running.md b/docs/documentation/running.md index cd663634f..4bb9d8685 100644 --- a/docs/documentation/running.md +++ b/docs/documentation/running.md @@ -285,7 +285,7 @@ or contribute a new template. **SLURM systems:** Most clusters use SLURM. MFC automatically generates appropriate `sbatch` scripts. -**LSF systems (e.g., Summit):** +**LSF systems:** IBM's JSRUN does not use the traditional node-based approach. MFC constructs equivalent resource sets for task and GPU counts. --- @@ -376,11 +376,11 @@ We have provided an example, `case.py` and `restart_case.py` in `/examples/1D_va ## Example Runs -- Oak Ridge National Laboratory's [Summit](https://www.olcf.ornl.gov/summit/): +- Oak Ridge National Laboratory's [Frontier](https://www.olcf.ornl.gov/frontier/): ```shell ./mfc.sh run examples/2D_shockbubble/case.py -e batch \ - -N 2 -n 4 -t simulation -a -c summit + -N 2 -n 4 -t simulation -a -c frontier ``` diff --git a/toolchain/bootstrap/modules.sh b/toolchain/bootstrap/modules.sh index 737716c4b..5661a150b 100644 --- a/toolchain/bootstrap/modules.sh +++ b/toolchain/bootstrap/modules.sh @@ -8,12 +8,7 @@ show_help() { echo "Options:" echo " -h, --help Display this help message and exit." echo " -c, --computer COMPUTER Configures for COMPUTER environment." - echo " Options: Ascent (a) | Frontier (f) | Frontier_amd (famd) | Summit (s) | Wombat (w)" - echo " AMD HPCFund (amdfund)" - echo " Bridges2 (b) | Expanse (e) | Delta (d) | DeltaAI (dai)" - echo " Phoenix (p) | Richardson (r) | Oscar (o)" - echo " Carpenter Cray (cc) | Carpenter GNU (c) | Nautilus (n)" - echo " Isaac (i)" + echo " Omit to be shown the list of supported systems." echo " -m, --mode MODE Configures into MODE." echo " Options: gpu (g) | cpu (c)" echo "" From 94b856568a58dcfc3c43585bdc67bdf0e13c7397 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 9 Aug 2026 16:36:15 -0400 Subject: [PATCH 7/8] Tighten the constraint-test assertions (Copilot review) errors_for caught bare Exception and returned str(exc), so a regression in the validator that raised KeyError or TypeError would be stringified and could satisfy an assertion instead of failing the test. Catch only CaseConstraintError; anything else propagates. assertAccepts checked that one message was absent, which a case that broke for an unrelated reason would still satisfy. It now asserts full validity (no violations at all) and takes only the params. Every call site already uses a fully valid configuration, verified individually, so no test needed relaxing to accommodate the stronger form. assertRejects gained a check that validation failed at all, so a case that is wrongly accepted reports "expected validation to fail with X" rather than the less obvious "X not found in ''". Verified the new assertions catch what the old ones missed: injecting a KeyError into CaseValidator.validate now errors the test instead of passing; assertAccepts on a fixture with an unrelated violation (dt <= 0) now fails; assertRejects on a case that validates cleanly now fails. The 2D synthetic-turbulence z-component test folded into test_accepts_fully_specified_zone -- with assertAccepts asserting full validity, accepting a fixture that sets only d = 1, 2 is itself the proof that turb_pos(1,3) and synth_L(1,3) are not required. 32 -> 31 tests. --- toolchain/mfc/test_case_validator.py | 53 ++++++++++++++++------------ 1 file changed, 31 insertions(+), 22 deletions(-) diff --git a/toolchain/mfc/test_case_validator.py b/toolchain/mfc/test_case_validator.py index 31478cd7c..53a47216e 100644 --- a/toolchain/mfc/test_case_validator.py +++ b/toolchain/mfc/test_case_validator.py @@ -9,7 +9,7 @@ import unittest -from .case_validator import CaseValidator +from .case_validator import CaseConstraintError, CaseValidator # A minimal 1D case that passes simulation validation. BASE = { @@ -80,23 +80,34 @@ class ConstraintTestCase(unittest.TestCase): """Base class providing assertions over simulation-stage validation.""" def errors_for(self, params) -> str: - """Return all simulation-stage validation errors for params, joined.""" + """Return the constraint violations for params, or "" if it validates. + + Only CaseConstraintError is caught. Anything else -- a KeyError or + TypeError from a regression in the validator -- propagates and fails + the test, rather than being stringified into something an assertion + might accept. + """ validator = CaseValidator(dict(params)) try: validator.validate("simulation") - except Exception as exc: # CaseConstraintError + except CaseConstraintError as exc: return str(exc) return "" def assertRejects(self, params, expected: str): - """params must fail validation with expected in the message.""" + """params must fail validation, and expected must name the reason.""" errors = self.errors_for(params) + self.assertNotEqual(errors, "", f"expected validation to fail with {expected!r}, but the case was accepted") self.assertIn(expected, errors) - def assertAccepts(self, params, unexpected: str): - """params must not trip the check identified by unexpected.""" - errors = self.errors_for(params) - self.assertNotIn(unexpected, errors) + def assertAccepts(self, params): + """params must validate cleanly -- no violations at all. + + Asserting full validity rather than the absence of one message means a + fixture that breaks for an unrelated reason fails here instead of + silently satisfying a narrower check. + """ + self.assertEqual(self.errors_for(params), "") class TestImmersedBoundaryFlags(ConstraintTestCase): @@ -106,7 +117,7 @@ def test_requires_ib(self): self.assertRejects({**BASE, "many_ib_patch_parallelism": "T"}, self.MSG) def test_not_tripped_when_disabled(self): - self.assertAccepts(BASE, self.MSG) + self.assertAccepts(BASE) class TestBodyForceSpatialSupport(ConstraintTestCase): @@ -119,7 +130,7 @@ def test_rejects_3d(self): self.assertRejects({**BASE_2D, "p": 50, "bf_spatial_support": "T"}, self.MSG) def test_accepts_2d(self): - self.assertAccepts({**BASE_2D, "bf_spatial_support": "T"}, self.MSG) + self.assertAccepts({**BASE_2D, "bf_spatial_support": "T"}) class TestChemistrySubstepping(ConstraintTestCase): @@ -145,14 +156,14 @@ def test_rejects_max_unset_below_floor(self): self.assertRejects({**CHEMISTRY, "chem_params%adap_substeps": "T", "chem_params%reaction_substeps": 5}, "reaction_substeps_max must be >=") def test_accepts_no_substepping(self): - self.assertAccepts(CHEMISTRY, "reaction_substeps") + self.assertAccepts(CHEMISTRY) def test_accepts_valid_adaptive_substepping(self): params = {**CHEMISTRY, "chem_params%adap_substeps": "T", "chem_params%reaction_substeps": 2, "chem_params%reaction_substeps_max": 8} - self.assertAccepts(params, "reaction_substeps") + self.assertAccepts(params) def test_accepts_igr_without_substepping(self): - self.assertAccepts({**CHEMISTRY, "igr": "T", "chem_params%reaction_substeps": 0}, "not supported with igr") + self.assertAccepts({**CHEMISTRY, "igr": "T", "chem_params%reaction_substeps": 0}) class TestReactiveBurnFluidPairing(ConstraintTestCase): @@ -177,7 +188,7 @@ def test_rejects_unset_qv(self): self.assertRejects(params, "fluid_pp(1)%qv > fluid_pp(2)%qv") def test_accepts_valid_configuration(self): - self.assertEqual(self.errors_for(REACTIVE_BURN), "") + self.assertAccepts(REACTIVE_BURN) class TestSyntheticTurbulence(ConstraintTestCase): @@ -212,15 +223,13 @@ def test_rejects_nonpositive_extent(self): self.assertRejects({**self.ENABLED, "synth_L(1,2)": 0.0}, "synth_L(1,2) must be positive") def test_accepts_fully_specified_zone(self): - self.assertEqual(self.errors_for(self.ENABLED), "") - - def test_third_dimension_not_required_in_2d(self): - """The Fortran loops d = 1, num_dims, so a 2D case needs no z components.""" - self.assertAccepts(self.ENABLED, "turb_pos(1,3)") - self.assertAccepts(self.ENABLED, "synth_L(1,3)") + """ENABLED sets d = 1, 2 only. The Fortran loops d = 1, num_dims, so a 2D + case needs no z components -- accepting it proves turb_pos(1,3) and + synth_L(1,3) are not required.""" + self.assertAccepts(self.ENABLED) def test_not_checked_when_disabled(self): - self.assertAccepts(BASE_2D, "num_turbulent_sources") + self.assertAccepts(BASE_2D) class TestTimeStepPositivity(ConstraintTestCase): @@ -233,7 +242,7 @@ def test_rejects_zero_dt(self): self.assertRejects({**BASE, "dt": 0.0}, self.MSG) def test_accepts_positive_dt(self): - self.assertAccepts(BASE, self.MSG) + self.assertAccepts(BASE) if __name__ == "__main__": From 7f4939174fb25a8acde6ac056a14e011b8a159c7 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 9 Aug 2026 17:09:05 -0400 Subject: [PATCH 8/8] Fix three validation regressions and harden the new lint rule Self-review of this branch found that three checks I moved to Python are weaker than the Fortran they replaced, all from the same cause: Python's `is not None` / `_is_numeric` guards no-op on an absent key, while the deleted Fortran compared against the dflt_real / dflt_int sentinels, so an unset parameter was itself a violation. Each turned a startup abort into a silently wrong run. - reactive_burn now requires both fluid_pp(1)%{gamma,pi_inf} and fluid_pp(2)%{gamma,pi_inf} to be set. Previously, omitting fluid 2's EOS was accepted and the product phase ran with gamma = pi_inf = dflt_real = -1e6, a negative stiffened-gas EOS. The earlier reasoning only covered both-unset, where f_approx_equal(dflt, dflt) is true. - reactive_burn now requires num_fluids and model_eqns to be set, not merely correct-if-present. Omitting both was accepted outright. - dt is required whenever stepping is not CFL-driven. The adap_dt exemption had no counterpart in the Fortran, which checked `if (.not. cfl_dt) dt <= 0` regardless. Cases that genuinely need no dt use cfl_adap_dt (2D_lagrange_rising_bubble), which is unaffected. Four tests cover the gaps; all four fail against the previous validator. TestTimeStepPositivity became TestTimeStep, since it previously only exercised a rule that predates this branch. lint_source.py fixes, all in code this branch added: - The runtime-check marker was consumed by the next physical line, so a blank line or Fypp directive between it and the @:PROHIBIT silently dropped the exemption. It now persists until the next @:PROHIBIT. - The subroutine regex accepted only impure/pure, so `recursive subroutine` reported "at module scope" and told the developer to add `None` to the allowlist. It now accepts the full prefix set and gives scope-appropriate advice. - s_check_inputs_weno and s_check_inputs_muscl left the allowlist. They mix runtime and input-only checks -- the muscl_order/int_comp check this branch deletes lived in one of them -- so allowlisting the subroutines would have let a replacement back in unnoticed. Their six grid-extent checks now carry explicit markers instead, and an int_comp-style addition to either is caught. - check_cluster_menu_slugs keyed on the literal strings "Select a system:" and "read u_c", so rewording the prompt disabled it and editing the read broke precheck repo-wide. It now keys on explicit cluster-menu-begin/end markers, tolerates rewording, still catches slug drift, and fails loudly if the markers go missing. Also drops HELP_TOPICS from commands.py and the help_topics field from CLISchema: 25 lines describing five help topics that no longer exist and that nothing reads, left behind when the help system was removed. --- src/simulation/m_checker.fpp | 6 +++ toolchain/bootstrap/modules.sh | 5 ++ toolchain/mfc/case_validator.py | 25 ++++++---- toolchain/mfc/cli/commands.py | 23 ---------- toolchain/mfc/cli/schema.py | 3 -- toolchain/mfc/lint_source.py | 68 +++++++++++++++++----------- toolchain/mfc/test_case_validator.py | 42 +++++++++++++++-- 7 files changed, 108 insertions(+), 64 deletions(-) diff --git a/src/simulation/m_checker.fpp b/src/simulation/m_checker.fpp index 0c6324b2e..0c1bc8f2d 100644 --- a/src/simulation/m_checker.fpp +++ b/src/simulation/m_checker.fpp @@ -55,11 +55,14 @@ contains character(len=5) :: numStr !< for int to string conversion call s_int_to_str(num_stcls_min*weno_order, numStr) + ! lint: runtime-check m/n/p are per-rank extents after MPI decomposition, not the case-file values @:PROHIBIT(m + 1 < num_stcls_min*weno_order, & & "m must be greater than or equal to (num_stcls_min*weno_order - 1), whose value is " // trim(numStr)) + ! lint: runtime-check per-rank n @:PROHIBIT(n + 1 < min(1, n)*num_stcls_min*weno_order, & & "For 2D simulation, n must be greater than or equal to (num_stcls_min*weno_order - 1), whose value is " & & // trim(numStr)) + ! lint: runtime-check per-rank p @:PROHIBIT(p + 1 < min(1, p)*num_stcls_min*weno_order, & & "For 3D simulation, p must be greater than or equal to (num_stcls_min*weno_order - 1), whose value is " & & // trim(numStr)) @@ -72,11 +75,14 @@ contains character(len=5) :: numStr !< for int to string conversion call s_int_to_str(num_stcls_min*muscl_order, numStr) + ! lint: runtime-check m/n/p are per-rank extents after MPI decomposition, not the case-file values @:PROHIBIT(m + 1 < num_stcls_min*muscl_order, & & "m must be greater than or equal to (num_stcls_min*muscl_order - 1), whose value is " // trim(numStr)) + ! lint: runtime-check per-rank n @:PROHIBIT(n + 1 < min(1, n)*num_stcls_min*muscl_order, & & "For 2D simulation, n must be greater than or equal to (num_stcls_min*muscl_order - 1), whose value is " & & // trim(numStr)) + ! lint: runtime-check per-rank p @:PROHIBIT(p + 1 < min(1, p)*num_stcls_min*muscl_order, & & "For 3D simulation, p must be greater than or equal to (num_stcls_min*muscl_order - 1), whose value is " & & // trim(numStr)) diff --git a/toolchain/bootstrap/modules.sh b/toolchain/bootstrap/modules.sh index 5661a150b..932590729 100644 --- a/toolchain/bootstrap/modules.sh +++ b/toolchain/bootstrap/modules.sh @@ -33,6 +33,10 @@ while [[ $# -gt 0 ]]; do done # Get computer (if not supplied in command line) +# The slugs advertised below must match the cluster definitions in +# toolchain/modules; check_cluster_menu_slugs in toolchain/mfc/lint_source.py +# enforces that and keys on these two markers. +# lint: cluster-menu-begin if [ -v $u_c ]; then log "Select a system:" log "$G""ORNL$W: Ascent (a) | Frontier (f) | Frontier_amd (famd) | Wombat (w)" @@ -52,6 +56,7 @@ if [ -v $u_c ]; then read u_c log fi +# lint: cluster-menu-end # Get CPU/GPU (if not supplied in command-line) if [ -v $u_cg ]; then diff --git a/toolchain/mfc/case_validator.py b/toolchain/mfc/case_validator.py index 45ddbb643..21cc3a91d 100644 --- a/toolchain/mfc/case_validator.py +++ b/toolchain/mfc/case_validator.py @@ -814,7 +814,6 @@ def check_time_stepping(self): """Checks time stepping parameters (simulation/post-process)""" cfl_dt = self.get("cfl_dt", "F") == "T" cfl_adap_dt = self.get("cfl_adap_dt", "F") == "T" - adap_dt = self.get("adap_dt", "F") == "T" time_stepper = self.get("time_stepper") # Check time_stepper bounds @@ -853,10 +852,13 @@ def check_time_stepping(self): ) if not variable_dt: - # dt is required in pure fixed dt mode (not cfl_dt, not cfl_adap_dt) - # adap_dt mode uses dt as initial value, so it's optional + # dt is required whenever stepping is not CFL-driven. adap_dt is not an + # exemption: it uses dt as its initial value, and the Fortran checked + # `if (.not. cfl_dt) dt <= 0`, which fired on the dflt_real sentinel when + # dt was left unset. Cases that genuinely need no dt set cfl_adap_dt + # instead, which lands in the variable_dt branch above. uses_fixed_stepping = self.is_set("t_step_start") or self.is_set("t_step_stop") - self.prohibit(uses_fixed_stepping and not adap_dt and not self.is_set("dt"), "dt must be set when using fixed time stepping (t_step_start/t_step_stop)") + self.prohibit(uses_fixed_stepping and not self.is_set("dt"), "dt must be set when using fixed time stepping (t_step_start/t_step_stop)") def check_finite_difference(self): """Checks constraints on finite difference parameters""" @@ -1631,19 +1633,26 @@ def check_reactive_burn(self): reactive_burn = self.get("reactive_burn", "F") == "T" if not reactive_burn: return + # These mirror Fortran checks that compared against the dflt_real / dflt_int + # sentinels, so an unset parameter was a violation there. A bare + # "is not None" guard would silently pass the unset case instead. model_eqns = self.get("model_eqns") # Supported on the 5-equation (pressure-equilibrium) and 6-equation multi-fluid models. - self.prohibit(model_eqns is not None and model_eqns not in (2, 3), "reactive_burn requires model_eqns = 2 or 3 (5- or 6-equation multi-fluid model)") + self.prohibit(model_eqns not in (2, 3), "reactive_burn requires model_eqns = 2 or 3 (5- or 6-equation multi-fluid model) to be set") # Exactly two fluids (reactant = 1, product = 2) sharing the stiffened-gas EOS and # differing only in qv; violating these silently corrupts the mass/energy balance. - num_fluids = self.get("num_fluids") - self.prohibit(num_fluids is not None and num_fluids != 2, "reactive_burn requires num_fluids = 2 (reactant then product)") + self.prohibit(self.get("num_fluids") != 2, "reactive_burn requires num_fluids = 2 (reactant then product) to be set") for prop in ("gamma", "pi_inf"): v1 = self.get(f"fluid_pp(1)%{prop}") v2 = self.get(f"fluid_pp(2)%{prop}") + if not self._is_numeric(v1) or not self._is_numeric(v2): + # Unset defaults to dflt_real in the solver, so a missing value is + # either a negative EOS or a mismatch against the fluid that is set. + self.prohibit(True, f"reactive_burn requires both fluid_pp(1)%{prop} and fluid_pp(2)%{prop} to be set (reactant and product share the EOS)") + continue self.prohibit( - self._is_numeric(v1) and self._is_numeric(v2) and not math.isclose(v1, v2, rel_tol=1e-10), + not math.isclose(v1, v2, rel_tol=1e-10), f"reactive_burn requires fluid_pp(1)%{prop} == fluid_pp(2)%{prop} (reactant and product share the EOS)", ) # qv defaults to 0 in the Fortran, so an unset value is treated as 0 here to match. diff --git a/toolchain/mfc/cli/commands.py b/toolchain/mfc/cli/commands.py index 7dbaee849..a96484809 100644 --- a/toolchain/mfc/cli/commands.py +++ b/toolchain/mfc/cli/commands.py @@ -1413,28 +1413,6 @@ ) -# HELP TOPICS - -HELP_TOPICS = { - "gpu": { - "title": "GPU Configuration", - "description": "How to configure GPU builds and runs", - }, - "clusters": { - "title": "Cluster Configuration", - "description": "How to configure MFC for different HPC clusters", - }, - "batch": { - "title": "Batch Job Submission", - "description": "How to submit batch jobs with MFC", - }, - "debugging": { - "title": "Debugging & Troubleshooting", - "description": "Tips for debugging MFC issues", - }, -} - - # COMPLETE CLI SCHEMA MFC_CLI_SCHEMA = CLISchema( @@ -1483,7 +1461,6 @@ COMMON_GPUS, COMMON_MFC_CONFIG, ], - help_topics=HELP_TOPICS, ) diff --git a/toolchain/mfc/cli/schema.py b/toolchain/mfc/cli/schema.py index 871d98f6d..187c24cf9 100644 --- a/toolchain/mfc/cli/schema.py +++ b/toolchain/mfc/cli/schema.py @@ -187,9 +187,6 @@ class CLISchema: # Reusable argument sets common_sets: List[CommonArgumentSet] = field(default_factory=list) - # Help topics (separate from commands) - help_topics: dict = field(default_factory=dict) - def get_command(self, name: str) -> Optional[Command]: """Get a command by name or alias.""" for cmd in self.commands: diff --git a/toolchain/mfc/lint_source.py b/toolchain/mfc/lint_source.py index d6664e197..f9aa909e4 100644 --- a/toolchain/mfc/lint_source.py +++ b/toolchain/mfc/lint_source.py @@ -33,11 +33,17 @@ # MPI proxy source directory -> params-registry target key MPI_PROXY_TARGETS = {"pre_process": "pre", "simulation": "sim", "post_process": "post"} -# Checker subroutines allowed to hold @:PROHIBIT. Every one of these depends on -# state the Python validator cannot see at case-validation time: the MPI -# decomposition, per-rank grid extents, the active compiler, or a value Cantera -# fills in at runtime. Constraints between input parameters belong in +# Checker subroutines whose every @:PROHIBIT depends on state the Python +# validator cannot see: the active compiler, the MPI decomposition, or a value +# Cantera fills in at runtime. Constraints between input parameters belong in # toolchain/mfc/case_validator.py instead -- see check_checker_input_constraints. +# +# Subroutines that mix runtime and input-only checks are deliberately NOT listed. +# s_check_inputs_weno and s_check_inputs_muscl are the case in point: their +# grid-extent checks need per-rank m/n/p, but the muscl_order/int_comp check that +# used to sit alongside them was pure input, and allowlisting the subroutine would +# have let its replacement back in unnoticed. Those lines carry an explicit +# RUNTIME_CHECK_MARKER instead, so anything new added there is still flagged. RUNTIME_CHECKER_SUBROUTINES = { # Compiler conditionals (#ifdef / #if guarded). "s_check_amd", @@ -46,9 +52,6 @@ # MPI decomposition: n_global, num_procs_y/z. "s_check_total_cells", "s_check_inputs_fft", - # Per-rank grid extents m/n/p, which differ from the case-file values. - "s_check_inputs_weno", - "s_check_inputs_muscl", # num_species is populated by Cantera at runtime. "s_check_inputs_ib_injection", } @@ -56,6 +59,10 @@ # Opt out of check_checker_input_constraints for a single @:PROHIBIT. RUNTIME_CHECK_MARKER = "lint: runtime-check" +# Fortran subroutine declaration, allowing any order of the prefixes MFC uses +# (impure/pure/elemental/recursive/module) before the `subroutine` keyword. +_SUBROUTINE_DECL = re.compile(r"(?:(?:impure|pure|elemental|recursive|module|non_recursive)\s+)*subroutine\s+(\w+)") + def _is_comment_or_blank(stripped: str) -> bool: """True if stripped line is blank, a Fortran comment, or a Fypp directive.""" @@ -469,41 +476,48 @@ def check_checker_input_constraints(repo_root: Path) -> list[str]: languages, and the copies drift. A @:PROHIBIT is allowed only inside a subroutine in - RUNTIME_CHECKER_SUBROUTINES, or on a line preceded by a - "! lint: runtime-check " comment. + RUNTIME_CHECKER_SUBROUTINES, or under a "! lint: runtime-check " + comment. The marker applies to the next @:PROHIBIT, so blank lines, Fypp + directives, and further comments may sit between the two. """ errors = [] for path in sorted((repo_root / SRC_DIR).rglob("m_checker*.fpp")): rel = path.relative_to(repo_root) subroutine = None - exempt_next = False + exempt = False for lineno, line in enumerate(path.read_text().splitlines(), 1): stripped = line.strip() - match = re.match(r"(?:impure\s+|pure\s+)?subroutine\s+(\w+)", stripped) + match = _SUBROUTINE_DECL.match(stripped) if match: subroutine = match.group(1) elif stripped.startswith("end subroutine"): subroutine = None if stripped.startswith("!"): - exempt_next = RUNTIME_CHECK_MARKER in stripped + exempt = exempt or RUNTIME_CHECK_MARKER in stripped + continue + if not stripped or stripped.startswith("#"): + # Blank lines and Fypp/preprocessor directives do not consume the marker. continue - if "@:PROHIBIT" in stripped and not exempt_next: - if subroutine not in RUNTIME_CHECKER_SUBROUTINES: - where = f"in {subroutine}" if subroutine else "at module scope" + if "@:PROHIBIT" in stripped: + if not exempt and subroutine not in RUNTIME_CHECKER_SUBROUTINES: + if subroutine: + where = f"in {subroutine}" + allowlist_hint = f"add '{subroutine}' to RUNTIME_CHECKER_SUBROUTINES in {Path(__file__).name}" + else: + where = "at module scope" + allowlist_hint = f"put it in a subroutine listed in RUNTIME_CHECKER_SUBROUTINES in {Path(__file__).name}" errors.append( f"{rel}:{lineno}: @:PROHIBIT {where} looks like an input-only constraint. " f"Add it to a check_* method in toolchain/mfc/case_validator.py instead. " - f"If it genuinely needs runtime or compiler state, add {subroutine!r} to " - f"RUNTIME_CHECKER_SUBROUTINES in {Path(__file__).name}, or mark the line with " - f"'! {RUNTIME_CHECK_MARKER} '." + f"If it genuinely needs runtime or compiler state, {allowlist_hint}, " + f"or mark it with '! {RUNTIME_CHECK_MARKER} '." ) - - exempt_next = False + exempt = False return errors @@ -533,13 +547,15 @@ def check_cluster_menu_slugs(repo_root: Path) -> list[str]: if not module_list_line.search(slug): defined.add(slug) - # The menu block runs from "Select a system:" to the answer prompt. + # The menu is delimited by explicit markers rather than by prose, so rewording + # the prompt or the read cannot silently disable this check. text = script.read_text() - try: - block = text[text.index("Select a system:") : text.index("read u_c")] - except ValueError: - return [f"{script.relative_to(repo_root)}: could not locate the cluster menu block"] - advertised = set(re.findall(r"\((\w[\w-]*)\)", block)) + block = re.search(r"# lint: cluster-menu-begin\n(.*?)# lint: cluster-menu-end", text, re.S) + if block is None: + return [f"{script.relative_to(repo_root)}: cluster-menu-begin/end markers are missing; check_cluster_menu_slugs cannot verify the menu against toolchain/modules"] + # Slugs are advertised as "Name (slug)"; require the closing paren to be + # followed by a separator so shell fragments like ${G} are not picked up. + advertised = set(re.findall(r"\((\w[\w-]*)\)(?=[\s|\"']|$)", block.group(1), re.M)) errors = [] rel = script.relative_to(repo_root) diff --git a/toolchain/mfc/test_case_validator.py b/toolchain/mfc/test_case_validator.py index 53a47216e..e2101244a 100644 --- a/toolchain/mfc/test_case_validator.py +++ b/toolchain/mfc/test_case_validator.py @@ -187,6 +187,22 @@ def test_rejects_unset_qv(self): params = {k: v for k, v in REACTIVE_BURN.items() if not k.endswith("%qv")} self.assertRejects(params, "fluid_pp(1)%qv > fluid_pp(2)%qv") + def test_rejects_unset_fluid2_eos(self): + """Unset gamma/pi_inf defaults to dflt_real in the solver, so fluid 2 would + carry a negative stiffened-gas EOS. The Fortran caught this by comparing + against the sentinel; an `is not None` guard would silently pass it.""" + for prop in ("gamma", "pi_inf"): + params = {k: v for k, v in REACTIVE_BURN.items() if k != f"fluid_pp(2)%{prop}"} + self.assertRejects(params, f"both fluid_pp(1)%{prop} and fluid_pp(2)%{prop} to be set") + + def test_rejects_unset_num_fluids(self): + params = {k: v for k, v in REACTIVE_BURN.items() if k != "num_fluids"} + self.assertRejects(params, "reactive_burn requires num_fluids = 2") + + def test_rejects_unset_model_eqns(self): + params = {k: v for k, v in REACTIVE_BURN.items() if k != "model_eqns"} + self.assertRejects(params, "reactive_burn requires model_eqns = 2 or 3") + def test_accepts_valid_configuration(self): self.assertAccepts(REACTIVE_BURN) @@ -232,14 +248,32 @@ def test_not_checked_when_disabled(self): self.assertAccepts(BASE_2D) -class TestTimeStepPositivity(ConstraintTestCase): - MSG = "dt must be positive" +class TestTimeStep(ConstraintTestCase): + """The Fortran checked `if (.not. cfl_dt) dt <= 0`, which fired both on an + explicitly bad dt and on the dflt_real sentinel left by an unset one.""" def test_rejects_negative_dt(self): - self.assertRejects({**BASE, "dt": -1.0}, self.MSG) + self.assertRejects({**BASE, "dt": -1.0}, "dt must be positive") def test_rejects_zero_dt(self): - self.assertRejects({**BASE, "dt": 0.0}, self.MSG) + self.assertRejects({**BASE, "dt": 0.0}, "dt must be positive") + + def test_rejects_unset_dt_under_fixed_stepping(self): + params = {k: v for k, v in BASE.items() if k != "dt"} + self.assertRejects(params, "dt must be set when using fixed time stepping") + + def test_rejects_unset_dt_with_adap_dt(self): + """adap_dt is not an exemption -- it uses dt as its initial value, and the + Fortran aborted on the sentinel regardless of adap_dt.""" + params = {k: v for k, v in BASE.items() if k != "dt"} + params.update({"adap_dt": "T", "bubbles_euler": "T", "polytropic": "T", "adv_n": "T", "nb": 1}) + self.assertRejects(params, "dt must be set when using fixed time stepping") + + def test_accepts_cfl_adap_dt_without_dt(self): + """CFL-driven stepping genuinely needs no dt (e.g. 2D_lagrange_rising_bubble).""" + params = {k: v for k, v in BASE.items() if k not in ("dt", "t_step_start", "t_step_stop", "t_step_save")} + params.update({"cfl_adap_dt": "T", "cfl_target": 0.5, "t_stop": 1.0, "t_save": 0.1, "n_start": 0}) + self.assertAccepts(params) def test_accepts_positive_dt(self): self.assertAccepts(BASE)