@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: kinetics phase key present, reactions phase key missing, with or without reactions section

• This should create the specified Kinetic model (I think this is your New Behavior)

Case 1b: kinetics phase key present, reactions phase key present and pointing to a local section, corresponding local section missing

• This should throw an error, for the missing section

Are you handling Case 1b differently from Case 1a?

It looks like you're handling these the same:

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

Merging #773 into master will increase coverage by 0.02%.
The diff coverage is 89.18%.

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

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 237936c...f111fc7. Read the comment docs.

I would regard your Case 1b as a separate case, and there has been no change in behavior for this. That is, specifying reactions: [invalid-section-name] still generates an error. The case that is changed here is if the reactions field is just reactions: all, where the section to take reactions from (reactions) is implicit.

We could make reactions: all with no reactions section an error, but guessing what the user meant is a little less clear-cut here than the other situations. I suppose issuing a warning would be an option.

We could make reactions: all with no reactions section an error, but guessing what the user meant is a little less clear-cut here than the other situations.

I guess the current behavior isn't clear to me then. If reactions: all is specified, where are reactions found from? Is every top level key tested to see if it has reaction definitions? I thought it would be just the reactions section.

Correct, it is just the reactions section that is search for. You're suggesting that both of these phase definitions should be errors (if no reactions section is present)?

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 example1 should be an error. My understanding of your comment is that is not an error, but results in an GasKinetics instance with zero reactions. I think example2 should not result in an error.

I don't mind the explicit reactions: none in example3 though, now that you mention it. That would make it so if kinetics is specified, reactions has to be as well, even if the value is none (or None, can it be case insensitive, given the closeness to the Python name?). I certainly see the value in that... if it makes it easier to parse the file (or reason about the parsing code) then that seems worthwhile.

No, only example1 should be an error. My understanding of your comment is that is not an error, but results in an GasKinetics instance with zero reactions. I think example2 should not result in an error.

The behavior I had originally intended is that if a kinetics model is specified, the default value for reactions is all. So it feels a little odd for one of these to throw an exception but the other be OK. Both of these are a little suspicious, so requiring the user to add reactions: none in the second case if that's what they actually mean doesn't seem too bad.

That would make it so if kinetics is specified, reactions has to be as well, even if the value is none

My thinking is that the most common use case is to have a single reactions section, and to add all reactions from that section, which is why I thought having reactions: all be the default made sense. What I was suggesting here was a case where specifying reactions: none actually makes sense.

if it makes it easier to parse the file (or reason about the parsing code) then that seems worthwhile.

None of this is all that difficult to handle with the parser. The bigger complexities come from the fact that the reactions entry can be a string, a list of strings, or a list of mappings.

I think it makes sense to have example1 and example2 both throw errors if we also allow example3, i.e., allow none for the reactions key. Explicit is better than implicit 😄 Would/could None and null also be allowed as synonyms? We'd just have to be careful in the serializer to set that field to None rather than the empty list when serializing a phase with no reactions, but that shouldn't be hard.

the fact that the reactions entry can be a string, a list of strings, or a list of mappings

Yes, this resulted in quite a bit of the complexity in ctml2yaml.py.

I think it makes sense to have example1 and example2 both throw errors if we also allow example3, i.e., allow none for the reactions key. Explicit is better than implicit 😄 Would/could None and null also be allowed as synonyms?

The example3 behavior is how things already work, even before this PR. I can see allowing None as equivalent to none, but I am not sold on allowing the YAML null keyword, since it would introduce yet another type of value for the reactions field, and I think it would be tricky to get that across to users.

Oh, hmm, I didn't remember that null is a type in YAML, I was thinking of 'null' as the synonym, but that would be super confusing. Ok, never mind about that.

The example3 behavior is how things already work, even before this PR.

I didn't realize none was valid already. Great!

I changed the behavior so that example1 and example2 are both errors. I think the scenario of wanting a kinetics model with no initial reactions is rare enough that it requiring reactions: none is reasonable.