Fix mixture entropy/gibbs_energy single-multi scaling mismatches - #132
Open
djkees wants to merge 1 commit into
Open
Fix mixture entropy/gibbs_energy single-multi scaling mismatches#132djkees wants to merge 1 commit into
djkees wants to merge 1 commit into
Conversation
entropy_multi had a stray /1.d3, gibbs_energy_multi's nj was never normalized by sum(weights), and both entropy overloads were missing /std_pressure in their pressure log term. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 726abee)
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
Fixes three related numerical bugs in the single-temperature vs multi-temperature overloads of
Mixture%calc_entropyandMixture%calc_gibbs_energy(source/mixture.f90), so that calling either overload with equivalent physical inputs now gives the same, physically correct result.djkees#66
Changes
mixture_calc_entropy_multidivided its result by an extra1.d3thatmixture_calc_entropy_singledid not (the/1.d3in_singlewas already commented out). Removed the stray division so both overloads scale identically.mixture_calc_gibbs_energy_multi'snjwas never normalized bysum(weights), unlike every other_multiroutine (calc_entropy_multi,calc_enthalpy_multi) and unlikecalc_gibbs_energy_single(built fromcalc_enthalpy/calc_entropy, both of which do normalize). Added the missing/total_weightnormalization.mixture_test.pf: bothcalc_entropyoverloads computelog(pressure/...)using the rawpressureargument directly, without dividing bystd_pressure(the 1 bar reference pressure) first.calc_gibbs_energy_multialready does this correctly (pr = pressures(j)/std_pressure), but neithercalc_entropyoverload did. Without this fix, both overloads agree with each other but disagree with the correct physical value. Added/std_pressureto the pressure term in both overloads.@assertRelativelyEqualassertions inmixture_test.pf'stest_calc_properties(calc_gibbs_energyandcalc_entropy, both the single- and multi-temperature call forms) — they've been commented out since the initial public release, and now pass with the fix.Testing
ctest/pFUnit locally: pFUnit's own CMake (vendored via GFE) unconditionally excludesRegexFilter.F90on Windows (if(NOT WIN32)inpFUnit/src/funit/core/CMakeLists.txt) whileFUnit.F90's main driver unconditionally doesuse pf_RegexFilter, so pFUnit itself fails to build on this Windows/MinGW dev environment regardless of this change. CI'spfunit-testsjob builds pFUnit on Ubuntu and should exercisemixture_test.pfandequilibrium_test.pf(includingtest_sv_derivatives, which uses the now-correctedcalc_entropyto build an initial-guess constraint value for an entropy/volume-constrained solve) — please confirm CI is green before merging, especially that test.cea_corefrom this branch and linked a standalone Fortran driver againstlibcea_core.athat replicatestest_calc_properties' exact inputs (RP-1311 Example Case 14 mixture: H2O/O2/H2O(L), T=304K, P=5066.25 Pa). Bothcalc_entropyoverloads now produce9463.0131180265817and bothcalc_gibbs_energyoverloads now produce-14830015.9..., matching the reference values9.46301312d+03and-1.48300159E+07inmixture_test.pfto 10 significant figures, and matching each other (single vs. multi) exactly to solver tolerance.Compatibility / Numerical behavior
This intentionally changes the numerical output of
Mixture%calc_entropyandMixture%calc_gibbs_energy(both single- and multi-temperature overloads). Per the audit trail above: (1) neither the_multioverloads (which were never exercised by any real caller — see issue #66) norcalc_entropy_single/calc_gibbs_energy_single(used internally bycalc_gibbs_energy_singleitself, but not otherwise called in production code outside of this module and one test) had validated numerical behavior to preserve; the only reference data available (mixture_test.pf's RP-1311-sourced expected values) was disabled specifically because these functions didn't produce correct output. (2) The one live caller ofcalc_entropyoutside this module,equilibrium_test.pf'stest_sv_derivatives, uses it only to construct an initial-guess entropy value fed into a self-consistent constant-entropy/volume equilibrium solve, not to check against a hardcoded expected value — this should remain valid under the corrected calculation, but I could not confirm convergence locally (see Testing above).Drafted with Claude's assistance
source/mixture.f90:833-967, cross-checked against the normalization pattern used consistently by every other_multi/_singlepair in the same file, and againstcalc_gibbs_energy_multi's existing (correct)std_pressurehandling.cea_corefrom this branch and linking a standalone driver program against it that reproducesmixture_test.pf'stest_calc_propertiesinputs exactly; results matched the RP-1311 reference values to 10 significant figures for both the single- and multi-temperature overloads, both before/after comparison for each fix and cross-checked against a hand-derived formula (S = (H-G)/T using the already-correctcalc_gibbs_energy_multi).Update
The pFUnit-on-Windows build issue referenced in Testing above (tracked as #163) now has a fix: djkees#167. With that fix,
cea_core_testwas built and run locally on this branch for the first time:ctest -R cea_core_test -V— 124/124 tests passed, includingmixture_test_suite.test_calc_properties(the re-enabledcalc_entropy/calc_gibbs_energyassertions this PR's fix targets) andequilibrium_test_suite.test_sv_derivatives(the one live caller ofcalc_entropyoutside this module, flagged above as unconfirmed) — both green.Also worth noting: this fork's GitHub Actions checks don't appear to run at all (
gh pr checksreports nothing on this PR despite.github/workflows/basic_build.ymlexisting), so the local run above is currently the only automated test evidence for this PR beyond manual review.