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

Peng-Robinson EoS: added setState_DP #1847

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

TJP-Karpowski
Copy link

@TJP-Karpowski TJP-Karpowski commented Feb 3, 2025

Changes proposed in this pull request

The Peng-Robinson EoS does not implement a DP update, which is added by this merge request.
The setState_DP function was extended with a Tguess value to enable the efficient search for the correct temperature.
The downstream application requires the SolutionBranch of the cubic EoS to be settable; thus, the API is exposed on the ThermoPhase level.

  • added setState_DP for Peng-Robinson EoS
  • added Tguess for the setState_DP function
  • the setForcedSolutionBranch is exposed on the ThermoPhase level

TODO

  • Add test of functionality to the Peng-Robinson Testset.

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

Copy link

codecov bot commented Feb 3, 2025

Codecov Report

Attention: Patch coverage is 87.03704% with 7 lines in your changes missing coverage. Please review.

Project coverage is 74.42%. Comparing base (e6f3e9d) to head (5249dce).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/thermo/PengRobinson.cpp 90.00% 2 Missing and 3 partials ⚠️
include/cantera/thermo/ThermoPhase.h 50.00% 1 Missing ⚠️
src/thermo/MixtureFugacityTP.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1847      +/-   ##
==========================================
+ Coverage   74.41%   74.42%   +0.01%     
==========================================
  Files         386      386              
  Lines       53628    53681      +53     
  Branches     9063     9075      +12     
==========================================
+ Hits        39905    39950      +45     
- Misses      10652    10656       +4     
- Partials     3071     3075       +4     

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

@ischoegl
Copy link
Member

ischoegl commented Feb 16, 2025

Hi @TJP-Karpowski ... thank you for the PR!

While I believe that your PR targets a feature we'd like to see, there are some aspects that you should consider. Most people will use the setState_DP class method from the Python API, - via DPX/DPY, - which currently does not support the optional parameter. Also, from an API design aspect, the third parameter isn't consistent with what's used for other comparable setters, all of which use tol as a third parameter, e.g.:

void setState_HP(double h, double p, double tol=1e-9);

Can you think of a way to provide an automatic initial guess instead? As an aside, it would be interesting to compare to CoolProp directly in the Python test suite - it's relatively straightforward to include optional dependencies for testing purposes.

@TJP-Karpowski
Copy link
Author

Hi @ischoegl, thank you for the comment.

I understand the issue. As short primer my usecase: I have Cantera embedded in a CFD solver doing the thermo-update, Looping over each cell and updating the properties. So I know the temperature from the last timestep which is already a very good guess.
Before I try to adapt it, could you give feedback on which solution you prefer?

Solutions:

  1. Is there an option to set an object's temperature without triggering the calculation of other properties? If so, I could set the Temperature of the object (without updating) and then use the Temperature in the setState_DP calculation.
    While this is a solution for an unknowing user, this is very undefined behavior, as the setState_DP might work or fail depending on the currently set Temperature. However, I could add it to the error statements.

  2. I could add a guess State parameter (T,P, Rho) that stores estimates that can be updated separately and used when needed. This has the same issue as the calculation convergence dependence on a hidden state, but it could be documented in the call function.

  3. I think the easiest and clearest solution would be to add a fourth TGuess parameter to the C++ API and also to the Python bindings, as this would make the calculation independent of the thermo-object's current state.

I will definitely add the tolerance (tol) as the third parameter to the call, as it is currently hard coded.
Which solution would you prefer?

Kind Regards

@ischoegl
Copy link
Member

ischoegl commented Feb 27, 2025

@TJP-Karpowski ... thanks for providing the context: the use case is definitely warranting a guess to be provided.

I agree that option (3) is the cleanest solution. Leaving things in an unphysical state is a dangerous proposition, especially for APIs that may be used by inexperienced users.

TJP-Karpowski and others added 3 commits February 28, 2025 11:20
@ischoegl
Copy link
Member

ischoegl commented Mar 23, 2025

Hi @TJP-Karpowski ... thanks for addressing. Before further commenting, I'd like to ask you to rebase your PR to the current main rather than introducing a merge commit as in 0b679cb. Please let us know in case you should need assistance.

Edit: please have a look at the Python code as well - please run scons build and scons test with the Python interface enabled, which should ensure that the submitted changes compile.

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