-
-
Notifications
You must be signed in to change notification settings - Fork 349
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
Implementation of Peng-Robinson EoS #641
Conversation
Codecov Report
@@ Coverage Diff @@
## main #641 +/- ##
==========================================
+ Coverage 72.39% 72.50% +0.10%
==========================================
Files 364 368 +4
Lines 46433 47047 +614
==========================================
+ Hits 33616 34111 +495
- Misses 12817 12936 +119
Continue to review full report at Codecov.
|
Can you please rebase onto the current |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @gkogekar thanks for adding that additional test. This is generally what I was thinking of. I have a few other suggestions, but it is very very close...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @gkogekar and @decaluwe! Thank you so much for this awesome addition, it was clearly a ton of work to put together. Don't mind the number of comments, with this much code to check through, there are always going to be small things. This is overall really good work and I'm looking forward to seeing it in Cantera!
Most of the comments here are about the code style and formatting. I didn't check the implementation/equations thoroughly, but there are a few implementation questions as well. To summarize some of the main comments:
- Please use spaces instead of tabs and consistently indent levels by 4 spaces.
- Please try to use more descriptive names for variables in some places, the names are kind of ambiguous. Remember that someone else (including you in a few months!) will have to come back and figure out what this code is doing.
- I left a comment that a longer documentation should be added to the header file. I'm not sure that's the right place for it though, I wonder if it should go on the website instead?
Otherwise, a few other changes I'd like to see that didn't fit in a line comment:
- Can this new class be created from Python? I don't see why not, but adding a test to the Python suite would be good.
- Can this new class be created from a YAML file? Again, don't see why not, but a test of that would be awesome.
- I'm a little concerned about the code coverage, you're only covering about half of the code you're adding, but we'd like to see closer to 70-80%. I'm especially concerned about the complexity of the cubic solver. Is there any way to add some tests of that solver, even if it is only testing the solver without any physical basis? In other words, is there a way to feed that solver an arbitrary set of coefficients of a cubic equation to test that it works properly?
Please let me know any questions/concerns that you have!
Hi @bryanwweber, thanks for these comments! @gkogekar will have a more detailed reply and fix some of these easier issues. Most are very clear, but I suppose we need to decide, re: documentation, if we want it in header files or on the website. I think long term the website is the better location, save for a very basic description in the header file. Then the header file can just link to the relevant section on the website. In the short term, this would make the website documentation oddly imbalanced (i.e. a whole lot about Peng-Robinson, but not much else), but I suppose that is unavoidable, and not that big a problem. So should we just agree that she should add this info to the website? |
@decaluwe Yes, I think the website would be a better place. We do intend to transfer all of the thermo model documentation there eventually, maybe the page that we add could note that most of the documentation is still in C++ and will be moved, to explain why P-R is the only one there 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like some good work toward implementing the Peng-Robinson equation of state. I have two high-level requests, which I think will help improve this PR.
First, many of the methods appear to be exactly the same as in RedlichKwongMFTP
(e.g. enthalpy_mole
, entropy_mole
, calcDensity
, setToEquilState
, setTemperature
, etc.), and there are others that could be refactored to extract common portions (e.g. the cubic equation solver, the getCoeff
method for reading from critProperties.xml
). I think it would be worth refactoring to eliminate this redundancy. The only question is whether the common methods should just be moved up to MixtureFugacityTP
, or whether there is a need for a new intermediate class. My preference would be for the former.
Second, do we need to have XML-based construction? Since the YAML format is now implemented, can we make this the first class to be part of that format only?
b88285d
to
f3c9fae
Compare
I agree with @bryanwweber’s comments: thanks for putting in a substantial amount of work! Beyond, It would be great if this PR could include an introductory example that illustrates usage (python would be ideal). In the spirit of #652, cantera users would benefit from a better overview of what different thermo objects are available and how to use them. Perhaps the example could, as an example, juxtapose ideal gas (default), Redlich-Kwong and the new Peng-Robinson EoS? |
61f04cc
to
d879455
Compare
@speth and @bryanwweber - a question about how non-ideal EoS are handled in YAML. I was looking to add an implementation of
Thanks, |
For Redlich-Kwong, the cross-fluid parameters are specified in a field called cantera/test/data/thermo-models.yaml Lines 536 to 552 in ba32337
That file ( Because of the YAML format, you cannot duplicate keys in a mapping, so you can't do something like this: equation-of-state:
model: Redlich-Kwong
model: Peng-Robinson or species:
- name: H2O
equation-of-state:
model: Redlich-Kwong
equation-of-state:
model: Peng-Robinson So I think you'd need a whole new species definition to add the PR parameters. It seems like the best bet is to make a new file for the PR case. I wonder if it would be better to have a simpler (although obviously less useful) example input file for these cases. |
No, you can't have multiple I hadn't really run across any examples where it seemed useful enough to implement a way of doing this while implementing the YAML format, but I did have one idea in mind that we could consider, which would be to allow the equation-of-state:
- model: Redlich-Kwong
a: 1.7458e+08 bar*cm^6/mol^2*K^0.5
b: 18.18 cm^3/mol
- model: Peng-Robinson
a: 2.34e+09
b: 12.5 cm^3/mol Then, when adding the species to the phase, the thermo model could decide which entry from this list to use. I'm not sure how much this ends up complicating the process of adding the species. In this particular case, I think the implementation would be pretty simple. There might be others where it ends up being a mess, but I guess we could always disallow the behavior in those cases. |
Thanks for the feedback, @bryanwweber and @speth. My thoughts are not fully formed here, but:
I would guess that the answer to the latter is "not very often," but I also think it is a nice goal to maintain the original spirit of the software, if the implementation of @bryanwweber - I could certainly construct a second yaml file for just this phase, but I'm thinking of making an example that can demonstrate, more generally, Cantera's non ideal EoS capabilities. Having a single input file for this purpose would be nice, both so that users have a one-stop reference, and to avoid "input file bloat." if possible. 😁 Thanks again, to you both! |
While it is a departure from the XML/CTI format, and setting aside the current limitations of the
I think there is also a parallel with how we handle species transport data - the parameters used by multiple transport models are part of the species definitions. The same limitation and possible extension of allowing this to be a list with data for multiple models applies here as well, for example if we were to introduce a transport model that didn't want the Lennard-Jones parameters. |
These are good points. Regardless of whether we enable listing of multiple EoS for a given species, I will proceed at the moment with a separate YAML for the Would the preference be that I eliminate all cti files from the PR? I'm guessing yes. |
note the typo in my last post has been corrected. I've added a YAML implementation. Should I remove all cti and xml files and references? |
Yes, I think any new models introduced at this point should use only the YAML format. |
c85b8c7
to
7df4499
Compare
Hi all, just wanted to get everyone on the same page, for this PR. By my reckoning, there are four main changes required to get this PR on solid footing for merging. The first two relate more generally to a wish for greater test coverage:
Thoughts on these? @speth and @bryanwweber it would also be good to know the time frame on the next release. Obviously I'd like to get this merged before then. Thanks! |
While I like the idea of using jupyter in general, I believe it may be illustrative to have a really simple/bare-bones illustration for how EoS behavior changes for the different thermo models. From what I've seen, there's currently only a NonIdealShockTube.ipynb, which throws in several more complications. I was hoping to see something more along an intro to thermo perspective instead of a combustion application. There's currently no such thing in the thermo folder (or the Python examples of the Cantera package). PS: Looks like my comment isn't too far from what's already suggested. My main point is to make it a new example in a different folder. |
throw CanteraError("PengRobinson::setSpeciesCoeffs", | ||
"Unknown species '{}'.", species); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the definition proposed by Pitzer, acentric factor is not likely to be less than -1. So is it more appropriate to detect the effectiveness of ω and report the incorrect input here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @Yuji-1 - thanks for this comment!
There is always a balance in what the user is allowed to input, in terms of what is known/thought to be "correct," and what a user might want to try out, for some reason perhaps not known to us. Think of it as a balance between "rules" and "freedom."
In this case, so long as an acentric factor less than -1 does not lead to errors or failures in the code, my preference would be to allow it.
Co-authored-by: Ray Speth <speth@mit.edu>
Co-authored-by: Bryan W. Weber <bryan.w.weber@gmail.com>
Co-authored-by: Bryan W. Weber <bryan.w.weber@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few unaddressed comments here, plus one minor formatting comment. Thanks @gkogekar!
src/thermo/PengRobinson.cpp
Outdated
double a0 = 0, a1 = 0; | ||
if (item2.second.isScalar()) { | ||
a0 = units.convert(item2.second, "Pa*m^6/kmol^2"); | ||
} | ||
setBinaryCoeffs(item.first, item2.first, a0, a1); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @speth said here, you're declaring a1
to be zero and then passing it into setBinaryCoeffs
right away, but is it supposed to be something else?
I put a good bit of effort into cleaning up the commit history on this branch to eliminate some of the unnecessary churn (commits switching line endings from LF to CRLF and back, commits that mistakenly added lines containing merge conflict markers, etc.) and pushed that branch to my fork (
@gkogekar, my suggestion at this point would be to reset your branch to match mine, and close this PR. Once you've had a chance to resolve the issues leading to these test failures, you can then open a new PR and we won't have this enormous comment history bogging things down. |
Closing this PR and creating a new one. |
A revised PR is created here: #1047 |
Changes proposed in this pull request: