refactor(params): auto-generate Fortran namelists from Python, eliminate toolchain duplication#1456
Merged
Merged
Conversation
Claude Code ReviewHead SHA: b5eeea3 Files changed:
Findings:
The newly added CMake stamp block lists set(_mfc_gen_inputs
"${CMAKE_CURRENT_SOURCE_DIR}/toolchain/mfc/params/generators/cmake_gen.py"
"${CMAKE_CURRENT_SOURCE_DIR}/toolchain/mfc/params/generators/fortran_gen.py"
"${CMAKE_CURRENT_SOURCE_DIR}/toolchain/mfc/params/namelist_targets.py" ← does not exist
"${CMAKE_CURRENT_SOURCE_DIR}/toolchain/mfc/params/definitions.py"
)This file does not exist anywhere in the repository —
# generate_namelist_fpp — sim path
nl_lines = _pack_namelist(normal, _FIRST_PREFIX, _CONT_PREFIX, _MAX_LINE)
nl_lines[-1] += ", &" # IndexError if nl_lines == []
|
Contributor
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1456 +/- ##
=======================================
Coverage 61.31% 61.31%
=======================================
Files 72 72
Lines 19771 19771
Branches 2852 2852
=======================================
Hits 12123 12123
Misses 5699 5699
Partials 1949 1949 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…n/namelist systems - Delete namelist_targets.py: NAMELIST_VARS moved to definitions.py (one file to edit when adding a param); CASE_OPT_EXCLUDE was a duplicate of CASE_OPT_PARAMS - Delete _FALLBACK_PARAMS (380 hardcoded lines) in namelist_parser.py: replaced by _fallback_params() that derives the same data from NAMELIST_VARS at runtime - Delete _SIMPLE_DESCS (145 lines) and _auto_describe in definitions.py: _r() now populates ParamDef.description from descriptions.get_description() so callers can read param.description directly; three callers (docs_gen, json_schema_gen, params_cmd) migrated off the external get_description() import - Delete _PREFIX_DESCS and _ATTR_DESCS (113 lines) in definitions.py: dead code after _auto_describe was removed - Add resolve_namelist_content() and TARGET_FROM_DIR to fortran_gen.py: single canonical helper for the detect-generated-include pattern duplicated across namelist_parser.py and lint_param_docs.py - Consolidate NAMELIST_VARS to use _nv() helper: 200-line explicit dict replaced by grouped calls (~75 lines); cmake_gen.py refactored to use get_generated_files() - Switch test runner from hardcoded unittest module list to pytest discover; add pytest to pyproject.toml; move three orphaned test files into params_tests/ - CMake: stamp-based caching so Fortran codegen only reruns when inputs change - Extract _format_constraints_cell() in docs_gen.py: 5-line pattern duplicated across pattern-view and full-table rendering paths - Inline 4 single-use wrapper functions in case_dicts.py Net vs master: -564 lines
- run/run.py: replace 4 duplicate profiler if-blocks with data-driven table; adding a new profiler now requires one entry in _PROFILERS - lint_source.py: remove check_pylint_directives — MFC uses ruff, no pylint directives exist anywhere in the codebase, check was provably dead code - bench.py: delete self-deprecating TODO comment - run/input.py: log Cantera candidate failures instead of swallowing silently; previously a Cantera yaml parse error looked identical to file-not-found
case_validator.py: - Add _check_order_fits_grid(): the 3-line m/n/p dimension check was duplicated identically in check_igr, check_weno, check_muscl; now one call - Add _get_recon_type(): the recon_type read + prohibit was duplicated in check_weno, check_muscl, check_weno_simulation, check_muscl_simulation; now one call each - Collapse two named param lists in check_muscl into inline literals test/cases.py: - Add make_3d_box_patches(): the 18-line 3-patch 3D box IC setup was copy-pasted identically in mpi_consistency_tests, restart_roundtrip_tests, and kernel_golden_tests; now one line each lint_docs.py: - Add private helpers to skip set (they call prohibit but are not check_* methods and should not require PHYSICS_DOCS entries)
The refac-params branch dropped the bare alpha_wrt scalar param from definitions.py — it was in master's _wrt loop but not carried over. This caused ./mfc.sh validate to reject example cases using 'alpha_wrt': 'T' (the schema had patternProperties for alpha_wrt(N) but not the scalar form), matching what master registers: both the bare scalar and the indexed forms.
generate_decls_fpp was emitting unconditional declarations for all sim-target variables including CASE_OPT_PARAMS (recon_type, weno_order, nb, mapped_weno, viscous, etc.). m_global_parameters.fpp for simulation contains a manual #:if MFC_CASE_OPTIMIZATION / #:else block that declares these same variables — as compile-time constants in case-opt builds and as regular variables otherwise. The generated include + the manual block created duplicate declarations, which is a Fortran compile error for every ./mfc.sh build --case-optimization invocation. Fix: skip CASE_OPT_PARAMS when generating declarations for the sim target. The manual block handles both the parameter and variable cases.
…ompletion flags run/input.py: - cantera_file was only assigned in the chemistry=T branch; the error message on line 69 would raise NameError when chemistry=F and all candidates (h2o2.yaml) fail to load — set cantera_file='h2o2.yaml' before the branch so the error message is always valid run/archive.py: - Extract _relpath_safe(path, dirpath) to replace three inline try/except blocks scattered across __build_manifest, __copy_dir, __write_tar - Fix manifest fallback: was falling back to the full absolute path on cross-device paths; now uses basename like copy/tar already did — manifest and archive contents now agree cli/completion_gen.py: - Bash and zsh completion lists for MFC config flags were hardcoded, already missing --reldebug/--no-reldebug; derive dynamically from MFCConfig dataclass fields (same source argparse_gen.py already uses) - New MFCConfig fields are now automatically reflected in completions
… example validation to precheck
…ling continuation
- completion_gen: fix zsh --gpu completion ('acc:mp' -> 'acc mp')
- case_validator: move recon_type prohibit to check_parameter_types (was duplicated 4x)
- CMakeLists: remove nonexistent namelist_targets.py stamp watch; add registry.py + schema.py
- fortran_gen: guard nl_with_cont[-1] against empty normal list
- docs_gen: use .get() for REGISTRY lookups to avoid KeyError on unknown params
Fortran decls and namelist bindings are now auto-generated. Simple scalar params only need definitions.py (_r + _nv) + cmake reconfigure.
Adds fortran_dim support for indexed-family array parameters. fluid_rho (pre) and 9 output arrays (post) no longer need manual Fortran declarations — all are generated from definitions.py. Remaining manual declarations are exclusively Fortran derived-type variables (bc_x, fluid_pp, patch_icpp, etc.) which require type information that lives in m_derived_types.fpp.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.fppfiles from the Python parameter registry (definitions.py) at CMake configure time, replacing ~390 lines of hand-written boilerplate across the 6m_global_parameters.fpp/m_start_up.fppfilesdefinitions.pyonly (previously 3–4 files)namelist_parser.py,descriptions.py,case_validator.py,test/cases.py, and several other filesWhat changed
Fortran codegen (original branch):
fortran_gen.py+cmake_gen.py: generategenerated_namelist.fppandgenerated_decls.fppper target at CMake configure timedefinitions.pyornamelist_targets.pyactually changem_start_up.fppandm_global_parameters.fppfor all 3 targets now#:includethe generated filesToolchain cleanup (on top):
NAMELIST_VARSmoved fromnamelist_targets.pyintodefinitions.py(deletednamelist_targets.py);CASE_OPT_EXCLUDEwas a duplicate of the existingCASE_OPT_PARAMS_FALLBACK_PARAMS(380 hardcoded lines innamelist_parser.py) replaced by a 4-line function that derives the same data fromNAMELIST_VARS— the old dict was also missing 5 simulation params that were in the actual Fortran_SIMPLE_DESCS+_auto_describe(200 lines indefinitions.py) deleted;ParamDef.descriptionis now populated fromdescriptions.get_description()at registration time, giving callers a single place to read descriptions_PREFIX_DESCS+_ATTR_DESCS(113 lines) deleted — dead code after_auto_describeremovalresolve_namelist_content()infortran_gen.py; bothnamelist_parser.pyandlint_param_docs.pynow use itlint_source.py: removed deadcheck_pylint_directives(no pylint directives exist in the codebase)run/run.py: 4 duplicate profiler if-blocks →_PROFILERSdata tablecase_validator.py: extracted_check_order_fits_grid()and_get_recon_type()— each was copy-pasted across 3–4 check methodstest/cases.py: extractedmake_3d_box_patches()— 18-line IC setup block was copy-pasted into 3 test functionsunittestmodule list topytestdiscover; moved 3 orphaned test files that were never runningNet diff vs master
−630 net lines. The original branch was +330 net before cleanup.
Test plan
./mfc.sh precheck -j 8passes (all 6 checks: format, spell, lint, source lint, doc refs, param docs)./mfc.sh build -j 8compiles with gfortran (CI will verify nvfortran, Cray ftn, ifx)./mfc.sh test -j 8regression suite passes./mfc.sh build --gpu acc -j 8on a GPU node (OpenACC path)./mfc.sh build --gpu mp -j 8on a GPU node (OpenMP path).fppfiles match expected namelist content for each target