Detect unspecified thirdbody collision partners

[ischoegl]

Changes proposed in this pull request

This PR is the second half of a fix for #967 (with the first half being taken care of by #968). I decided to post this as a dedicated PR instead of attaching the commits to #968, as review policies weren't clear, and this addition doesn't depend on the solution proposed in #968.

I believe the question on how to treat concentration has been settled in discussions in #967 and #968, but a process to convert reactions that explicitly specify a collision partner was still missing (per @decaluwe's suggestion). As an alternative to explicitly relabeling all affected reactions, it is possible to leave the existing syntax

- equation: H + 2 O2 <=> HO2 + O2
  rate-constant: {A: 2.08e+19, b: -1.24, Ea: 0.0}

and use simple rules to identify this reaction as a three-body reaction in a pre-processing step (similar to what is already done for the detection of electrochemical reactions). I.e. O2 would be detected as the collision partner, and treated as if one would manually specify

- equation: H + O2 + M <=> HO2 + M
  rate-constant: {A: 2.08e+19, b: -1.24, Ea: 0.0}
  type: three-body
  default-efficiency: 0
  efficiencies: {O2: 1.0}

Specifically, I am suggesting the following four rules for converting an elementary to a three-body reaction:

1. one species has to occur on both sides
2. reaction type must not be specified
3. integer stoichiometric coefficients for all participating species
4. one side has to have exactly 3 participating species to qualify

If applicable, fill in the issue number this pull request is fixing

Fixes #967 (as long as #968 is merged also)

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

[ischoegl]

@speth, @decaluwe and @rwest ... after making this its own PR - and adding unit tests - I believe this is ready for review and/or comments. If the approach is acceptable and finds approval, it should probably be merged at the same time as #968 so #967 can be resolved.

[rwest]

Looks OK to me! Thanks Ingmar

[ischoegl]

@decaluwe ... I saw that you merged #968 (which is the companion PR). I resolved the merge conflict created by Blowers-Masel, so this is ready for review again.

[speth]

This looks pretty good to me, @ischoegl. All of my comments are on fairly minor items. Beyond the in-line comments, there are just a couple of other things:

• Can you add a test for a reaction created via XML, given that you implemented this logic for both YAML and CTI/XML derived reactions?
• Have you thought about how these reactions should be serialized? Right now, they are converted to three-body reactions with the default-efficiency and efficiencies fields set as one might expect. I guess the question is whether or not that extra information should be deleted.

Lastly, I just wanted to note that I like that this ends up moving the third body to the end of the reactant / product strings for these reactions.

[ischoegl]

@speth ... thanks for your review. Hadn't thought about serialization as this is still a brand-new addition, but your point is well taken. I had to change duplicate checks but will look at this again.

I just wanted to note that I like that this ends up moving the third body to the end of the reactant / product strings for these reactions.

Likewise. Have to admit that this was an unintended 'feature', and would probably have fought to keep the new behavior (as it is both more instructive and easier to implement).

PS: Regarding XML tests, those are implicitly included by test_convert.py.

[ischoegl]

@speth (and @bryanwweber) ... I think this should take care of the review comments. I did not feel the need to implement XML unit tests, as those are (implicitly) covered by test_convert.py: it is already verified that equation types created from CTI and YAML correspond.

I did, however, implement a shortened serialization output, which I had not thought about.

[ischoegl]

Thanks @speth!