ck2yaml doesn't set third_body when parsing A+B+M reactions

[rwest]

When parsing these reactions

H+CH2+M=CH3+M			 6.E14		 0.0		0.0
H+CH2(+M)<=>CH3(+M)                      6.0E+14     .000        .00
     LOW  /  1.040E+26   -2.760   1600.00/
     TROE/   .5620  91.00  5836.00  8552.00/

it seems that ck2yaml will interpret them both as pressure dependent and set pressure_dependent=True on both, but will only set the third_body attribute to M in the second reaction (the first reaction will have third_body=None).

When running through check_duplicate_reactions it will then reach the lines

                elif (r1.third_body.upper() == 'M' and
                      r1.kinetics.efficiencies.get(r2.third_body) == 0):

and give an AttributeError: 'NoneType' object has no attribute 'upper' because r1.third_body is None.

Presumably the fix is for the parsing of the first reaction to set third_body='M' ??

But putting

                if kind == 'third-body':
                    third_body = True
                    falloff3b = 'M'  # <<<< new line

in the parse_expression method means we're setting a variable named "fall-off third body" for something that's technically not a "fall off" reaction. And may have other issues.

Or is the answer to change how check_duplicate_reactions works?

Should

H+CH2+M=CH3+M			6.E14		 0.0		0.0
H+CH2(+M)<=>CH3(+M)                      6.000E+14     .000        .00
     LOW  /  1.040E+26   -2.760   1600.00/
     TROE/   .5620  91.00  5836.00  8552.00/

be considered duplicates anyway?

I know that Cantera does not consider these to be duplicates:

H+CH2=CH3			6.E14		 0.0		0.0
H+CH2(+M)<=>CH3(+M)                      6.000E+14     .000        .00

System information

• Cantera version: 2.4
• OS: Mac OS X
• Python: Python 3.7

Attachments
mechanism.txt
thermo.txt

[bryanwweber]

Super not relevant, but congrats on raising Issue #800! 🎉

[tsikes]

I have run into the same issue today and modified ck2yaml to accommodate such an error by checking if None in [r1.third_body, r2.third_body] inside of check_duplicate_reactions. Previously such a reaction pair was allowed in Cantera, but I believe that this should be considered a duplicate reaction. I can submit a pull request if it's decided that this is the correct solution.

[rwest]

Hmmm. I'd forgotten about this. Thanks for the suggestion.
I guess your fix solves this problem with minimal changes, but if the problem is that the third_body for H+CH2+M=CH3+M should be M but is currently None (causing a NoneType error), then I think I'd prefer a fix that makes it M rather than one that allows it to be None.

[bryanwweber]

Hi @tsikes I'm not sure I understand your method, can you push the fix to a PR so we can take a look at it? Thanks!

[speth]

For this particular case, would it make sense to rename the variable fallof3b to third_body_name, and set Reaction.third_body for both kinds of reactions? This would avoid the weirdness of having a name particular to falloff reactions associated with simple third body reactions.

The issue of what should and shouldn't be a duplicate reaction is something that is better discussed as part of an enhancement proposal. Changing the current behavior could be tricky if we want to maintain some form of compatibility with Chemkin, or backward compatibility with what Cantera has allowed in the past.

[tsikes]

I modified my solution to act in the default permissive manner of Cantera. I don't think that my solution is very elegant and @speth has a better answer to this issue. Regardless I created a pull request so that you can see what I've done. #854

It makes sense that changing the current behavior could have some adverse effects on backwards compatibility.

Edit: This issue was bugging me so I fixed it properly. I also found another bug, which I fixed as well. #855