[thermo] `RedlichKwongMFTP` critical property lookups by decaluwe · Pull Request #464 · Cantera/cantera

decaluwe · 2017-07-03T11:52:54Z

Changes proposed in this pull request:
-Makes it so that Thermo::RedlichKwongMFTP input files do not necessarily have to provide pureFluidParameters for every species. Since these can be calculated from the critical temperature and pressure for the Redlich Kwong EoS, we provide the ability to look up Tc and Pc for well-known species.
-Provides a database of species with well-known critical properties. Species are currently matched by chemical formula, so isomers are currently prohibited. Can possibly add functionality to scan by species name, but this seems like it could lead to significant ambiguity. My preference is to keep this database limited to species where potential ambiguity is near zero. Database is stored in the data folder. Updating the database is relatively straight-foward for any end users - user simply has to lookup Tc and Pc and enter them into the database, rather than lookup Tc and Pc, calculate a and b parameters for R-K EoS, and then update the cti file.
-Adds test suite coverage for this capability.

codecov · 2017-07-03T15:20:49Z

Codecov Report

Merging #464 into master will increase coverage by 0.04%.
The diff coverage is 80.21%.

@@            Coverage Diff             @@
##           master     #464      +/-   ##
==========================================
+ Coverage   58.23%   58.28%   +0.04%     
==========================================
  Files         386      386              
  Lines       42310    42393      +83     
  Branches     7252     7266      +14     
==========================================
+ Hits        24640    24707      +67     
- Misses      15488    15495       +7     
- Partials     2182     2191       +9

Impacted Files	Coverage Δ
include/cantera/thermo/RedlichKwongMFTP.h	`100% <ø> (ø)`	⬆️
test/thermo/RedlichKwongMFTP_Test.cpp	`100% <100%> (ø)`	⬆️
src/thermo/RedlichKwongMFTP.cpp	`49.42% <73.91%> (+2.08%)`	⬆️
src/base/stringUtils.cpp	`64.06% <0%> (+0.78%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6bf74d1...38535fc. Read the comment docs.

speth

This looks really interesting, and will certainly make the RedlichKwong class much easier to use!

Based on the implementation, I would say that species are matched by name (identifier), not formula (composition), which does allow you to define isomers, but with no standard naming scheme. In order to expand the database and eliminate ambiguities, I think we would need to have a better identifier present, e.g. something like InChI. Not something that we need to worry about now, but worth considering for the future.

Rather than having users update this database, the other option for providing Tc/Pc instead of the a and b coefficients would be to look for this information in the species declaration as well. I think this would be worth looking at after we get a bit further with the new input file format.

speth · 2017-07-06T21:52:30Z

data/inputs/critProperties.xml

@@ -0,0 +1,1150 @@
+<ctml>


Is there a reference for these values?

speth · 2017-07-06T21:53:10Z

data/inputs/critProperties.xml

@@ -0,0 +1,1150 @@
+<ctml>
+
+	   <species name=      "CCL4">


Spaces, not tabs, please.

speth · 2017-07-06T21:58:14Z

src/thermo/RedlichKwongMFTP.cpp

+doublereal* RedlichKwongMFTP::getCoeff(const std::string& iName)
+{
+    vector_fp vParams;
+    static doublereal spCoeff[2];


I would recommend against using static arrays, as they introduce potential race conditions. Better to have this function return a vector<double>.

speth · 2017-07-06T22:00:26Z

src/thermo/RedlichKwongMFTP.cpp

+                                             b_vec_Curr_[iSpecies]);
+                        }
+                        else
+                        {


Prefer writing this on one line, e.g.

end block 1; } else { start block 2;

speth · 2017-07-06T22:07:23Z

src/thermo/RedlichKwongMFTP.cpp

+    for (size_t isp = 0; isp < nDatabase; isp++)
+    {
+        XML_Node& acNodeDoc = doc->child(isp);
+        acNodeDocPtr = &acNodeDoc;


Doesn't look like this variable ever gets used.

speth · 2017-07-07T02:46:15Z

src/thermo/RedlichKwongMFTP.cpp

+            P_crit = P_crit * 100000;
+
+            spCoeff[0] = 0.42748 * pow(GasConstant, 2) * pow(T_crit, 2.5) / P_crit; //coeff a
+            spCoeff[1] = 0.08664 *GasConstant * T_crit / P_crit; // coeff b


minor nitpick: space on both sides of the *.

speth · 2017-07-07T02:47:50Z

src/thermo/critProperties.xml

@@ -0,0 +1,3 @@
+<ctml>


I think this file should be deleted (preferably, do this as a separate commit, then do an interactive rebase and squash that commit with the one that introduced the file in this location, so that it's like it never existed)

speth · 2017-07-07T02:50:22Z

test/thermo/RedlichKwongMFTP_Test.cpp

 TEST_F(RedlichKwongMFTP_Test, construct_from_cti)
 {
+    const std::string valid_file("../data/co2_RK_example.cti");
+    initializeTestPhase(valid_file);


I think it's fine to just write initializeTestPhase("../data/co2_RK_example.cti");

speth · 2017-07-07T02:51:50Z

test/thermo/RedlichKwongMFTP_Test.cpp

 public:
-    RedlichKwongMFTP_Test() {
-        test_phase.reset(newPhase("../data/co2_RK_example.cti"));
+    void initializeTestPhase(const std::string & filename)


Our normal style is to not have the space between the typename and the &, e.g. std::string& filename.

speth · 2017-07-07T02:52:27Z

test/thermo/RedlichKwongMFTP_Test.cpp

+
+TEST_F(RedlichKwongMFTP_Test, critPropLookup)
+{
+    // Check to make sure that RedlichKwongMFTP is able to properly calcualte a and b


"calculate"

speth · 2017-12-02T03:33:21Z

Before making any more changes on this branch, I think we should address the warning regarding conflicts and rebase this PR on top of the current master. I have already done this if you want to pull the branch https://github.com/speth/cantera/tree/critProps and reset this PR to that commit.

decaluwe · 2017-12-02T04:35:18Z

Hi Ray, Yes, I’ve already done this on my local branch. On the speciesName(i) call, what I’m saying is that making that change does not work. The critPropLookup test fails (this is replicated after I install, just trying to load a cti with some missing pureFluidParemeters - the critical properties are not successfully read from the xml.) Have not been able to dig in and figure out why, but it seems odd that it would not work. Steven

…

Sent from my iPhone

On Dec 1, 2017, at 8:33 PM, Ray Speth ***@***.***> wrote: Before making any more changes on this branch, I think we should address the warning regarding conflicts and rebase this PR on top of the current master. I have already done this if you want to pull the branch https://github.com/speth/cantera/tree/critProps and reset this PR to that commit. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

speth · 2017-12-02T04:54:55Z

That's strange. What do you get if you use writelog to print out the species name from each source as you go through the loop? If you want to push a branch showing the problem, I can take a look at it.

speth requested changes Jul 7, 2017

View reviewed changes

decaluwe closed this Nov 17, 2018

decaluwe force-pushed the critProps branch from 38535fc to ded7c67 Compare November 17, 2018 23:06

decaluwe mentioned this pull request Nov 17, 2018

Crit props 2018 #574

Merged

Uh oh!

Conversation

decaluwe commented Jul 3, 2017

Uh oh!

codecov bot commented Jul 3, 2017

Codecov Report

Uh oh!

speth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

speth commented Dec 2, 2017

Uh oh!

decaluwe commented Dec 2, 2017 via email

Uh oh!

speth commented Dec 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants