Repository navigation
Resolve the validator's Fortran constants from definitions.py - #2042
Conversation
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 MFlowCode#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 MFlowCode#2021, which caught the num_ibs cap; the num_patches_max one turned up while checking the rest.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71d2a36a7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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) |
There was a problem hiding this comment.
When src/ is unavailable and this fallback is revised, _load() still shadows the new module constant with its own _fc(..., 10) assignment at line 863, so the validator and the registered turb_pos/synth_L range retain independently edited limits and can drift again. Remove the local assignment and let _load() use this exported NTS so the change actually establishes one source of truth, as required by the repository's DRY guidance.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
_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.
|
Claude Code Review Head SHA: 15006cd Files changed:
Findings: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2042 +/- ##
=======================================
Coverage 61.91% 61.91%
=======================================
Files 86 86
Lines 22848 22848
Branches 3360 3360
=======================================
Hits 14147 14147
Misses 6207 6207
Partials 2494 2494 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Follow-up to #2021, from a Codex review comment on it.
case_validator.pyre-resolved sixm_constants.fppvalues with its ownget_fortran_constants().get(name, default)fallbacks — a second copy of numbersdefinitions.pyalready resolves through_fc(). Two had drifted:m_constants.fppnum_ib_patches_max_namelistnum_patches_maxThe first is what Codex caught: raising the
definitions.pyfallback in #2021 did not actually make 810000 usable, becausecheck_ibm()still cappednum_ibsat 50000. The second turned up while checking the remaining four (num_ib_airfoils_max,num_stl_models_max,num_turb_sources_max,num_bc_patches_max, all correct), and is the worse direction: too strict gives a clear rejection, too permissive sails past validation into an array overrun.No CI or development behaviour changes. These defaults are only reached when
src/is absent, as in the Homebrew layout they exist for; with the sources presentget_fortran_constants()already returned the real values, so the validator was already using 810000 and 10. Only the packaged case is affected.Changes
case_validator.pyimports the resolved constants fromdefinitions.pyinstead of re-defaulting, so there is one source of truth.get_fortran_constantsis no longer needed there.definitions.pygainsNTSfornum_turb_sources_max, the one constant the validator needed that was not already exposed.get_fortran_constants().get(name, default)whose default disagrees withm_constants.fpp.TestFortranConstantFallbackswalked_fc()calls indefinitions.pyonly, which is exactly why it passed over both of these — closing the class rather than the two instances.Testing
Written test-first: the new
TestFortranConstantGetFallbacksfails on master naming both stale fallbacks, and passes after the change.pytest toolchain/mfc/params_tests/test_definitions.py— 22 passedpytest toolchain/mfc/ -k "validator or case_valid"— 101 passed./mfc.sh precheck— all 7 stages pass