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 of IAPWS water EoS #1394

Merged
merged 2 commits into from
Sep 25, 2022
Merged

Fix consistency of IAPWS water EoS #1394

merged 2 commits into from
Sep 25, 2022

Conversation

speth
Copy link
Member

@speth speth commented Sep 25, 2022

Changes proposed in this pull request

  • Fix dimensionalization of internal energy in WaterSSTP (was off by a factor of $T$)
  • Correct conversion of nondimensional IAPWS equation of state to always be based on converting to a mass basis using the original defined value of $R$, then convert to a molar using the nominal molecular weight. This is the approach recommended by the IAPWS95 documentation.

A number of regression test values have changed, generally by factors on the order of $18.015268/18.015\approx 1.0000148$ which is the ratio of the molecular weight for water corresponding to the IAPWS95 value for $R$ and Cantera's molecular weight for water.

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

Closes #1315

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

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

@bryanwweber
Copy link
Member

bryanwweber commented Sep 25, 2022

I have no idea what's causing the Windows 2019 failures, that wouldn't also be the same on Windows 2022. Setuptools did release a new version today that updates distutils, but that doesn't seem to be fixed by capping the version of setuptools in pyproject.toml. Anyhow, #1392 (although currently DO NOT MERGE) enables running everything from Windows 2022 by fixing SCons/scons#3664, and all the builds on Windows 2022 succeed, so 🤷

I was trying to test over in #1392 what happens when SCons is < 4.4.0 and you have the msvc_toolset_version variable present, but I'm getting a shell error or something, so the failures are unrelated.

Also, the example failure is due to a new warning from Matplotlib 3.6.0, and us setting the environment variable to treat warnings as errors. I fixed that in #991 by installing Matplotlib <3.6.0, but it may be worth restricting the environment variable to only treat Cantera-generated warnings as errors.

I'm also very curious what you're working on that this was a priority to fix 😄 Unless it's just a general "well, I see how to fix this, so might as well" 😂

@codecov
Copy link

codecov bot commented Sep 25, 2022

Codecov Report

Merging #1394 (64225fa) into main (5e15f57) will decrease coverage by 0.05%.
The diff coverage is 74.32%.

@@            Coverage Diff             @@
##             main    #1394      +/-   ##
==========================================
- Coverage   71.08%   71.03%   -0.06%     
==========================================
  Files         363      363              
  Lines       51839    51855      +16     
  Branches    17354    17362       +8     
==========================================
- Hits        36849    36834      -15     
- Misses      12645    12676      +31     
  Partials     2345     2345              
Impacted Files Coverage Δ
include/cantera/thermo/WaterPropsIAPWS.h 100.00% <ø> (ø)
src/thermo/WaterPropsIAPWS.cpp 35.39% <65.85%> (-5.68%) ⬇️
src/thermo/PDSS_Water.cpp 72.26% <76.19%> (-0.28%) ⬇️
src/thermo/WaterSSTP.cpp 80.37% <100.00%> (+0.09%) ⬆️

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

@speth
Copy link
Member Author

speth commented Sep 25, 2022

I'm also very curious what you're working on that this was a priority to fix 😄 Unless it's just a general "well, I see how to fix this, so might as well" 😂

This isn't impacting anything specific I'm working on. Mostly just trying to make a little progress on the pile of bugs that were uncovered by #1299. I thought this was low-hanging fruit after figuring out the first one (scaling by R instead of RT), and then realized there were still some inexplicable discrepancies which were trickier to hunt down.

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 taking on some of the bugs. The code changes make sense to me (caveat: I have not reviewed underlying equations and am relying on the thermodynamics-inspired tests that uncovered the bug in the first place); I only have minor comments.

src/thermo/WaterPropsIAPWS.cpp Outdated Show resolved Hide resolved
src/thermo/WaterPropsIAPWS.cpp Show resolved Hide resolved
@ischoegl
Copy link
Member

ischoegl commented Sep 25, 2022

I have no idea what's causing the Windows 2019 failures, that wouldn't also be the same on Windows 2022. […]

The windows 2022 runner works a little different, as it builds in a conda environment.

@ischoegl
Copy link
Member

ischoegl commented Sep 25, 2022

Also, the example failure is due to a new warning from Matplotlib 3.6.0, and us setting the environment variable to treat warnings as errors. I fixed that in #991 by installing Matplotlib <3.6.0, but it may be worth restricting the environment variable to only treat Cantera-generated warnings as errors.

One other alternative would be to create a pytest runner that handles the examples, so we get access to the warnings filters.

PS: neither of the issues are related to the bug fix in this PR …

The nondimensional EOS should always be dimensionalized to a mass
basis using the value of R given in the original source. Values on
a molar basis can then be computed consistently using the best available
value for the molecular weight of water.

Partially resolves Cantera#1315
@speth
Copy link
Member Author

speth commented Sep 25, 2022

I'm going to pass on trying to fix the pre-existing CI failures as part of this PR.

I briefly tried adjusting the PYTHONWARNINGS environment variable, and couldn't figure out how to get it to actually catch warnings from a specific module. I think PYTHONWARNINGS=error:::cantera should have been the right thing, but it didn't do anything when I deliberately triggered a warning from Cantera.

@speth speth merged commit f6744ec into Cantera:main Sep 25, 2022
ischoegl added a commit to ischoegl/cantera that referenced this pull request Sep 27, 2022
ischoegl added a commit to ischoegl/cantera that referenced this pull request Sep 27, 2022
@ischoegl ischoegl mentioned this pull request Sep 27, 2022
@speth speth mentioned this pull request Sep 28, 2022
5 tasks
speth pushed a commit that referenced this pull request Sep 29, 2022
c-randall pushed a commit to c-randall/cantera that referenced this pull request Oct 7, 2022
jongyoonbae pushed a commit to jongyoonbae/cantera that referenced this pull request Feb 2, 2023
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.

Incorrect implementation of internal energy in WaterSSTP (liquid-water-IAPWS95)
3 participants