Skip to content

Update to support CoolPropv8 - #1815

Open
StephenBurroughs wants to merge 5 commits into
IDAES:mainfrom
waikato-ahuora-smart-energy-systems:coolpropv8
Open

Update to support CoolPropv8#1815
StephenBurroughs wants to merge 5 commits into
IDAES:mainfrom
waikato-ahuora-smart-energy-systems:coolpropv8

Conversation

@StephenBurroughs

Copy link
Copy Markdown
Contributor

Fixes

Updates coolprop wrapper tests to expect new assertion error for missing components.

This doesn't fix the compatability test, as it's in a separate repository. I have opened a pull request in that repository to update the tolerance of the haber bosch test which should fix it: https://github.com/IDAES/idaes-compatibility/pull/13

Summary/Motivation:

Coolprop has updated to v8, the only breaking change is that it now returns a different error.

Changes proposed in this PR:

-Fix test

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@StephenBurroughs StephenBurroughs changed the title updated coolprop wrapper tests for coolpropv8 Update to support CoolPropv8 Jul 1, 2026
@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.84%. Comparing base (4275c45) to head (e30b353).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1815      +/-   ##
==========================================
- Coverage   73.84%   73.84%   -0.01%     
==========================================
  Files         422      422              
  Lines       67509    67509              
  Branches    11417    11417              
==========================================
- Hits        49853    49852       -1     
- Misses      15039    15040       +1     
  Partials     2617     2617              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sufikaur

sufikaur commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Thank you for doing this!
I have temporarily restricted the version from pyproject.toml to be coolprop <8. You may want to override that in this PR, and when you merge into main can also merge in the req without the temporary "<8"

@StephenBurroughs

Copy link
Copy Markdown
Contributor Author

@ksbeattie I've made two changes here. One is to add a check for the version of CoolProp within the CoolPropWrapper test, then check that the thrown error is appropriate based on the version. This should be fine to go in as is given that it has no other change, although the compatibility repository will still need to have that PR merged to make that pass.
There's a second commit here that makes some additional changes as well. It seems that CoolPropV8 returns slightly different results for the BT PR tests in the CoolPropWrapper test. I'm not sure why this wasn't flagged previously during the GitHub test run, but it seems to be a consistent point of failure on my machine. I'm not exactly sure why this is happening (it's possible that the cubic EOS within CoolProp has been slightly modified, but I haven't had the time to do a deep dive yet and am not sure when that might happen) but it doesn't push things out by a large margin, so I've updated some of the tolerances within the relevant tests to account for this.

I'm happy to rollback that change and just go with the error message one if that's preferable for now, but it does raise a bit of conundrum...
If it's preferable to keep the tolerances as is, then it probably means that the change to the cubic EOS needs to be tracked down and updated. Given that the Benzene/Toluene formulations use the CoolPropWrapper to load data, it seems likely that the divergence is then in the IDAES/CoolProp PR formulations themselves. The challenge here is that if the IDAES PR formulation is updated, then it will pass with CoolPropV8 but fail with CoolPropV7 - basically the same issue but inversed.

Given that it seems impractical to have a case-based formulation and the results are still actually quite accurate (tolerance of 1e-6 vs 1e-4), it seems like updating the tolerances is probably the best course of action...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Priority:Normal Normal Priority Issue or PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants