Abort frozen detonation solve instead of returning uninitialized data - #131
Open
djkees wants to merge 1 commit into
Open
Abort frozen detonation solve instead of returning uninitialized data#131djkees wants to merge 1 commit into
djkees wants to merge 1 commit into
Conversation
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
Fix
DetonSolver_solvesilently returning an uninitializedDetonSolutionwhenfrozen=.true.is requested, instead of erroring. Frozen-composition detonation was never implemented (thesolve_frozenprocedure is commented out), but the code only logged an info message and fell through — it never touchedsoln, which besides theconvergedfield (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
detproblem with thefrozenflag set) and from the C/Python bindings — with zero test coverage catching it.Changes
source/detonation.f90: replace thelog_info(...)+ no-op frozen branch inDetonSolver_solvewithcall 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_ENDincea_recovery.c), which converts a Fortranabort()intoierr = CEA_FORTRAN_ABORT. The Python layer already turns that into aRuntimeErrorcarrying the message (CEA.pyx's_check_ierr). No new plumbing was needed — the fix was to actually use what's already there.det+frozenrun now aborts with a clear message instead of writing an output file full of undefined values.frozen=Truenow raises/returnsCEA_FORTRAN_ABORTwith an explicit message, instead of the previousCEA_NOT_CONVERGED(which was misleading — it implied the solver tried and failed to converge, when it never ran at all) alongside garbage solution fields.cea_excel_api.c's detonation wrapper treatsCEA_NOT_CONVERGEDas 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_ABORTisn't covered by that soft-case check, so it now falls through to the existingexcel_catch_cea_errorpath and surfaces the real message instead. The existing Excel test only exercisesfrozen=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: addedtest_detonation_frozen_raises_runtimeerror_and_python_continues, mirroring the existingtest_rocket_abort_raises_runtimeerror_and_python_continues— asserts theRuntimeError, checks bothCEA_FORTRAN_ABORTand 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).cea_core_test.exe, pFUnit,devCMake preset) — 122 tests pass, including the existingdetonation_test.pf::test_deton. Note this is a weaker check than it sounds: the diff only touches theif (frozen_)branch, andtest_detonnever passesfrozen=.true., so it exercises the untouchedelsebranch (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 catchstop 1in-process), which is exactly why the Python test below exists.pip install -e . --no-build-isolation, equivalent tomake py-rebuild) to confirm the.pyx/binding layer picks up the Fortran change correctly.Compatibility / Numerical behavior
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
coreCMake preset (Fortran-only, no C bindings — seeCMakePresets.json) fails to linkcea.exe:fb_utils'sabort()unconditionally referencescea_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 rebuildingcorefrom scratch fails identically, sincemain.f90already callsabort()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.
DetonSolution's type definition directly (onlyconvergedhas a default initializer) and confirming no other code path populatessolnbefore the frozen no-op returns.main.f90'srun_detonation_problemanddeton_output,bindc.F90'scea_detonation_solver_solve), not assumed.abort()already surfaces cleanly through the C/Python layers) was verified by readingcea_recovery.c'sCEA_GUARD_BEGIN/CEA_GUARD_ENDmacros andCEA.pyx's_check_ierr, and by actually running the new and existing abort-recovery tests rather than assuming the pattern would work.cea_excel_api.c's error-classification logic directly, not assumed by analogy with the C/Python path.