Repository navigation
Reject chem_params%diffusion without chemistry; it segfaults on the first step - #2104
Closed
ruru25254649 wants to merge 1 commit into
Closed
ruru25254649 wants to merge 1 commit into
ruru25254649 wants to merge 1 commit into
Conversation
The diffusion flux arrays (flux_src_n(E) and the species slots) are allocated only when chemistry is on, but the Riemann-state zeroing and the s_compute_additional_physics_rhs loops test chem_params%diffusion alone. A non-reacting case with chem_params%diffusion = T passed validation and then segfaulted on the first time step. Prohibit the combination in check_chemistry, and give the matching deallocation in s_finalize_rhs_module the chemistry guard its allocation has (MFlowCode#1908). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WPhkY8p1EHhGY7jScizJmC
ruru25254649
requested review from
sbryngelson
and removed request for
a team
October 25, 2026 07:04
|
Claude Code Review Head SHA: 0ebf1ab Files changed:
Findings: |
ruru25254649
marked this pull request as draft
October 25, 2026 07:19
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
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
A case that sets
chem_params%diffusion = 'T'but notchemistry = 'T'passes./mfc.sh validateand then crashes on the first time step. For example,examples/1D_sodshocktubewith that one line added, on master3ef5e1ebuilt with GNU 13.4 + Open MPI:The flux arrays the diffusion path writes are allocated only under
if (chemistry)ins_initialize_rhs_module:flux_src_nflux_src_n(E)when nothing else allocates itSeveral loops test
chem_params%diffusionon its own, though, so they run in a non-reacting case:m_riemann_state.fpp:if (chem_params%diffusion)zeroesflux_src_vf(E:species%end)m_rhs.fpp:s_compute_additional_physics_rhsis called onviscous .or. surface_tension .or. chem_params%diffusion .or. heat_conduction, and inside it each direction'sif (chem_params%diffusion)block readsflux_src_n(E)#1908 points out the same mismatch on the deallocation side.
This PR stops the case at validation instead. It adds one rule next to the other chemistry checks in
check_chemistry:This follows the existing "parameter set without its feature" checks, such as
sigma is set but surface_tension is not enabled. Guarding the six-plus Fortran sites instead would spread one condition over several files.As #1908 asks, the deallocation in
s_finalize_rhs_modulealso gets thechemistry .and.guard its allocation has. With the new rule this is now unreachable, but it keeps allocation and deallocation symmetric.Two unit tests are added to
toolchain/mfc/test_case_validator.py: one for the rejection, and one showing that diffusion with chemistry is still accepted.Verification
chem_params%diffusion = 'T', masterCase parameter constraint violations: 1. chem_params%diffusion requires chemistry = Tpytest toolchain/mfc/test_case_validator.py5DCF300C 1550B67E E8372F50 5D59DDF0 4D7D85B4 2BDE2018 6B3AE48F F8ADA51B 7BBCE9E9 259B2428 C4EB58A8 56F8C4BC 8E56ACC1 4587725A 41AD0140), GNU 13.4 + Open MPI8E56ACC1,4587725Aand41AD0140)./mfc.sh precheck(including example-case validation: all 20 examples that setchem_params%diffusionalso setchemistry)961cde5(on a branch merging all candidates)Re-checked on master
961cde5: the Sod case withchem_params%diffusion = 'T'still SIGSEGVs on step 1, and this PR rejects it withchem_params%diffusion requires chemistry = T.Side effects
chem_params%diffusion = 'T'and nochemistryis now rejected at validation. It used to crash on step 1, so no working case changes.Contribution Policy
All contributions are expected to demonstrate:
If these expectations are not met, we would prefer to implement the changes ourselves rather than spend time reviewing low-effort submissions.
Acknowledgement
This PR is from Your Team Name participating in the SC26 Student Cluster Competition.
🤖 Generated with Claude Code
https://claude.ai/code/session_01WPhkY8p1EHhGY7jScizJmC