Skip to content

Reject chem_params%diffusion without chemistry; it segfaults on the first step - #2104

Closed
ruru25254649 wants to merge 1 commit into
MFlowCode:masterfrom
ruru25254649:fix-chem-diffusion-without-chemistry
Closed

ruru25254649 wants to merge 1 commit into
MFlowCode:masterfrom
ruru25254649:fix-chem-diffusion-without-chemistry

Conversation

@ruru25254649

Copy link
Copy Markdown
Contributor

Summary

A case that sets chem_params%diffusion = 'T' but not chemistry = 'T' passes ./mfc.sh validate and then crashes on the first time step. For example, examples/1D_sodshocktube with that one line added, on master 3ef5e1e built with GNU 13.4 + Open MPI:

 Simulating a regular 399x0x0 case on 1 rank(s) on CPUs.
 [  0%] step 1 of 21 (t_step 0) ...
Program received signal SIGSEGV: Segmentation fault - invalid memory reference.

The flux arrays the diffusion path writes are allocated only under if (chemistry) in s_initialize_rhs_module:

  • the species slots of flux_src_n
  • flux_src_n(E) when nothing else allocates it

Several loops test chem_params%diffusion on its own, though, so they run in a non-reacting case:

  • m_riemann_state.fpp: if (chem_params%diffusion) zeroes flux_src_vf(E:species%end)
  • m_rhs.fpp: s_compute_additional_physics_rhs is called on viscous .or. surface_tension .or. chem_params%diffusion .or. heat_conduction, and inside it each direction's if (chem_params%diffusion) block reads flux_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:

self.prohibit(diffusion and not chemistry, "chem_params%diffusion requires chemistry = T")

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_module also gets the chemistry .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

Check Result
Sod shock tube + chem_params%diffusion = 'T', master passes validation, then SIGSEGV on step 1 (GNU 13.4 + Open MPI, CPU)
Same case, this PR Case parameter constraint violations: 1. chem_params%diffusion requires chemistry = T
pytest toolchain/mfc/test_case_validator.py 96 passed, including the 2 new tests
All 15 chemistry regression tests (5DCF300C 1550B67E E8372F50 5D59DDF0 4D7D85B4 2BDE2018 6B3AE48F F8ADA51B 7BBCE9E9 259B2428 C4EB58A8 56F8C4BC 8E56ACC1 4587725A 41AD0140), GNU 13.4 + Open MPI 15 passed, 0 failed (these include the diffusion cases 8E56ACC1, 4587725A and 41AD0140)
./mfc.sh precheck (including example-case validation: all 20 examples that set chem_params%diffusion also set chemistry) passes
Full regression suite, GNU 13.3 + Open MPI 4.1.6, CPU, master 961cde5 (on a branch merging all candidates) 725 passed, 0 failed (39 convergence cases skipped by default)

Re-checked on master 961cde5: the Sod case with chem_params%diffusion = 'T' still SIGSEGVs on step 1, and this PR rejects it with chem_params%diffusion requires chemistry = T.

Side effects

  • A case with chem_params%diffusion = 'T' and no chemistry is now rejected at validation. It used to crash on step 1, so no working case changes.
  • The new deallocation guard is unreachable after the validator rule. It only keeps allocate and deallocate symmetric.
  • No solver results change: all 15 chemistry tests and the full suite pass against the existing goldens.

Contribution Policy

All contributions are expected to demonstrate:

  • A clear understanding of the codebase
  • Alignment with product direction
  • Thoughtful reasoning behind changes
  • Evidence of real-world usage or hands-on experience with the problem

If these expectations are not met, we would prefer to implement the changes ourselves rather than spend time reviewing low-effort submissions.

Acknowledgement

  • I confirm this PR meets the above expectations and reflects my own understanding and real-world context.

This PR is from Your Team Name participating in the SC26 Student Cluster Competition.

🤖 Generated with Claude Code

https://claude.ai/code/session_01WPhkY8p1EHhGY7jScizJmC

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
ruru25254649 requested a review from a team as a code owner October 25, 2026 07:04
@ruru25254649
ruru25254649 requested review from sbryngelson and removed request for a team October 25, 2026 07:04
@github-actions

Copy link
Copy Markdown

Claude Code Review

Head SHA: 0ebf1ab

Files changed:

  • 3
  • src/simulation/m_rhs.fpp
  • toolchain/mfc/case_validator.py
  • toolchain/mfc/test_case_validator.py

Findings:
No issues found. Checked in step 4: m_rhs.fpp (the new chemistry guard on the DEALLOCATE matches the allocation condition at line 261), case_validator.py (diffusion and chemistry are defined in check_chemistry), test_case_validator.py.

@ruru25254649
ruru25254649 marked this pull request as draft October 25, 2026 07:19
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.

1 participant