-
Notifications
You must be signed in to change notification settings - Fork 15
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
add fugacity funcs compatible with Sept 2023 CoolProp master branch #33
Conversation
Do you have any idea on the timeline for the CoolProp release? We could also release a JLL based on the git version. |
No, but I would like to get as many of the low-level passthrough functions implemented at the same time to avoid making a million little releases since it takes a while to do each release |
I have been working on a comprehensive review of functions existing in These functions in AbstractState.h cannot be called with a PT_INPUTS call and do not have a passthrough function currently. For property lookups:
Several other miscellaneous functions (I do not know which are critical to include as passthrough functions): |
OK, I'll look at merging the backlog of PRs here, first also accepting the upgrade to CoolProp 6.5. Note that we could also switch to using CxxWrap to use the C++ interface directly, that might be an easier solution? |
If you want to implement them all, that would be fine, but I personally think it makes sense to wait for someone to want it rather than implementing it before there is a need. But if it comes to the Julia wrapper, it might make sense to just do all the interfacing in one shot. |
I agree that it makes more sense to add them as they come up, but then that could lead to many PR's and release requests as you fear. However, it is another good chunk of work to do one big push, that no one else is requesting right now. Really, we only need these fugacity funcs in a C++ release to be able to stably deploy the use of We would be willing to make at least "some more" of these passthrough functions right now and maybe in exchange we could get a C++ release done. Perhaps, adding remaining property lookups, like fugacity, make the most sense and could be a good compromise. Let me know your thoughts. Thanks |
If it helps, this fork has some of the low-level functions added in branch update-CoolProp-library. The branch update-Julia-wrapper has the same added to the Julia wrapper. Not fully tested though. I had planned to submit the changes after adding all of the low-level functions and attempting to update the Julia wrapper (at least for local use if not merged upstream) with following changes:
Progress on the above is in new-Julia-wrapper branch. |
Thanks for the info @n-aswin. I will review what you have shared and will see where I might be able to help. In the meantime, since I've only needed the fugacity calls, specifically, for my current work, I was able to sleuth how to make a local version of the |
Just to be clear: we wait with merging this one until a new CoolProp comes out, right? |
That is correct. |
@barche CoolProp 6.6.0 has been released and added to Yggdrasil |
I have added an updated version (rebased on master) at PR #35, let's continue the discussion there. |
Closes #31
Fugacity lookups did not exist in
CoolProp.jl
. They are added here. However, a fugacity function in the base C++ which is compatible with Julia wrapping did not exist previously. The C++ code was added successfully. CoolProp/CoolProp#2286This will be a draft PR until
CoolProp
releases a new version that includes the change which is ahead of v6.5. Once that is released andCoolProp_jll
is updated, this PR can be merged.