Read YAML using C++ newSolution in Python / access root level data by ischoegl · Pull Request #1129 · Cantera/cantera

ischoegl · 2021-10-23T22:50:23Z

Changes proposed in this pull request

Use C++ newSolution to parse YAML strings and file input
Save root level YAML information and input source in C++
Ensure that an empty transport manager is created in C++ newSolution
Replaces previous Python implementation of YAML input
Implementation expands existing serialization introduced with Enable serialization of Cantera objects #984
Allow for user-defined top-level YAML fields in YamlWriter (e.g. description)

Also:

Deprecate Species.fromYaml in favor of Species.from_yaml
Ensure that resetting transport in Python is registered at the C++ level
~~Fix a minor glitch where LaTeX formatting in a figure title crashes example CI runs~~

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

Closes #1013, closes #1131, supersedes #881, needed for #692

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

In [1]: import cantera as ct
   ...: gas = ct.Solution("h2o2.yaml", transport_model=None)
   ...: gas.input_header
   ...:
Out[1]: 
{'cantera-version': '2.5.0',
 'date': 'Wed, 11 Dec 2019 16:59:04 -0500',
 'description': 'Hydrogen-Oxygen submechanism extracted from GRI-Mech 3.0.\nModified from the original to include N2.',
 'generator': 'ck2yaml',
 'input-files': ['h2o2.inp', 'gri30_tran.dat'],
 'units': {'activation-energy': 'cal/mol', 'length': 'cm', 'quantity': 'mol'}}

The transport_model is no longer Transport if not set, but yields

In [2]: gas.transport_model
Out[2]: 'None'

where appropriate safeguards are now also implemented in C++.

Further, it is now possible to specify top-level YAML fields for YamlWriter. For example in extract_submechanism.py

gas2 = ct.Solution(name="gri30-CO-H2-submech",
                   thermo="ideal-gas", kinetics="gas", transport_model="Mix",
                   species=species, reactions=reactions)

# Save the resulting mechanism for later use
gas2.update_user_header({"description": "CO-H2 submechanism extracted from GRI-Mech 3.0"})
gas2.write_yaml("gri30-CO-H2-submech.yaml", header=True)

which produces

description: CO-H2 submechanism extracted from GRI-Mech 3.0
generator: YamlWriter
cantera-version: 2.6.0a3
git-commit: ec800b863
date: Mon Oct 25 03:01:22 2021
...

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-10-23T23:25:21Z

Codecov Report

Merging #1129 (f51bd6d) into main (2d211db) will increase coverage by 0.10%.
The diff coverage is 85.45%.

@@            Coverage Diff             @@
##             main    #1129      +/-   ##
==========================================
+ Coverage   73.52%   73.62%   +0.10%     
==========================================
  Files         365      366       +1     
  Lines       48242    48373     +131     
==========================================
+ Hits        35470    35617     +147     
+ Misses      12772    12756      -16

Impacted Files	Coverage Δ
include/cantera/base/Solution.h	`100.00% <ø> (ø)`
include/cantera/base/YamlWriter.h	`100.00% <ø> (ø)`
src/transport/TransportBase.cpp	`31.57% <ø> (+3.00%)`	⬆️
include/cantera/transport/TransportBase.h	`48.61% <47.22%> (+40.03%)`	⬆️
src/transport/TransportFactory.cpp	`98.33% <83.33%> (-1.67%)`	⬇️
src/base/Solution.cpp	`93.06% <93.75%> (-2.28%)`	⬇️
include/cantera/kinetics/KineticsFactory.h	`100.00% <100.00%> (ø)`
include/cantera/thermo/ThermoFactory.h	`100.00% <100.00%> (ø)`
include/cantera/transport/TransportFactory.h	`100.00% <100.00%> (ø)`
src/base/YamlWriter.cpp	`100.00% <100.00%> (ø)`
... and 18 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...f51bd6d. Read the comment docs.

ischoegl · 2021-10-25T15:07:36Z

@speth / @tsikes ... this PR replaces #881 (access to top-level YAML annotations), with a - hopefully - better fate. I improved the linkage of YAML input in Python and the underlying C++ so both use the C++ version of YAML input for Solution. I also replaced the Transport band-aid in Python with something that works universally.

There is currently one failing test (Sundials 5.7 not getting correct thermal_diff_coeffs values), where I'm running out of ideas as I cannot replicate the issue on either of my two machines (may have to create a docker with the exact same environment).

Edit: this docker container can reproduce the failure.

Methods are renamed to Species.from_yaml and Species.from_yaml_file

Also update listFromYaml in test_thermo.py

In addition, additional information available in the input file is saved.

speth

Thanks, @ischoegl, this generally looks pretty good to me. I had a couple of suggestions for simplifications, as well as some other minor comments.

include/cantera/base/Solution.h

src/base/Solution.cpp

include/cantera/base/Solution.h

src/base/Solution.cpp

interfaces/cython/cantera/base.pyx

interfaces/cython/cantera/test/test_transport.py

interfaces/cython/cantera/base.pyx

test/transport/transportModels.cpp

ischoegl · 2021-11-25T06:26:43Z

