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

Implement serialization of Peng-Robinson phases #1180

Merged
merged 6 commits into from
Feb 8, 2022

Conversation

speth
Copy link
Member

@speth speth commented Jan 20, 2022

Changes proposed in this pull request

  • Implement serialization of Peng-Robinson phases
  • Update Redlich-Kwong serialization to save critical parameters for species where those were provided instead of R-K specific parameters
  • Some formatting cleanup of the PengRobinson class
  • Fix incorrect acentric factors for O2 and N2 in critical-properties.yaml

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

import cantera as ct
p = ct.Solution('test/data/thermo-models.yaml', 'CO2-PR')
print(p.write_yaml(skip_user_defined=True))
...
species:
  - name: CO2
    composition: {C: 1.0, O: 2.0}
    thermo:
      model: NASA7
      temperature-ranges: [200.0, 1000.0, 3500.0]
      data:
        - [2.35677352, 8.98459677e-03, -7.12356269e-06, 2.45919022e-09,
        -1.43699548e-13, -4.83719697e+04, 9.90105222]
        - [3.85746029, 4.41437026e-03, -2.21481404e-06, 5.23490188e-10,
        -4.72084164e-14, -4.8759166e+04, 2.27163806]
    equation-of-state:
      model: Peng-Robinson
      a: 3.958134e+05
      b: 0.0266275
      acentric-factor: 0.228
      binary-a:
        H2O: 7.897e+06
...

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 Jan 20, 2022

Codecov Report

Merging #1180 (0d4cef3) into main (fcff592) will increase coverage by 0.05%.
The diff coverage is 96.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1180      +/-   ##
==========================================
+ Coverage   65.35%   65.40%   +0.05%     
==========================================
  Files         315      318       +3     
  Lines       45999    46178     +179     
  Branches    19531    19639     +108     
==========================================
+ Hits        30062    30203     +141     
- Misses      13445    13470      +25     
- Partials     2492     2505      +13     
Impacted Files Coverage Δ
include/cantera/thermo/MixtureFugacityTP.h 0.00% <ø> (ø)
include/cantera/thermo/RedlichKwongMFTP.h 100.00% <ø> (ø)
include/cantera/thermo/ThermoPhase.h 30.57% <ø> (ø)
src/thermo/MixtureFugacityTP.cpp 37.50% <ø> (+0.13%) ⬆️
src/thermo/RedlichKwongMFTP.cpp 71.18% <88.88%> (+0.66%) ⬆️
src/thermo/PengRobinson.cpp 85.62% <98.73%> (+0.79%) ⬆️
include/cantera/thermo/PengRobinson.h 100.00% <100.00%> (ø)
src/transport/TransportBase.cpp 25.00% <0.00%> (-6.71%) ⬇️
src/base/Solution.cpp 79.51% <0.00%> (-5.53%) ⬇️
include/cantera/kinetics/Kinetics.h 36.52% <0.00%> (-2.28%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fcff592...0d4cef3. Read the comment docs.

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 so much, @speth, both for the new capabilities and the significant cleanup of PengRobinson.

Everything looks good to me. I haven't downloaded and run installation to test, figuring that the test coverage would catch issues, but LMK if you'd like me to take that step.

No "real" changes below, only two comment lines that got moved that I think we'd probably be fine deleting.

include/cantera/thermo/MixtureFugacityTP.h Show resolved Hide resolved
src/thermo/PengRobinson.cpp Outdated Show resolved Hide resolved
src/thermo/PengRobinson.cpp Outdated Show resolved Hide resolved
src/thermo/RedlichKwongMFTP.cpp Show resolved Hide resolved
@bryanwweber
Copy link
Member

@decaluwe Is this good to merge?

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.

LGTM - thanks, @speth

@bryanwweber bryanwweber merged commit 8c3a1b5 into Cantera:main Feb 8, 2022
@speth speth deleted the serialize-PengRobinson branch February 8, 2022 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants