From 71d2a36a7b0ff9451a1c28d571e1696c1a9696da Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Mon, 12 Oct 2026 22:21:09 -0400 Subject: [PATCH 1/2] Resolve the validator's Fortran constants from definitions.py case_validator.py re-resolved six m_constants.fpp values with its own get_fortran_constants().get(name, default) fallbacks, a second copy of numbers definitions.py already resolves through _fc(). Two had drifted: - num_ib_patches_max_namelist: 50000 vs 810000, too strict, so #2021 raising the definitions.py fallback did not actually make 810000 usable. - num_patches_max: 1000 vs 10, too permissive, which is the worse direction -- it accepts 1000 IC patches into a 10-element Fortran array instead of rejecting them. These defaults are only reached when src/ is absent, as in a Homebrew layout, so with the sources present the validator already used the real values: no CI or development behaviour changes here, only the packaged case. Import the resolved constants instead, adding NTS for the one the validator needed that definitions.py did not already expose, and widen the fallback test to scan the whole toolchain for any get_fortran_constants().get(name, default) whose default disagrees with m_constants.fpp. The existing test walked _fc() calls in definitions.py only, which is why it passed over both of these. Reported by Codex on #2021, which caught the num_ibs cap; the num_patches_max one turned up while checking the rest. --- toolchain/mfc/case_validator.py | 16 ++++----- toolchain/mfc/params/definitions.py | 1 + .../mfc/params_tests/test_definitions.py | 35 +++++++++++++++++++ 3 files changed, 44 insertions(+), 8 deletions(-) diff --git a/toolchain/mfc/case_validator.py b/toolchain/mfc/case_validator.py index 36fce10a0..476009fbd 100644 --- a/toolchain/mfc/case_validator.py +++ b/toolchain/mfc/case_validator.py @@ -19,9 +19,9 @@ from . import eos from .common import MFCException -from .params.definitions import CONSTRAINTS +from .params.definitions import CONSTRAINTS, NAF, NB, NIB, NSM, NTS, NUM_PATCHES_MAX from .params.eos_families import EOS_FAMILIES -from .params.namelist_parser import get_fortran_constants, get_fortran_real_constants +from .params.namelist_parser import get_fortran_real_constants from .state import CFG # Above this the Ensemble-Averaged Bubble Model's O(alpha) expansion, which enters the closure as @@ -790,7 +790,7 @@ def check_ibm(self): ib and num_ibs <= 0 and not has_particle_clouds, "num_ibs must be >= 1 when ib is enabled (or specify at least one particle_cloud with num_particles > 0)", ) - num_ib_patches_max = get_fortran_constants().get("num_ib_patches_max_namelist", 50000) + num_ib_patches_max = NIB self.prohibit( ib and num_ibs > num_ib_patches_max, f"num_ibs must be <= {num_ib_patches_max} (num_ib_patches_max_namelist in m_constants.fpp)", @@ -996,8 +996,8 @@ def check_ibm(self): f"particle_cloud({i}) hemisphere shell {name}-extent must lie within {name}_domain", ) - num_ib_airfoils_max = get_fortran_constants().get("num_ib_airfoils_max", 5) - num_stl_models_max = get_fortran_constants().get("num_stl_models_max", 10) + num_ib_airfoils_max = NAF + num_stl_models_max = NSM num_stl_models = self.get("num_stl_models", 0) self.prohibit( num_stl_models < 0, @@ -1436,7 +1436,7 @@ def check_synthetic_turbulence(self): 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) + num_turb_sources_max = NTS 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)", @@ -2011,7 +2011,7 @@ def check_restart(self): self.prohibit(old_grid and t_step_old is None, "old_grid requires t_step_old to be set") self.prohibit(num_patches < 0, "num_patches must be non-negative") self.prohibit(num_patches == 0 and t_step_old is None, "num_patches must be positive for the non-restart case") - num_patches_max = get_fortran_constants().get("num_patches_max", 1000) + num_patches_max = NUM_PATCHES_MAX self.prohibit( num_patches > num_patches_max, f"num_patches must be <= {num_patches_max} (num_patches_max in m_constants.fpp)", @@ -2407,7 +2407,7 @@ def check_patch_physics(self): def check_bc_patches(self): """Checks boundary condition patch geometry (pre-process)""" num_bc_patches = self.get("num_bc_patches", 0) - num_bc_patches_max = get_fortran_constants().get("num_bc_patches_max", 10) + num_bc_patches_max = NB if num_bc_patches <= 0: return diff --git a/toolchain/mfc/params/definitions.py b/toolchain/mfc/params/definitions.py index 159dd9aae..7ee6f91ba 100644 --- a/toolchain/mfc/params/definitions.py +++ b/toolchain/mfc/params/definitions.py @@ -35,6 +35,7 @@ def _fc(name: str, default: int) -> int: NAF = _fc("num_ib_airfoils_max", 5) # ib_airfoil (Fortran array bound) NSM = _fc("num_stl_models_max", 10) # stl_models (Fortran array bound) NPB = _fc("num_particle_clouds_max", 10) # particle_cloud (Fortran array bound) +NTS = _fc("num_turb_sources_max", 10) # turbulent source array bound (case_validator) # Enumeration limits for families not yet converted to IndexedFamily. # These are smaller than the Fortran array bounds to keep the registry compact. # The CONSTRAINTS dict below uses the Fortran constants for validation. diff --git a/toolchain/mfc/params_tests/test_definitions.py b/toolchain/mfc/params_tests/test_definitions.py index 3c4cc913b..70dfaeb0d 100644 --- a/toolchain/mfc/params_tests/test_definitions.py +++ b/toolchain/mfc/params_tests/test_definitions.py @@ -6,6 +6,8 @@ import ast import inspect +import pathlib +import re import unittest from ..params import REGISTRY, definitions @@ -194,3 +196,36 @@ def test_fallbacks_match_m_constants(self): for call in calls: name, default = (ast.literal_eval(arg) for arg in call.args) self.assertEqual(default, constants[name], f"_fc('{name}', {default}) fallback differs from m_constants.fpp") + + +class TestFortranConstantGetFallbacks(unittest.TestCase): + """The same rule for get_fortran_constants().get(name, default) anywhere in the toolchain. + + definitions.py resolves its constants through _fc(), which the test above + guards. Callers that re-resolve a constant with their own .get() default + are a second, unguarded copy of the same number: case_validator.py capped + num_ibs at 50000 while m_constants.fpp said 810000, and allowed 1000 + num_patches while the Fortran array holds 10 -- too strict in one + direction and, worse, too permissive in the other. Those defaults are only + reached when src/ is absent, as in a Homebrew layout, so nothing at + development time notices they have drifted. + + Prefer importing the already-resolved constant from definitions.py. This + test allows a .get() default only when it still matches the Fortran. + """ + + def test_get_fallbacks_match_m_constants(self): + constants = get_fortran_constants() + self.assertTrue(constants, "m_constants.fpp could not be parsed") + + toolchain = pathlib.Path(__file__).resolve().parents[2] + pattern = re.compile(r"""get_fortran_constants\(\)\.get\(\s*["'](\w+)["']\s*,\s*(\d+)\s*\)""") + + stale = [] + for path in sorted(toolchain.rglob("*.py")): + for name, default in pattern.findall(path.read_text(encoding="utf-8", errors="ignore")): + if name in constants and int(default) != constants[name]: + rel = path.relative_to(toolchain) + stale.append(f"{rel}: .get('{name}', {default}) != {constants[name]} in m_constants.fpp") + + self.assertEqual(stale, [], "stale Fortran-constant fallbacks:\n " + "\n ".join(stale)) From 15006cd92e2f918d25065e6140be626879d141fa Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Mon, 12 Oct 2026 22:36:15 -0400 Subject: [PATCH 2/2] Drop the local NTS that shadowed the exported one _load() resolved num_turb_sources_max a second time, so the NTS that case_validator imports and the NTS that registers turb_pos/synth_L were two independently editable assignments -- not the single source of truth this PR is for. Use the module-level constant. Guard it: no name may be resolved by more than one _fc() call. It was the only duplicate; num_synth_shells_max stays local because nothing outside _load() needs it. Reported by Codex on this PR. --- toolchain/mfc/params/definitions.py | 3 +-- toolchain/mfc/params_tests/test_definitions.py | 13 +++++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/toolchain/mfc/params/definitions.py b/toolchain/mfc/params/definitions.py index 7ee6f91ba..ba5613db2 100644 --- a/toolchain/mfc/params/definitions.py +++ b/toolchain/mfc/params/definitions.py @@ -35,7 +35,7 @@ def _fc(name: str, default: int) -> int: NAF = _fc("num_ib_airfoils_max", 5) # ib_airfoil (Fortran array bound) NSM = _fc("num_stl_models_max", 10) # stl_models (Fortran array bound) NPB = _fc("num_particle_clouds_max", 10) # particle_cloud (Fortran array bound) -NTS = _fc("num_turb_sources_max", 10) # turbulent source array bound (case_validator) +NTS = _fc("num_turb_sources_max", 10) # turb_pos, synth_L (Fortran array bound) # Enumeration limits for families not yet converted to IndexedFamily. # These are smaller than the Fortran array bounds to keep the registry compact. # The CONSTRAINTS dict below uses the Fortran constants for validation. @@ -860,7 +860,6 @@ def _load(): _r("num_turbulent_sources", INT, {"synthetic_turbulence"}) _r("synth_U_inf", REAL, {"synthetic_turbulence"}, math=r"\f$U_\infty\f$") NSS = _fc("num_synth_shells_max", 50) - NTS = _fc("num_turb_sources_max", 10) for i in range(1, NSS + 1): _r(f"synth_n_waves_per_shell({i})", INT, {"synthetic_turbulence"}) _r(f"synth_k_shell({i})", REAL, {"synthetic_turbulence"}, math=r"\f$k_s\f$") diff --git a/toolchain/mfc/params_tests/test_definitions.py b/toolchain/mfc/params_tests/test_definitions.py index 70dfaeb0d..bb0c20b57 100644 --- a/toolchain/mfc/params_tests/test_definitions.py +++ b/toolchain/mfc/params_tests/test_definitions.py @@ -197,6 +197,19 @@ def test_fallbacks_match_m_constants(self): name, default = (ast.literal_eval(arg) for arg in call.args) self.assertEqual(default, constants[name], f"_fc('{name}', {default}) fallback differs from m_constants.fpp") + def test_each_constant_is_resolved_once(self): + """No constant should be resolved by two _fc() calls. + + A second _fc() for the same name is a second place to edit. _load() + had its own NTS = _fc("num_turb_sources_max", 10) shadowing the + module-level one, so importing NTS elsewhere would not have given a + single source of truth. + """ + calls = [node for node in ast.walk(ast.parse(inspect.getsource(definitions))) if isinstance(node, ast.Call) and getattr(node.func, "id", None) == "_fc"] + names = [ast.literal_eval(call.args[0]) for call in calls] + duplicated = sorted({name for name in names if names.count(name) > 1}) + self.assertEqual(duplicated, [], f"resolved by more than one _fc() call: {duplicated}") + class TestFortranConstantGetFallbacks(unittest.TestCase): """The same rule for get_fortran_constants().get(name, default) anywhere in the toolchain.