@speth ... I believe I took care of all the comments. Regarding the transport model, it's possible to replace the C++ "None" with a Python None, but the string version may be more intuitive/descriptive to users.

ischoegl · 2021-11-26T17:13:17Z

@speth ... thanks for the clarification. I updated as suggested ...

speth

Thanks for the updates, @ischoegl, I think this is almost set. I just had a couple of further comments, mainly related to the new factory functions.

include/cantera/thermo/ThermoFactory.h

include/cantera/transport/TransportFactory.h

interfaces/cython/cantera/yamlwriter.pyx

ischoegl · 2021-11-27T17:55:20Z

@speth ... I implemented the one remaining suggestion for newTranport. I believe this should be ready ...

speth

Thanks, @ischoegl, this looks good to me.

Regarding one of your last comments, I would agree that the various global factory functions for new Cantera thermo / kinetics / transport / etc. objects could use some housekeeping. At a minimum, there's both the inconsistency whether they return raw pointers, unique_ptr or shared_ptr as well as confusing variability in their names (do we need the Mgr suffix in newKineticsMgr?).

ischoegl mentioned this pull request Oct 23, 2021

Access YAML annotations #881

Closed

4 tasks

ischoegl force-pushed the fix-1013 branch from 48d3bbb to e5eafc2 Compare October 23, 2021 22:52

ischoegl mentioned this pull request Oct 23, 2021

Improve handling of transport_model="None" #1131

Closed

ischoegl mentioned this pull request Oct 23, 2021

Paper cuts in YAML serialization API #1053

Closed

11 tasks

ischoegl force-pushed the fix-1013 branch 9 times, most recently from 7a4ff1b to ec800b8 Compare October 25, 2021 02:41

This was referenced Oct 25, 2021

Broken example CI runs #1132

Closed

Improve YAML output formatting #1128

Closed

ischoegl marked this pull request as ready for review October 25, 2021 02:59

ischoegl mentioned this pull request Oct 26, 2021

Simpler way to create objects defining an interface Cantera/enhancements#103

Closed

ischoegl force-pushed the fix-1013 branch 2 times, most recently from cb136b3 to dafe140 Compare November 9, 2021 03:47

ischoegl added 9 commits November 19, 2021 21:56

[Python] Address unpythonic Species.fromYaml and Species.fromYamlFile

6ce6bb8

Methods are renamed to Species.from_yaml and Species.from_yaml_file

[CI] Switch test_transport.py tests to h2o2.yaml

a8f325c

Also update listFromYaml in test_thermo.py

[Solution] Avoid re-parsing of YAML input in C++

e5b5292

In addition, additional information available in the input file is saved.

[Python] Expose C++ YAML input data at root level

897e655

[Solution] Add information on input data used for newSolution

c107167

[Solution] Skip non-standard YAML reaction fields in header

9c69c7d

[Transport] Simplify allocation of empty transport managers

6b3a4a5

[Transport] Improve exception handling

2a55c54

[Python] Build Solution from YAML input using C++ newSolution

165cece

ischoegl added 3 commits November 19, 2021 21:56

[Samples] Explicitly specify transport model in extract_submechanism.py

9bdc366

[UnitTests] Add test for customized YamlWriter header

f914571

[CI] Switch sundials runner to pytest

863125b

ischoegl force-pushed the fix-1013 branch from dafe140 to 863125b Compare November 20, 2021 03:57

speth requested changes Nov 23, 2021

View reviewed changes

ischoegl force-pushed the fix-1013 branch 7 times, most recently from ad084ab to 4b199df Compare November 25, 2021 06:22

ischoegl requested a review from speth November 25, 2021 06:23

speth requested changes Nov 26, 2021

View reviewed changes

ischoegl added 5 commits November 26, 2021 17:40

Small fixes of docstrings and nomenclature

13a42fa

[Python] Remove local copies of shared_ptr in _SolutionBase

401f69a

[UnitTests] Fix failing test

f6b796a

[Solution] Limit YAML header to fields preceding 'phases' entry

c845540

[Transport] Remove 'NoTransport' class

46f43a8

ischoegl force-pushed the fix-1013 branch from ed11833 to 46f43a8 Compare November 27, 2021 00:02

ischoegl requested a review from speth November 27, 2021 00:02

[Transport] Update signature of new newTransport function

f51bd6d

speth approved these changes Nov 27, 2021

View reviewed changes

speth merged commit 0805854 into Cantera:main Nov 27, 2021

ischoegl deleted the fix-1013 branch November 27, 2021 20:17

ischoegl mentioned this pull request Nov 28, 2021

Pickle serialization of Solution objects #692

Merged

ischoegl mentioned this pull request Dec 14, 2021

Export Solution to Chemkin Format Cantera/enhancements#52

Closed

Uh oh!

Conversation

ischoegl commented Oct 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ischoegl commented Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

speth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ischoegl commented Nov 25, 2021

Uh oh!

ischoegl commented Nov 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

speth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ischoegl commented Nov 27, 2021

Uh oh!

speth left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ischoegl commented Oct 23, 2021 •

edited

Loading

codecov bot commented Oct 23, 2021 •

edited

Loading

ischoegl commented Oct 25, 2021 •

edited

Loading

ischoegl commented Nov 26, 2021 •

edited

Loading