Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix consistency issues with SurfPhase and RedlichKisterVPSSTP #1449

Merged
merged 11 commits into from
Mar 4, 2023

Conversation

speth
Copy link
Member

@speth speth commented Mar 4, 2023

Changes proposed in this pull request

  • Fix implementation of SurfPhase::partialMolarEntropies to include concentration term
  • Fix SurfPhase::getChemPotentials to avoid returning inf
  • Fix calculations of enthalpy, entropy, and specific heat capacities in RedlichKisterVPSSTP
  • Add a consistency test case of RedlichKisterVPSSTP that includes "excess entropy" terms and more than just a binary solution.
  • Clean up documentation and implementation of RedlichKisterVPSSTP
  • Fix handling of density and molar-density options in equation-of-state fields for phases that use PDSS_ConstVol.

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

Fixes #1313
Fixes #1314
Fixes #1320

If applicable, provide an example illustrating new features this pull request is introducing

Plots corresponding to those in #1320 with updated results:
enthalpies-new

entropies-new

entropy-T-new

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
  • The pull request is ready for review

@speth speth added the Thermo label Mar 4, 2023
@codecov
Copy link

codecov bot commented Mar 4, 2023

Codecov Report

Merging #1449 (67acd7e) into main (c54e429) will increase coverage by 0.00%.
The diff coverage is 85.00%.

@@           Coverage Diff           @@
##             main    #1449   +/-   ##
=======================================
  Coverage   70.94%   70.94%           
=======================================
  Files         369      369           
  Lines       55259    55262    +3     
  Branches    18207    18213    +6     
=======================================
+ Hits        39201    39207    +6     
+ Misses      13593    13591    -2     
+ Partials     2465     2464    -1     
Impacted Files Coverage Δ
include/cantera/thermo/GibbsExcessVPSSTP.h 0.00% <ø> (ø)
include/cantera/thermo/RedlichKisterVPSSTP.h 100.00% <ø> (ø)
src/thermo/GibbsExcessVPSSTP.cpp 72.58% <33.33%> (-5.76%) ⬇️
src/thermo/RedlichKisterVPSSTP.cpp 71.06% <83.33%> (+1.06%) ⬆️
src/thermo/PDSS_ConstVol.cpp 77.19% <100.00%> (+3.72%) ⬆️
src/thermo/SurfPhase.cpp 87.86% <100.00%> (+0.11%) ⬆️
src/thermo/BinarySolutionTabulatedThermo.cpp 87.96% <0.00%> (-1.01%) ⬇️
include/cantera/base/ValueCache.h 100.00% <0.00%> (ø)
src/thermo/IdealSolidSolnPhase.cpp 84.01% <0.00%> (+0.43%) ⬆️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

PDSS_ConstVol now converts 'density' and 'molar-density' fields from
the 'equation-of-state' field, making it consistent with other phases
that support these options.
Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@speth ... thanks for the bug fixes. As in #1442, these changes are covered by unit tests, so I am approving (the plots further illustrate that things work as expected).

As an aside, one comment I have about consistency checks is that it may be worthwhile separating them from the test-thermo gtest runner, as the volume of output makes it hard to check on other thermo tests.

@speth speth merged commit 7f04d83 into Cantera:main Mar 4, 2023
@speth speth deleted the fix-1313 branch March 4, 2023 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
2 participants