Fix documentation bug for YAML format #1002
Conversation
Makes the `Reaction.listFromFile` example valid by adding the required Kinetics object as the second argument (`gas`, created in the example above.) I'm not sure if this is the best solution: it has the major downside of requiring `gas` to already be initialized to a `Kinetics` object with the appropriate species. I'm new to Cantera, but I couldn't find an easy way to make a "dummy" Kinetics object. Is there a better way to do this? Maybe the example should just be removed?
There was a problem hiding this comment.
Thanks for pointing this out. While this requirement is a little confusing, you need to know the thermo model of each reactant species in a reaction in order to convert the rate constant to Cantera's internal unit system of mks+kmol. This wasn't necessary before because the XML format already had the rate constants converted to those units, and the CTI sort of cheated and already knew the right units to use for the handful of thermo models that it supported.
| @@ -63,7 +63,7 @@ class Solution(ThermoPhase, Kinetics, Transport): | |||
| directly in Python:: | |||
|
|
|||
| spec = ct.Species.listFromFile('gri30.yaml') | |||
| rxns = ct.Reaction.listFromFile('gri30.yaml') | |||
| rxns = ct.Reaction.listFromFile('gri30.yaml', gas) | |||
There was a problem hiding this comment.
Can you update this to give a complete example, where you first initialize a gas with the species but no reactions, then create the list of reactions? Right now, gas is used before it is defined.
There was a problem hiding this comment.
I pushed an update defining spec_gas explicitly from the species (plus a sentence stating the requirement). It looks a bit contrived, but the example runs without error. What do you think?
|
One alternative would be to provide a base unit system as a second argument, which however would require the C++ unit system being exposed to Python. Another issue are electrochemical reactions, where the current YAML import requires associated thermo models. |
Now the example will run as-is and includes a sentence stating that the `Kinetics` object is required with YAML.
|
@ischoegl, the unit system isn't really the issue. What is needed to convert the coefficients of the rate constant are the dimensions of the standard concentration for each reactant, along with the dimensions of the phase where the reaction occurs. |
There was a problem hiding this comment.
Looks good to me. Maybe an option for making this less awkward would be to get rid of the Reaction.listFromFile constructor and add methods to the Kinetics class for creating a list of Reactions objects from a file, and a method for adding a list of Reaction objects to that Kinetics object, rather than specifying the reactions in the constructor.
Changes proposed in this pull request
Fixes the
Reaction.listFromFileexample syntax in theSolutiondocs by adding the required second argument (using thegasobject created in a previous code example.)I'm not sure if this is the best solution: it has the downside of requiring
gasto already be initialized to aKineticsobject with the appropriate species, which doesn't seem super useful. I'm new to Cantera, but I couldn't find an easy way to make a "dummy" Kinetics object. Is there a better way to do this?Maybe the example should just be removed?
If applicable, fill in the issue number this pull request is fixing
Fixes #1001
Checklist
scons build&scons test) and unit tests address code coverage