Update handling of critical properties #1141

speth · 2021-11-21T05:33:00Z

Changes proposed in this pull request

Use critical properties stored in the species critical-parameters YAML field to compute Redlich-Kwong and Peng-Robinson coefficients
Use the acentric factor from the `critical-properties YAML field in the high-pressure gas transport model
Replace critProperties.xml with critical-properties.yaml. Entries in the new file are consistent with YAML species definitions.

I believe this last change has the distinction of being the last place in the code where the XML format was being used without a YAML alternative. This therefore allows us to start making "noisy" deprecation warnings about CTI and XML.

If applicable, fill in the issue number this pull request is fixing

This resolves a secondary point that was raised in Cantera/enhancements#20, and further discussed in Cantera/enhancements#107.

If applicable, provide an example illustrating new features this pull request is introducing

A species entry in a YAML input file:

- name: H2
  composition: {H: 2}
  thermo:
    model: NASA7
    temperature-ranges: [200.0, 1000.0, 3500.0]
    data:
    - [2.34433112, 0.00798052075, -1.9478151e-05, 2.01572094e-08, -7.37611761e-12,
      -917.935173, 0.683010238]
    - [3.3372792, -4.94024731e-05, 4.99456778e-07, -1.79566394e-10, 2.00255376e-14,
      -950.158922, -3.20502331]
  critical-parameters:
    critical-temperature: 32.938
    critical-pressure: 1.2858 MPa
    acentric-factor: -0.22

An entry from critical-properties.yaml:

- name: CS2
  descriptive-name: carbon-disulfide
  molecular-weight: 76.131
  critical-parameters:
    critical-temperature: 552
    critical-pressure: 7.9e+06
    critical-molar-volume: 0.160
    critical-compressibility: 0.276
    acentric-factor: 0.109

Checklist

The pull request includes a clear description of this code change
Commit messages have short titles and reference relevant issues
Build passes (scons build & scons test) and unit tests address code coverage
Style & formatting of contributed code follows contributing guidelines
The pull request is ready for review

codecov · 2021-11-21T05:54:03Z

Codecov Report

Merging #1141 (c38f826) into main (2d211db) will decrease coverage by 0.01%.
The diff coverage is 87.50%.

❗ Current head c38f826 differs from pull request most recent head 3511aea. Consider uploading reports for the commit 3511aea to get more accurate results

@@            Coverage Diff             @@
##             main    #1141      +/-   ##
==========================================
- Coverage   73.52%   73.50%   -0.02%     
==========================================
  Files         365      365              
  Lines       48242    48247       +5     
==========================================
- Hits        35470    35466       -4     
- Misses      12772    12781       +9

Impacted Files	Coverage Δ
include/cantera/base/AnyMap.h	`100.00% <ø> (ø)`
include/cantera/thermo/PengRobinson.h	`100.00% <ø> (ø)`
include/cantera/thermo/RedlichKwongMFTP.h	`100.00% <ø> (ø)`
src/transport/GasTransport.cpp	`87.36% <50.00%> (-0.17%)`	⬇️
src/thermo/RedlichKwongMFTP.cpp	`83.49% <80.39%> (-1.43%)`	⬇️
src/base/AnyMap.cpp	`90.53% <81.81%> (+0.69%)`	⬆️
src/thermo/PengRobinson.cpp	`88.66% <97.43%> (+0.40%)`	⬆️
src/thermo/Species.cpp	`98.91% <100.00%> (+0.01%)`	⬆️
test/thermo/PengRobinson_Test.cpp	`100.00% <100.00%> (ø)`
test/thermo/RedlichKwongMFTP_Test.cpp	`100.00% <100.00%> (ø)`
... and 4 more

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 2d211db...3511aea. Read the comment docs.

ischoegl

@speth … this looks overall very good. The only thing I could see is that the current (and previous!) implementation requires repeated parsing of a large file (6000+ lines for YAML). Would it make sense to read the file once and buffer either everything or only the subset of species that are used by a thermo model (which may, however, not be known at the time this is needed … ). A middle ground would be to introduce a PengRobinson::getCoeffs(const std::vector<std::string>& iNames) so everything can be done in one sweep if so required.

ischoegl · 2021-11-21T18:39:15Z

src/thermo/PengRobinson.cpp

-    // If the species is not present in the database, throw an error
-    if(isnan(spCoeff[0]))
-    {
+    AnyMap data = AnyMap::fromYamlFile("critical-properties.yaml");


While this isn’t an approach that was introduced here, this means that the entire file is being parsed any time coefficients are needed? Is there a more efficient way to accomplish this (e.g. by buffering the file content the first time the file is read?)

This isn't quite as bad as it looks -- AnyMap::fromYamlFile caches the AnyMap generated for any given input file, and only re-reads it from disk if it's been modified. Changing this is a bit of scope creep for this PR, but I think solving it (for both P-R and R-K) won't be too complicated.

If the resulting AnyMap is indeed cached (i.e. yaml-cpp does not have to parse again), then this observation could be laid to rest by including a comment at the two instances where the AnyMap::fromYamlFile is called (i.e. no other action needed).

ischoegl · 2021-11-21T18:43:37Z

src/thermo/RedlichKwongMFTP.cpp

-            spCoeff[1] = omega_b * GasConstant * T_crit / P_crit; // coeff b
-            break;
-        }
+    AnyMap data = AnyMap::fromYamlFile("critical-properties.yaml");


ischoegl

The only other improvement I can see is to use preferred IUPAC names (PIN’s), as the descriptive-name choices are somewhat arbitrary and probably do not consider various isomers (e.g. octane presumably refers to ‘isooctane’, which however is 2,2,4-trimethylpentane). I am not necessarily suggesting to add this here, but just to prepare for this capability by using fields for iupac-name and another one for synonyms). Down the line, this may allow for disambiguation of alternative species names a mechanism may be using.

it looks like there are several packages that support lookup, e.g. PubChemPy or ChemSpiPy … there are likely others

speth · 2021-11-22T14:44:21Z

The only other improvement I can see is to use preferred IUPAC names (PIN’s), as the descriptive-name choices are somewhat arbitrary and probably do not consider various isomers (e.g. octane presumably refers to ‘isooctane’, which however is 2,2,4-trimethylpentane). I am not necessarily suggesting to add this here, but just to prepare for this capability by using fields for iupac-name and another one for synonyms). Down the line, this may allow for disambiguation of alternative species names a mechanism may be using.

I chose the name descriptive-name for this information (copied directly from critProperties.xml) specifically because the information available doesn't uniformly follow the IUPAC convention. This way, if someone in the future wants to augment this dataset, they can add an iupac-name field or a smiles field. I'm open to a different suggestion, as long as it doesn't involve updating information for all ~700 species in this dataset.

ischoegl · 2021-11-22T14:50:04Z

This way, if someone in the future wants to augment this dataset, they can add an iupac-name field or a smiles field.

Certainly understand, but as implemented, the somewhat arbitrary choices for name and descriptive-name are a limitation to this approach. I guess this is an enhancement request ...

decaluwe · 2021-11-22T14:58:32Z

FWIW, I think that is sort of the point of the descriptive-name tag - an acknowledgment that it is not quite an ideal solution (no pun intended), but a placeholder until the longer-term project of adding iupac or smiles entries is implemented. But I don't know it's fair to put that task on this PR (which I think we all agree with, from the comments above).

Certainly worth waiting to see if this is in much demand, before undertaking the significant amount of work it would require.

speth · 2021-11-22T15:02:58Z

This way, if someone in the future wants to augment this dataset, they can add an iupac-name field or a smiles field.

Certainly understand, but as implemented, the somewhat arbitrary choices for name and descriptive-name are a limitation to this approach. I guess this is an enhancement request ...

I think the underlying issue is that most user input files come from Chemkin-format input files where there is no canonical species name, so matching the name is really the best we can do, until someone (RMG?) starts generating Cantera YAML files directly with additional species identifiers.

I'd say that the bigger feature of this PR (besides freeing us from XML) is defining a home for the critical state properties in user input files, so they can be used with any real gas model without having to compute the model-specific parameters manually. I'm not as sure about how useful this particular database is, but I think it's probably at least worth keeping around.

Because the "input" AnyMap for a species isn't a full copy (the "thermo" and "transport" child nodes are stored separately), the node metadata needs to be copied explicitly for InputFileError to work correctly.

ischoegl

@speth ... thanks for updating! This looks good to me.

speth added 4 commits November 20, 2021 22:29

[Thermo] Read species 'critical-parameters' entries for Redlich-Kwong

ff45af2

[Thermo] Read species 'critical-parameters' entries for Peng-Robinson

14e0b34

[Thermo] Convert critical properties database to YAML

22cb600

[Transport] Read acentric factor from critical-parameters field

f1f86b6

ischoegl reviewed Nov 21, 2021

View reviewed changes

speth added 2 commits November 22, 2021 16:36

[Input] Improve formatting of errors referencing species nodes

d20e93d

Because the "input" AnyMap for a species isn't a full copy (the "thermo" and "transport" child nodes are stored separately), the node metadata needs to be copied explicitly for InputFileError to work correctly.

[Thermo] Avoid reading critical-properties.yaml multiple times

3511aea

speth requested a review from ischoegl November 23, 2021 16:56

ischoegl approved these changes Nov 23, 2021

View reviewed changes

ischoegl merged commit 0e2a33d into Cantera:main Nov 23, 2021

ischoegl mentioned this pull request Dec 10, 2021

Import critical properties for thermo and transport calculation Cantera/enhancements#107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update handling of critical properties #1141

Update handling of critical properties #1141

speth commented Nov 21, 2021

codecov bot commented Nov 21, 2021 •

edited

Loading

ischoegl left a comment •

edited

Loading

ischoegl Nov 21, 2021 •

edited

Loading

speth Nov 22, 2021

ischoegl Nov 22, 2021 •

edited

Loading

ischoegl Nov 21, 2021

ischoegl left a comment •

edited

Loading

speth commented Nov 22, 2021

ischoegl commented Nov 22, 2021

decaluwe commented Nov 22, 2021 •

edited

Loading

speth commented Nov 22, 2021

ischoegl left a comment

Update handling of critical properties #1141

Update handling of critical properties #1141

Conversation

speth commented Nov 21, 2021

codecov bot commented Nov 21, 2021 • edited Loading

Codecov Report

ischoegl left a comment • edited Loading

Choose a reason for hiding this comment

ischoegl Nov 21, 2021 • edited Loading

Choose a reason for hiding this comment

speth Nov 22, 2021

Choose a reason for hiding this comment

ischoegl Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

ischoegl Nov 21, 2021

Choose a reason for hiding this comment

ischoegl left a comment • edited Loading

Choose a reason for hiding this comment

speth commented Nov 22, 2021

ischoegl commented Nov 22, 2021

decaluwe commented Nov 22, 2021 • edited Loading

speth commented Nov 22, 2021

ischoegl left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 21, 2021 •

edited

Loading

ischoegl left a comment •

edited

Loading

ischoegl Nov 21, 2021 •

edited

Loading

ischoegl Nov 22, 2021 •

edited

Loading

ischoegl left a comment •

edited

Loading

decaluwe commented Nov 22, 2021 •

edited

Loading