Fixed: Ck2yaml crashes if reaction equation has both ThreeBodyReaction and Falloff reaction types

[tsikes]

Bug: check_duplicate_reactions breaks if sent a third body reaction and falloff reaction because r1.third_body (or r2) is None and has no attribute upper.

Fix: This changes the third body reaction type to have a .third_body of M and then in check_duplicate_reactions check that r1 and r2 are not a mixture of three body reaction type and something else.

Bug: if blank space in header, parser crashes due to regular expression search (for indent size) to return None and not have start attribute

Fix: check that the comment is not empty before trying to determine indent size

Improvement: inform user which reaction contains multiple reaction types

Checklist

• There is a clear use-case for this code change
• The commit message has a short title & references relevant issues
• Build passes (scons build & scons test) and unit tests address code coverage
• The pull request is ready for review

Fixes #800

Changes proposed in this pull request

• Change third body reaction type to have a .third_body of M and then in check_duplicate_reactions check that r1 and r2 are not a mixture of three body reaction type and something else.
• Check that the comment is not empty before trying to determine indent size
• Inform user which reaction contains multiple reaction types

[codecov]

Codecov Report

Merging #855 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #855   +/-   ##
=======================================
  Coverage   71.40%   71.40%           
=======================================
  Files         372      372           
  Lines       43605    43605           
=======================================
  Hits        31134    31134           
  Misses      12471    12471           

---

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 0089281...60c978c. Read the comment docs.

[bryanwweber]

Hi @tsikes! Thanks for sending this. I'd like to ask for two small changes before we can merge this:

1. Can you use a better summary line for the commit? "Fixes issue 800" won't really mean anything to someone else who looks at this commit.
2. Can you add a test for the behavior that would previously raise an error? If you need pointers about where/how to add that, I'm happy to help.

Thanks!

[tsikes]

@bryanwweber I changed the title to be more descriptive. I'll be happy to add some tests if you can give me a bit of guidance. Also, I have never used scons, so I've not done either of those checks.

[bryanwweber]

Hi @tsikes! Thanks for the quick reply. I actually meant the commit message summary. You can edit the commit message by typing git commit --amend and then pushing to this branch with git push -f, where -f means force since the commit will have changed rather than simply adding new commits.

For the test, I think adding a pair of reactions in the CHEMKIN input file format like those in #800 to a new file in the test/data directory would be good. Then, you can add a test function like:

    def test_third_body_plus_m_reactions(self):
        self.convert('your-input-file-name.inp', thermo='dummy-thermo.dat')
        gas = ct.Solution('your-input-file-name' + self.ext)
        self.assertEqual(gas.n_reactions, 2)

after the function test_surface_mech2() in interfaces/cython/cantera/test/test_convert.py.

For your other fix, you can add a minimized version of the file that lead you to make that change into test/data and then create a similar function to test_extra() in the ck2yamlTest class in the same test_convert.py file.

We run the tests here on our continuous integration, so you don't need to run scons locally. It would certainly help with debugging your tests in this case though. Let me know if you need help setting up scons.

[tsikes]

Hi @bryanwweber. I think I got everything you needed. I made 2 tests that failed previously, but now succeed in the patched ck2yaml. I squashed the commits into 1 and changed the commit summary message.

[tsikes]

If I made a change in how ck2yaml reads in NASA7 notes, would it be appropriate to combine that with the above changes or should I make another PR?

Previously all spacing was destroyed because the species name + notes were split and stripped. I instead changed it so that they only split at the first space instead of any space.

[bryanwweber]

@tsikes I think a new PR would be better

[bryanwweber]

@tsikes Thanks for the updates! I pushed a few small changes here, removing some spaces at the ends of lines, converting tabs to spaces, and making sure all the files ended with a blank line. Nothing substantive though, just some nitpicky stuff to keep all our files consistent. Once the tests pass, I'll merge the PR (probably in the morning EDT). Thanks again for your work here!

[bryanwweber]

Thank you for submitting this @tsikes!

[rwest]

Thanks @tsikes! Glad to see #800 closed :)

[tsikes]

I'm glad I could help!