Skip to content

Make MixtureFugacityTP::cubicSolver more robust#2141

Open
ischoegl wants to merge 1 commit into
Cantera:mainfrom
ischoegl:issue-1157
Open

Make MixtureFugacityTP::cubicSolver more robust#2141
ischoegl wants to merge 1 commit into
Cantera:mainfrom
ischoegl:issue-1157

Conversation

@ischoegl

@ischoegl ischoegl commented Jun 21, 2026

Copy link
Copy Markdown
Member

Changes proposed in this pull request

  • Make cubic-EOS triple-root detection independent of compiler-specific floating-point rounding.
  • Use cbrt for signed cube roots and correct validation of the negative-root branch.
  • Re-enable the cubic solver test on MinGW and verify all roots coincide at the critical point.

If applicable, fill in the issue number this pull request is fixing

Closes #1157

Extensive use of generative AI.
Significant portions of code or documentation were generated with AI, including logic and implementation decisions. All generated code and documentation were reviewed and understood by the contributor. Specifically, OpenAI Codex (gpt-5.5) was used to diagnose and fix the issue.

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • AI Statement is included
  • The pull request is ready for review

@codecov

codecov Bot commented Jun 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.06%. Comparing base (a6a5f5e) to head (22fe8a2).

Files with missing lines Patch % Lines
src/thermo/MixtureFugacityTP.cpp 95.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2141      +/-   ##
==========================================
+ Coverage   78.02%   78.06%   +0.03%     
==========================================
  Files         452      452              
  Lines       54104    54115      +11     
  Branches     9063     9065       +2     
==========================================
+ Hits        42213    42243      +30     
+ Misses       8853     8835      -18     
+ Partials     3038     3037       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ischoegl ischoegl marked this pull request as ready for review June 21, 2026 13:50
@ischoegl ischoegl requested a review from a team June 21, 2026 13:59
Comment thread src/thermo/MixtureFugacityTP.cpp Outdated
Comment thread src/thermo/MixtureFugacityTP.cpp
@speth

speth commented Jun 23, 2026

Copy link
Copy Markdown
Member

I'm not quite sure what behavior we're trying to ensure with the test in question. There are bound to be threshold conditions in any calculation that maps a floating point values to a discrete set. The most that you can expect is that the threshold value where it flips between different outputs is close (i.e. small multiple of machine precision) for when using different compilers / systems.

The test was (apparently) getting past the relative comparison EXPECT_NEAR(expected_result[2], Vroot[0], 1.e-6); and only "failing" on whether decided the two solutions were on the liquid branch or the gas branch. Of course, at exactly the critical point (or as close as one can get with floating point), the failing check EXPECT_NEAR(nSolnValues, -2, 1.e-6) is arguably not correct in any case -- there is only one distinct root in this case, so I'd expect that this should either return 1 or 3 depending on whether the expectation is to count distinct or total real roots, though that perhaps also depends on how this return value is meant to be used.

Creating a radius around the critical point that effectively snaps to this triple root just moves the threshold condition to some set of points near but not quite at the critical point.

I do agree that the change to use cbrt over pow makes sense, not only for its handling of the case where the operand is negative but also because it is generally more accurate and faster.

@ischoegl

ischoegl commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

Thanks, @speth!

[...] Of course, at exactly the critical point (or as close as one can get with floating point), the failing check EXPECT_NEAR(nSolnValues, -2, 1.e-6) is arguably not correct in any case -- there is only one distinct root in this case, so I'd expect that this should either return 1 or 3 depending on whether the expectation is to count distinct or total real roots, though that perhaps also depends on how this return value is meant to be used.

I agree with this interpretation. The MinGW failure exposed that the old test was asserting a liquid/gas branch sign at the critical point, where that distinction is degenerate. I’ve updated the fix so the detected triple-root case returns 1 rather than 2 or -2. At Tc, Pc, the cubic has one distinct real root with multiplicity three, so 1 seems like the clearest convention for the single usable thermodynamic state. I also changed the test to assert that convention and to check the critical molar volume directly, rather than checking a branch sign.

I do agree that the change to use cbrt over pow makes sense, not only for its handling of the case where the operand is negative but also because it is generally more accurate and faster.

I kept the cbrt change since it fixes the negative-operand path and is the more appropriate operation here.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MixtureFugacityTP::solveCubic finds wrong root on mingw

3 participants