Skip to content

Complete named-constant adoption and parameter declaration generation#1551

Open
sbryngelson wants to merge 9 commits into
MFlowCode:masterfrom
sbryngelson:params-pipeline-completion
Open

Complete named-constant adoption and parameter declaration generation#1551
sbryngelson wants to merge 9 commits into
MFlowCode:masterfrom
sbryngelson:params-pipeline-completion

Conversation

@sbryngelson

Copy link
Copy Markdown
Member

Summary

Follow-up to #1550, completing two more pieces of the same strategy — one source of truth in the parameter registry, generated everywhere, no behavior change:

  1. Named-constant adoption finishes. The 9 remaining enumerated vocabularies (bubble_model, avg_state, wave_speeds, recon_type, muscl_order, muscl_lim, int_comp, format, precision) are swept across all three executables — 99 decision sites move from integer literals to the generated named constants. The legacy WENO_TYPE/MUSCL_TYPE constants are retired in favor of the generated recon_type_weno/recon_type_muscl (same values, one naming scheme).
  2. The params pipeline now generates ALL namelist-bound declarations. A TYPED_DECLS table in definitions.py drives generation of the 14 derived-type namelist declarations (fluid_pp, patch_icpp, acoustic, …) per target — including their GPU_DECLARE lines and Doxygen descriptions — and the simulation case-optimization #:if MFC_CASE_OPTIMIZATION block is emitted from a registry loop instead of being hand-maintained. The corresponding manual declarations in the three m_global_parameters.fpp are deleted (net −40 LOC of hand-synced Fortran). Adding a parameter no longer requires touching Fortran declarations at all; adding a case-optimization parameter no longer requires editing a parallel #:else block.

Two infrastructure hardenings that fell out of the work:

  • Generated includes regenerate at build time. Previously they were written only at CMake configure time, so a cached build that skipped reconfigure compiled stale includes (a silent wrong-binary hazard we hit during development). A ninja-tracked add_custom_command now owns regeneration: touch definitions.py && ninja does the right thing without reconfigure.
  • Doc lint accepts registry-derived tokens. lint_docs.py validates backticked tokens in case.md against the registry's value names and the analytic-expression intrinsics instead of a hand-grown skip list.

Verification

  • Every Fortran-touching commit was gated on the full golden suite (sharded across two parallel jobs) with zero golden-file regenerations, four full-suite runs in total including one against current master after Unify ICPP STL onto the shared IB model path #1546 landed.
  • The declaration-generation steps were additionally proven by declaration-set equivalence on the Fypp-emitted .f90: every declaration and GPU-residency directive before the deletion is byte-identical (modulo relocation and comments) to the generated form after, per target and — for the case-optimization block — in both MFC_CASE_OPTIMIZATION configurations.
  • Toolchain pytest suite extended to 323 tests; precheck green on every commit.

What still requires manual edits (documented in .claude/rules/common-pitfalls.md)

Derived-type member definitions in m_derived_types.fpp, default-value assignments, the six case.py-computed case-opt extras, multi-variable declaration lines (bc_x/y/z, domains), and the MPI broadcast lists — the broadcast lists are the next generation target.

Copilot AI review requested due to automatic review settings June 10, 2026 18:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@codecov

codecov Bot commented Jun 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 23.95833% with 73 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.17%. Comparing base (c0792ec) to head (12dfa2a).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/post_process/m_data_output.fpp 5.00% 7 Missing and 12 partials ⚠️
src/simulation/m_riemann_solvers.fpp 25.00% 4 Missing and 5 partials ⚠️
src/common/m_mpi_common.fpp 0.00% 0 Missing and 7 partials ⚠️
src/simulation/m_qbmm.fpp 50.00% 0 Missing and 5 partials ⚠️
src/simulation/m_bubbles_EL.fpp 0.00% 0 Missing and 4 partials ⚠️
src/simulation/m_muscl.fpp 0.00% 0 Missing and 4 partials ⚠️
src/post_process/m_mpi_proxy.fpp 0.00% 1 Missing and 2 partials ⚠️
src/pre_process/m_data_output.fpp 0.00% 0 Missing and 3 partials ⚠️
src/simulation/m_cbc.fpp 25.00% 0 Missing and 3 partials ⚠️
src/simulation/m_data_output.fpp 0.00% 0 Missing and 3 partials ⚠️
... and 8 more
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1551   +/-   ##
=======================================
  Coverage   61.17%   61.17%           
=======================================
  Files          74       74           
  Lines       20313    20313           
  Branches     2961     2961           
=======================================
  Hits        12427    12427           
  Misses       5870     5870           
  Partials     2016     2016           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sbryngelson

Copy link
Copy Markdown
Member Author

Code review

No issues found in this PR's own commits. Checked for bugs and CLAUDE.md compliance across five independent passes (compliance, shallow bug scan, git-history conflicts with recent master merges, prior-PR feedback, and in-code guidance), with emitted-declaration equivalence verified per target. The codecov patch-coverage number reads low because the diff is dominated by generated Fortran declarations, which the golden regression suite (not unit tests) exercises; the toolchain Python in this PR is unit-covered.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants