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 Redlich-Kwong partial molar properties #1218

Merged
merged 2 commits into from
Mar 18, 2022

Conversation

speth
Copy link
Member

@speth speth commented Mar 14, 2022

Changes proposed in this pull request

  • Remove incorrect implementation of RedlichKwongMFTP::getPartialMolarCp
  • Correct implementation of RedlichKwongMFTP::getPartialMolarIntEnergies

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

Closes #998

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

@codecov
Copy link

codecov bot commented Mar 14, 2022

Codecov Report

Merging #1218 (c72f095) into main (719275e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1218   +/-   ##
=======================================
  Coverage   65.43%   65.44%           
=======================================
  Files         320      320           
  Lines       46249    46250    +1     
  Branches    19657    19659    +2     
=======================================
+ Hits        30265    30267    +2     
+ Misses      13454    13453    -1     
  Partials     2530     2530           
Impacted Files Coverage Δ
include/cantera/thermo/RedlichKwongMFTP.h 100.00% <100.00%> (ø)
src/thermo/RedlichKwongMFTP.cpp 71.65% <100.00%> (+0.46%) ⬆️
src/thermo/MixtureFugacityTP.cpp 37.03% <0.00%> (-0.47%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@decaluwe decaluwe self-requested a review March 15, 2022 01:08
decaluwe
decaluwe previously approved these changes Mar 15, 2022
Copy link
Member

@decaluwe decaluwe 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 tackling this one, @speth. It looks good to me.

Is the idea that hopefully some day we will remove the check_cp flag from the checkThermo definition? I think it's the right call, right now, but does seem odd that just that specific property is excepted (and I certainly wouldn't want it viewed as a precedent for anyone who just doesn't feel like implementing a full thermo feature set, but I think that's easy enough for us to head off).

getPartialMolarEnthalpies(ubar);
double p = pressure();
for (size_t k = 0; k < nSpecies(); k++) {
ubar[k] -= p * m_partialMolarVolumes[k];
Copy link
Member

Choose a reason for hiding this comment

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

Nice! I was fearing some long derivation on which I'd have to go and run due diligence to check 😂

@speth
Copy link
Member Author

speth commented Mar 15, 2022

Ah, that's an interesting point -- as I had it, it would have probably gone unnoticed to remove the check_cp=False settings from those tests if getPartialMolarCp was later implemented, so I changed it to test that an exception is raised when check_cp=False.

I'm not necessarily worried about thermo models not providing a "complete" interface. I'd be more interested in defining what the minimal interface that a model needs to provide is, say to be compatible with the reactor network or 1D flame models. As it is, I don't know when you actually need to know the partial molar specific heats -- they appear in the 1D flame for the ideal gas case, but end up being replaced by the partial molar enthalpies in the non-ideal formulation (according to #1079).

@decaluwe
Copy link
Member

Ah, good point - the partial molar enthalpies being the generally correct approach, I would be slightly curious to see if there is a speed difference, or if we could just replace the partial molar specific heats in the ideal gas reactor with partial molar enthalpies, and be no worse for it.

IIRC, one of J. Shepherd's students originally wrote asking for access to these partial molar entities, for use with non-ideal phases in SD-ToolBox.

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.

@decaluwe ... is this ready to be merged? If not, just dismiss my approval.

@decaluwe decaluwe merged commit 0364d89 into Cantera:main Mar 18, 2022
@speth speth deleted the fix-998-redlich-kwong branch July 23, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redlich-Kwong implementations of getPartialMolarIntEnergies and getPartialMolarCp are incorrect
3 participants