-
Notifications
You must be signed in to change notification settings - Fork 21
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
Replace CoolProp #232
Replace CoolProp #232
Conversation
The fluids are implemented as derived objects and have double inheritance from a Given that |
@j-c-cook no, we're not going to do that. The test values came from there, but I'm not going to introduce that dependency into our testing. As I said, CP development has been waning, so relying on them for our tests is a step in the wrong direction. |
The test values do not have to be magic numbers if they are read from an external file (e.g. json or csv). A script could be written (not a dependency of tests) that generates the external test value file. If I am understanding your comment correctly, you are currently following a similar process, but without showing your work. Opinion: Magic numbers should be avoided when possible. @MassimoCimmino I think a lighter weight fluid property package would be beneficial, and could help resolve #204. |
@j-c-cook if I am understanding you correctly, you are just wanting to move the magic numbers out of a python unit test file and into an external file. I don't think this is necessary. The physical property data isn't going to change, so looking up values one time and hard coding them into the unit tests is a reasonable approach. While magic numbers in the program code itself are generally frowned upon, I don't think the argument against magic numbers works as well when you are talking about unit tests comparing function outputs against known fixed meaningful physical property values. |
The I agree with Matt that making
|
Thank you @mitchute for the contribution. I will try and address all points.
|
@MassimoCimmino I have started a validation of I am getting some error levels that I really wasn't expecting from work done by @mitchute and @Myoldmopar. I am assuming that I have made a mistake somewhere. Could you please check my work? The current absolute errors I am seeing are:
The script calculates fluid properties in pygfunction 2.2.1 and j-c-cook/SecondaryCoolantProps@ad1c480d105d6ef14a6793671dc1395355bb4470 over a 2-dimensional range. The concentration and temperature values are varied. A |
Fixed my error here. The
MEG and MPG appear to produce nearly identical results, while MEA and MMA do have some differences. (Note: There could be an error in this analysis. A second set of eyes would be helpful.) |
There may be an issue with freeze point temperatures.
|
@MassimoCimmino Thanks for pointing that out. When I correlated the freezing points I mistakenly kept the concentration in percent, instead of fraction of concentration. The new version has fixed this and changed the freezing point calc over to the Melinder correlations also. Responding to your points above:
Yes, this is a major reason for this work. We would like these relatively simple fluid property routines to not continue to be a bottleneck for this and other projects. I will say that I've run into some issues testing with Python 3.10, but that's because the
Agreed - these are standard correlations by Melinder 2010, or CRC Handbook in the case of water.
In the case of physical properties, hardcoded numbers are fine IMO. A better way in some cases would be to run multiple versions (base branch and mod branch) and then compare results. The CI system here isn't so sophisticated, so having hardcoded numbers at least lets us know when something has broken, or even changed for valid reasons.
Yes, we will keep it stable. |
|
Tests pass on all versions from 3.7 to 3.11. I will do a final review and merge in the coming days. |
Updated checks on freeze points:
|
PR replaces the CoolProp fluid properties library with the lighter, zero-dependency library SecondaryCoolantProps. CoolProp has historically been the go-to library for refrigerant and coolant props, but development activity and support as of recent years have been waning, and up until recently versions >3.8 were unsupported. It's unclear if this will persist into the future, so relying on that dependency limits the usability of pygfunction and introduces uncertainty into the project.
SCP is an ultra-lightweight, zero-dependency library focused just on secondary coolants, which IMO should be enough for this application.