Skip to content

Abort frozen detonation solve instead of returning uninitialized data - #1

Merged
djkees merged 1 commit into
mainfrom
fix/detonation-frozen-not-implemented
Sep 1, 2026
Merged

Abort frozen detonation solve instead of returning uninitialized data#1
djkees merged 1 commit into
mainfrom
fix/detonation-frozen-not-implemented

Conversation

@djkees

@djkees djkees commented Aug 29, 2026

Copy link
Copy Markdown
Owner

Summary

Fix DetonSolver_solve silently returning an uninitialized DetonSolution when frozen=.true. is requested, instead of erroring. Frozen-composition detonation was never implemented (the solve_frozen procedure is commented out), but the code only logged an info message and fell through — it never touched soln, which besides the converged field (defaulted .false.) has no default initializers, so every other field (pressure, mach, gamma, P1, M1, ...) held whatever was already in that memory.

This is reachable today from the CLI (a det problem with the frozen flag set) and from the C/Python bindings — with zero test coverage catching it.

Changes

  • source/detonation.f90: replace the log_info(...) + no-op frozen branch in DetonSolver_solve with call abort('DetonSolver: frozen composition not supported yet'). This matches the existing "not yet supported" convention used elsewhere in the codebase (e.g. main.f90's "Entropy calculation from reactants not yet supported"), and reuses the recovery machinery that's already wired up for every C API call (CEA_GUARD_BEGIN/CEA_GUARD_END in cea_recovery.c), which converts a Fortran abort() into ierr = CEA_FORTRAN_ABORT. The Python layer already turns that into a RuntimeError carrying the message (CEA.pyx's _check_ierr). No new plumbing was needed — the fix was to actually use what's already there.
    • CLI impact: a det+frozen run now aborts with a clear message instead of writing an output file full of undefined values.
    • C/Python impact: frozen=True now raises/returns CEA_FORTRAN_ABORT with an explicit message, instead of the previous CEA_NOT_CONVERGED (which was misleading — it implied the solver tried and failed to converge, when it never ran at all) alongside garbage solution fields.
    • Excel impact (no code change, verified by tracing the call path): cea_excel_api.c's detonation wrapper treats CEA_NOT_CONVERGED as a soft/"warning" case and deliberately continues on to read solution properties into the output cells. That means the old bug was actually worse for Excel specifically — it wouldn't just report a misleading status, it would write garbage values into the user's spreadsheet labeled only "CEA solve warning 8." CEA_FORTRAN_ABORT isn't covered by that soft-case check, so it now falls through to the existing excel_catch_cea_error path and surfaces the real message instead. The existing Excel test only exercises frozen=0, so this path wasn't previously verified either way; no new Excel-level test added here (kept out of scope), but worth a follow-up if that binding gets test attention.
  • source/bind/python/tests/test_abort_recovery.py: added test_detonation_frozen_raises_runtimeerror_and_python_continues, mirroring the existing test_rocket_abort_raises_runtimeerror_and_python_continues — asserts the RuntimeError, checks both CEA_FORTRAN_ABORT and the specific message text, and confirms the Python process survives the abort.

No changes to the non-frozen (equilibrium) detonation solve path, and no changes to any solver algorithm, convergence logic, or numerical behavior.

Testing

  • pytest source/bind/python/tests (100 tests) — all pass, including the two abort-recovery tests (rocket + new detonation one).
  • Fortran unit tests (cea_core_test.exe, pFUnit, dev CMake preset) — 122 tests pass, including the existing detonation_test.pf::test_deton. Note this is a weaker check than it sounds: the diff only touches the if (frozen_) branch, and test_deton never passes frozen=.true., so it exercises the untouched else branch (solve_eq) either way — it confirms the file still compiles and runs, not that the frozen path behaves correctly. There's no Fortran-level test for the frozen/abort path itself (pFUnit has no established way to catch stop 1 in-process), which is exactly why the Python test below exists.
  • Rebuilt the editable Python install from scratch (pip install -e . --no-build-isolation, equivalent to make py-rebuild) to confirm the .pyx/binding layer picks up the Fortran change correctly.

Compatibility / Numerical behavior

  • No expected changes to numerical results
  • Expected changes (explain and provide validation)

The non-frozen (equilibrium) detonation solve path (DetonSolver_solve_eq) is untouched. The frozen path never produced valid results before this change (it returned uninitialized memory), so there is no prior valid numerical behavior to preserve — this converts a silent, undefined-behavior failure into a clean, explicit error.

Note on a pre-existing, unrelated finding

While verifying the fix, I found that the core CMake preset (Fortran-only, no C bindings — see CMakePresets.json) fails to link cea.exe: fb_utils's abort() unconditionally references cea_bindc_abort_recovery_is_active/cea_bindc_abort_recover, which are only defined in the C-bindings sources (cea_abort_support.c). I confirmed this is pre-existing and not introduced by this change — reverting my edit and rebuilding core from scratch fails identically, since main.f90 already calls abort() elsewhere (e.g. the reactant-entropy "not yet supported" case) regardless of my change. Not fixed here; flagging separately as it's out of scope for this PR.


Drafted with Claude's assistance.

  • The uninitialized-memory claim was verified by reading DetonSolution's type definition directly (only converged has a default initializer) and confirming no other code path populates soln before the frozen no-op returns.
  • CLI/binding reachability was traced through actual call sites (main.f90's run_detonation_problem and deton_output, bindc.F90's cea_detonation_solver_solve), not assumed.
  • The recovery-mechanism claim (that abort() already surfaces cleanly through the C/Python layers) was verified by reading cea_recovery.c's CEA_GUARD_BEGIN/CEA_GUARD_END macros and CEA.pyx's _check_ierr, and by actually running the new and existing abort-recovery tests rather than assuming the pattern would work.
  • The Excel-impact claim was verified the same way — reading cea_excel_api.c's error-classification logic directly, not assumed by analogy with the C/Python path.
  • All test results above (100 Python tests, 122 Fortran tests) were from real local builds and runs, not predicted.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant