Skip to content
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

Read YAML using C++ newSolution in Python / access root level data #1129

Merged
merged 25 commits into from
Nov 27, 2021

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Oct 23, 2021

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
Copy link

codecov bot commented Oct 23, 2021

Codecov Report

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

Impacted file tree graph

@@            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 ischoegl mentioned this pull request Oct 23, 2021
11 tasks
@ischoegl 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
@ischoegl ischoegl marked this pull request as ready for review October 25, 2021 02:59
@ischoegl
Copy link
Member Author

ischoegl commented Oct 25, 2021

@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.

Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ischoegl ischoegl force-pushed the fix-1013 branch 7 times, most recently from ad084ab to 4b199df Compare November 25, 2021 06:22
@ischoegl ischoegl requested a review from speth November 25, 2021 06:23
@ischoegl
Copy link
Member Author

@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
Copy link
Member Author

ischoegl commented Nov 26, 2021

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

Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ischoegl
Copy link
Member Author

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

Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve handling of transport_model="None" Serialization of root-level information in YAML
2 participants