-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
Broken extract_submechanism.py example #821
Comments
This bug will also affect a range of unit tests once |
hey, i'm interested in working on it |
@sabasafdar This may not be a very good "first issue" to resolve. It requires some pretty deep knowledge of the new input format. Feel free to try it out if you want, and if you get stuck feel free to ask questions here. |
@sabasafdar ... From what I can tell, there are several ways to address this; the new YAML philosophy is somewhat different than the old XML, so some guidance from developers is certainly needed: a fix would potentially reach deep into the C++ sub layer. |
I'm going to try tackling this |
FWIW, this was introduced in 1866e35 which switched the input file to YAML. The cause of the error is that constructing a cantera/src/kinetics/Reaction.cpp Lines 1071 to 1084 in cf1c081
and cantera/src/kinetics/Reaction.cpp Line 308 in cf1c081
#845 formalized the idea that The simplest fix for the error here is to replace the offending line with all_reactions = ct.Solution('gri30.yaml').reactions() rather than using |
I see this in a similar way, but am not sure that creating a temporary
... the switch of input file formats made a pre-existing bug apparent. @kyleniemeyer ... what's your opinion on this? |
@ischoegl I will let @speth jump in as necessary, but my understanding is that the YAML format is a little more flexible in terms of the units, such that it is necessary to determine the units of the activity concentration when the reaction is created (see the definition of cantera/src/kinetics/Reaction.cpp Line 332 in cf1c081
Kinetics object is a bug, but a change in behavior that changes how this function must be used.
That said, there is a case where this function could be useful, which is if you have a Nonetheless, for this particular example, since the reactions and species are in the same file, the fix I had earlier seems like the best bet. I'll push a branch shortly, once I see if any other examples need fixes (aside from those in #876 😄 ) |
@bryanwweber ... fwiw, it would be nice if the PS: the name of the method should likely be deprecated in favor of |
Which ThermoPhase definition should be used for this allocation? What if the input file has no ThermoPhase definition, only a reactions section? How do we know if it should be bulk-phase |
@bryanwweber ... I'm not sure that we're talking about the same thing. Let's use a unit test as an example: cantera/test/kinetics/kineticsFromYaml.cpp Lines 10 to 18 in cf1c081
The I'm still on-board with your approach to load the entire solution, but would argue that doing this within PS: the change of behavior (from XML input) is summarized here ... cantera/include/cantera/kinetics/Reaction.h Lines 264 to 271 in cf1c081
i.e. the XML version didn't need |
@bryanwweber ... on further inspection, a simple fix including the original call would be
PS: After looking at this some more, there's probably too much guesswork in creating a temporary
and the error thrown would refer to |
@ischoegl Just to clarify a few things, recognizing that we're in agreement 😄 :
They can be used to create
I agree, except
I don't see a reason to include the original call. It doesn't demonstrate a use case for |
Ad 2.5.0 - I realize that it's close ... when is feature freeze / beta scheduled? There are a couple of PR's that appear to be close but haven't been updated in a while. Myself, I don't have any plans for further PR's anytime soon (am planning to wait until the counter goes up to at least #1k) Regarding |
One last comment ... |
The Reaction.listFromFile() method requires a Kinetics instance when reading a YAML file. In this case, the appropriate Kinetics instance could be created by Solution('gri30.yaml'). Since we'd have to create that object anyways to be able to use Reaction.listFromFile(), we might as well use the Solution.reactions() method, which returns the same list as Reaction.listFromFile() for this case where the species definitions are in the same file as the reaction definitions. Fixes Cantera#821, problem introduced in 1866e35
The Reaction.listFromFile() method requires a Kinetics instance when reading a YAML file. In this case, the appropriate Kinetics instance could be created by Solution('gri30.yaml'). Since we'd have to create that object anyways to be able to use Reaction.listFromFile(), we might as well use the Solution.reactions() method, which returns the same list as Reaction.listFromFile() for this case where the species definitions are in the same file as the reaction definitions. Fixes Cantera#821, problem introduced in 1866e35
When loading reactions from a YAML file, an instance of a Kinetics object is required so that the species associated with the reactions are available. The required object is now constructed from the already loaded species definitions. Fixes Cantera#821, problem introduced in 1866e35
System information
Expected behavior
The example
kinetics/extract_submechanism.py
runs.Actual behavior
To Reproduce
See above.
The text was updated successfully, but these errors were encountered: