Duplicate reactions not being caught in ck2yaml

[tsikes]

Several variations of duplicate reactions are not being caught when parsing chemkin mechanisms.

System information

• Cantera version: 2.5.0a4
• OS: Windows 10
• Python/MATLAB version: 3.7

Expected behavior

When given a duplicate reaction, an error should be thrown to inform the user.

Actual behavior

• When passed A + B = C + D and A + B = C + D:

The old problem of r1.third_body being None and having no attribute upper crops up yet again.

• When passed A + B = C + D and B + A = C + D:

The duplicate reactions are not flagged and the reactions are added.

• When passed A + B = C + D and A + B = D + C:

The duplicate reactions are not flagged and the reactions are added.

• When passed A + B = C + D and C + D = A + B (note at least one is reversible):

The duplicate reactions are not flagged. This one I am not certain of but I believe that newer versions of Chemkin would flag this as an unmarked duplicate reaction?

To Reproduce

Steps to reproduce the behavior:

1. Run the attached mechanism through ck2yaml

Attachments
duplicate-reactions.txt

Additional context

All of these issues are found in the check_duplicate_reactions function of ck2yaml.

I've fixed the first issue by checking that r1/r2 has the attribute upper before checking third body things.

I've fixed the second and third problems by ordering the reactants and products by name.

Finally the third I've fixed by checking if the reactant/product/pressure dep tuple named k exists in possible_duplicates as product/reactant/pressure dep.

If these solutions seem alright, I can do a pull request to resolve these bugs.

[bryanwweber]

Thanks @tsikes! A PR would be good, thank you!

[speth]

The main issue here is that the better duplicate reaction check in C++ (Kinetics::checkDuplicates) isn't actually being called for YAML input files. I've pushed a simple fix for this to my fork at https://github.com/speth/cantera/tree/fix-yaml-duplicate-reaction-check. I think that handles all of the cases here which are expected to be seen as errors.

Ideally, Kinetics::checkDuplicates would be the one and only place for checking duplicate reactions. The only reason to keep something in the converter is that it's easier to provide an error message indicating the line in the original input file, while Kinetics::checkDuplicates doesn't even (currently) tell you the line in the YAML file, which is still annoying to trace back to the corresponding line in the CK input file.

[tsikes]

@speth running yaml inputs through the C++ checkDuplicates seems like a must do; however, I also agree with what you said about transparency for tracking down problems with CK input files. Additionally, while the C++ checkDuplicates would fix problems 2-4, it would not fix problem 1 because the conversion itself breaks. I think doing both would work though.

Edit: Nevermind, I assumed you were checking the information that was being sent from the yaml file. Your solution is much cleaner and succinct. I'll close my pull request.