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

Simplify Molar Mixture Properties #1837

Merged
merged 11 commits into from
Jan 21, 2025

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Jan 7, 2025

Changes proposed in this pull request

There are some instances where ThermoPhase objects use redundant code for the evaluation of molar mixture properties.

This PR seeks to provide a more uniform approach to evaluating molar properties. By default, average properties should be calculated as a weighted sum, e.g. $\bar{h} = \sum X_k \bar{h}_k$. getPartialMolar<> getters are used to obtain partial molar properties.

  • default behavior for molar mixture properties is changed to summation rather than throwing an exception
  • improve exception handling for any virtual ThermoPhase method that is not implemented
  • redundant/equivalent overrides are removed unless speed advantages are apparent

One caveat is that this approach has limitations for intEnergy_mole and cv_mole. These properties are implemented differently as they are largely based on simplified thermodynamic relationships rather than summations. Interestingly, an attempt to base intEnergy_mole on getPartialMolarIntEnergies causes failures in the zeroD module, albeit only on windows and sundials runners.

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

@ischoegl ischoegl added the Thermo label Jan 7, 2025
@ischoegl ischoegl force-pushed the simplify-molar-thermo-props branch from 70d10e2 to e6d5de4 Compare January 7, 2025 03:40
Copy link

codecov bot commented Jan 7, 2025

Codecov Report

Attention: Patch coverage is 78.16092% with 19 lines in your changes missing coverage. Please review.

Project coverage is 74.39%. Comparing base (2e0d8ac) to head (809181e).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
include/cantera/thermo/ThermoPhase.h 61.36% 17 Missing ⚠️
include/cantera/base/Solution.h 60.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1837      +/-   ##
==========================================
- Coverage   74.41%   74.39%   -0.03%     
==========================================
  Files         382      382              
  Lines       53411    53330      -81     
  Branches     9026     9021       -5     
==========================================
- Hits        39747    39676      -71     
+ Misses      10617    10607      -10     
  Partials     3047     3047              

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

@ischoegl ischoegl force-pushed the simplify-molar-thermo-props branch from e6d5de4 to 987dfbe Compare January 7, 2025 04:30
@ischoegl ischoegl marked this pull request as ready for review January 7, 2025 05:50
@ischoegl ischoegl requested a review from a team January 7, 2025 06:15
@ischoegl ischoegl force-pushed the simplify-molar-thermo-props branch from 40981d9 to 1d4127d Compare January 7, 2025 14:58
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks for this, @ischoegl. My comments here are mostly about the documentation related to these changes.

I'm curious about the test failures when replacing intEnergy_mole and cv_mole that you mentioned cropping up on specific runners.

src/clib/ct.cpp Show resolved Hide resolved
include/cantera/thermo/Phase.h Outdated Show resolved Hide resolved
include/cantera/thermo/ThermoPhase.h Outdated Show resolved Hide resolved
include/cantera/thermo/ThermoPhase.h Outdated Show resolved Hide resolved
@ischoegl
Copy link
Member Author

ischoegl commented Jan 21, 2025

Thanks for the review, @speth. I've taken care of all requests.

I'm curious about the test failures when replacing intEnergy_mole and cv_mole that you mentioned cropping up on specific runners.

While I have deleted those commits, I recall having to change some of the internal plumbing, which was a little more intrusive than what was necessary for h/s/g/cp. It wasn't difficult, but I decided against the changes as I could not troubleshoot - there weren't any errors on macOS.

PS: New CI failures are unrelated to changes in this PR. It appears that boost downloads from JFrog are currently not working, which may or may not resolve. A workaround may involve downloading assets from GH boostorg.

@ischoegl ischoegl force-pushed the simplify-molar-thermo-props branch from ef4f620 to 809181e Compare January 21, 2025 03:10
@ischoegl ischoegl requested a review from speth January 21, 2025 03:55
@ischoegl ischoegl force-pushed the simplify-molar-thermo-props branch from 1e222f2 to 809181e Compare January 21, 2025 13:32
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks, @ischoegl, this looks good to me.

@speth speth merged commit 10d7754 into Cantera:main Jan 21, 2025
87 of 97 checks passed
@ischoegl ischoegl deleted the simplify-molar-thermo-props branch January 21, 2025 22:50
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.

2 participants