-
Notifications
You must be signed in to change notification settings - Fork 26
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
YAML format documentation #91
Conversation
f2c7560
to
d69cb4a
Compare
I'm having a little trouble with the |
I don't see any |
They've already been lowercased at that point. |
Aha, I figured it out - this goes the other way around. According to the Docutils docs (http://docutils.sourceforge.net/docs/ref/rst/restructuredtext.html#reference-names) the names are supposed to be case insensitive, and be normalized to lowercase. It shouldn't be looking for the capitalized reference name in the first place. |
Needed to be updated after setting Doxygen to use subdirectories
2e5b8b1
to
fff7e8e
Compare
I think this is ready for review. I've also pushed this to the @ischoegl I'd like to request your review here as well -- with this documentation completed, this is a good opportunity for feedback on the YAML format in general. |
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.
Looks mostly great, thanks @speth!
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 @bryanwweber mentioned, this looks really good.
Just had a couple of comments. Thanks for creating something really useful (both YAML/AnyMap input and great documentation).
|
||
An SRI falloff function may be specified in the CTI format using the | ||
:cti:class:`SRI` directive, or in the YAML format using the | ||
:ref:`SRI <sec-yaml-falloff>` field in the entry. | ||
|
||
Chemically-Activated Reactions |
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.
I still believe that the minute difference to fall-off doesn't warrant a separate reaction type, see Cantera/cantera#751
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.
Note: Looks like in some instances, the lines I'm referring to appear at the bottom! (apologies)
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.
I'm going to leave this as-is for now. Obviously, depending on the resolution of that issue, this documentation may need to be revised.
Chebyshev reactions can be defined in the CTI format using the | ||
:cti:class:`chebyshev_reaction` entry, or in the YAML format using the | ||
:ref:`Chebyshev <sec-yaml-Chebyshev>` reaction ``type``. | ||
|
||
Surface Reactions |
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.
Should there be a mention of electrochemical
reactions (see Cantera/cantera#747; I'm still not convinced that automatic detection is a clean implementation, which however was the consensus from Cantera/cantera#750)
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.
Yes, there certainly should, but I'm not trying to resolve all of the deficiencies of the "science" section of the website in this PR.
conversion. The numeric field values should all be entered as pure numbers, with | ||
no attached units string. | ||
effect of each species on the transport properties of the phase. Currently, | ||
ideal-gas transport property models are implemented. |
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.
Is this still true? There appears to be a transport model for water ...
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.
Well, the only ones for which species-level transport coefficients are specified are for gases.
pages/tutorials/input-files.rst
Outdated
- Use one of the pre-existing input files provided with Cantera | ||
- Convert a pre-existing mechanism from Chemkin (CK) format to Cantera (CTI) format | ||
- Create your own CTI file, either from scratch (not recommended) or by editing an existing file | ||
- Create your own YAML file from scratch or by editing an existing file *(New in | ||
Cantera 2.5)* |
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.
Shouldn't conversion from CK to YAML and CTI/XML to YAML be added here as well? (it's described below, but not mentioned 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.
Yes, that was missing. I've added a new section with a ck2yaml
tutorial, which is essentially a duplicate of the ck2cti
tutorial.
pages/tutorials/yaml/reactions.rst
Outdated
- :ref:`elementary <sec-yaml-elementary>` | ||
- :ref:`three-body <sec-yaml-three-body>` | ||
- :ref:`falloff <sec-yaml-falloff>` | ||
- :ref:`chemically-activated <sec-yaml-chemically-activated>` |
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.
see previous comment: I think this could be handled more efficiently by adding a boolean chemically-activated
field to falloff
reactions.
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.
Pending Cantera/cantera#751
======= | ||
|
||
A species entry in Cantera is used to specify the name, composition, | ||
thermodynamic, and transport properties of an individual 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.
Should Cantera/enhancements#14 be considered for 2.5.0?
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.
Sure, I'd consider including an implementation if someone made a PR, but I don't think it's a blocker for the release.
I think I've addressed all the issues comments except for a few related to open PRs on |
One easily addressed comment: I noticed that there's no YAML input file link in the 'Other Documentation' box on the index page. [Also, searching for |
Even on the "testing" site, that main API docs page is for the stable release. If you follow the "Development Version" link further down the page under "Need something else?", the search will find all of the YAML API docs for the development version, and you will see links to YAML documentation under the "Other Documentation" section. |
@speth Wow! Was definitely not expecting to change all I'll take a look at the rest of this tomorrow. Thanks again! |
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.
One small change and responding to a comment. Thanks again @speth
I have also pushed this PR branch to the
testing
branch, so this is currently live at https://testing.cantera.org (specifically, https://testing.cantera.org/tutorials/yaml/defining-phases.html)