-
-
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
Improve behavior when kinetics / reactions are unspecified #773
Conversation
@speth Thanks for fixing this so quickly! I have a question about handling Case 1. I think there are two sub-cases there: Case 1a:
Case 1b:
It looks like you're handling these the same: cantera/test/data/phase-reaction-spec1.yaml Lines 13 to 18 in 8b95189
I would prefer if the case highlighted here throw an error, continuing the current behavior. That seems more natural, it is likely that someone made a typo in a section name or something like that. Is there a reason not to throw in this case? |
Codecov Report
@@ Coverage Diff @@
## master #773 +/- ##
==========================================
+ Coverage 71.41% 71.44% +0.02%
==========================================
Files 372 372
Lines 43904 43935 +31
==========================================
+ Hits 31355 31388 +33
+ Misses 12549 12547 -2
Continue to review full report at Codecov.
|
I would regard your Case 1b as a separate case, and there has been no change in behavior for this. That is, specifying We could make |
I guess the current behavior isn't clear to me then. If |
Correct, it is just the phases:
- name: example1
species: [{gri30.yaml/species: [O2, H2, H2O]}]
thermo: ideal-gas
kinetics: gas
reactions: all
- name: example2
species: [{gri30.yaml/species: [O2, H2, H2O]}]
thermo: ideal-gas
kinetics: gas In that case, to get a phase with a Kinetics model but no reactions, you'd have to explicitly write: - name: example3
species: [{gri30.yaml/species: [O2, H2, H2O]}]
thermo: ideal-gas
kinetics: gas
reactions: none which I suppose isn't so bad, given that that's a relatively uncommon scenario. |
No, only I don't mind the explicit |
The behavior I had originally intended is that if a kinetics model is specified, the default value for
My thinking is that the most common use case is to have a single
None of this is all that difficult to handle with the parser. The bigger complexities come from the fact that the |
I think it makes sense to have
Yes, this resulted in quite a bit of the complexity in |
The |
Oh, hmm, I didn't remember that
I didn't realize |
8b95189
to
9451ac9
Compare
I changed the behavior so that |
Including a 'reactions' field without a 'kinetics' field in the phase definition is an error. Including a 'kinetics' field without a 'reactions' field is allowed only if a 'reactions' section exists. To specify a kinetics model with no reactions, the 'reactions' field must be set to 'none'.
Make a test case which always requires swapping the species
dff3f92
to
f111fc7
Compare
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 good to me, the error messages make it pretty clear how to fix the problems
This fixes a few oddities in the handling of cases where one or more of the following were not provided in the input file:
kinetics
field in the phase entryreactions
field in phase entryreactions
sectionCase 1:
kinetics
field withreactions: all
(or noreactions
field) and noreactions
section:reactions
sectionCase 2:
reactions: all
with nokinetics
field, with or without areactions
section:Thanks to @bryanwweber for pointing out the first case.