[Kinetics] Fix third body concentrations for non-ideal gases

[speth]

Changes proposed in this pull request

• Consistently use physical concentrations when calculating third body and falloff reaction rates, for both specifically enumerated efficiencies and generic third body colliders.

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

Partial fix for #967

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

[codecov]

Codecov Report

Merging #968 (ef593d7) into main (b3c69ac) will increase coverage by 1.74%.
The diff coverage is 94.15%.

❗ Current head ef593d7 differs from pull request most recent head b5582e8. Consider uploading reports for the commit b5582e8 to get more accurate results

@@            Coverage Diff             @@
##             main     #968      +/-   ##
==========================================
+ Coverage   70.66%   72.40%   +1.74%     
==========================================
  Files         361      364       +3     
  Lines       44522    46435    +1913     
==========================================
+ Hits        31460    33620    +2160     
+ Misses      13062    12815     -247     

Impacted Files	Coverage Δ
include/cantera/base/Solution.h	100.00% <ø> (ø)
include/cantera/base/Units.h	100.00% <ø> (ø)
include/cantera/kinetics/GasKinetics.h	100.00% <ø> (ø)
include/cantera/kinetics/Kinetics.h	51.51% <ø> (ø)
include/cantera/kinetics/Reaction.h	100.00% <ø> (ø)
include/cantera/kinetics/RxnRates.h	92.13% <ø> (ø)
...ude/cantera/thermo/BinarySolutionTabulatedThermo.h	100.00% <ø> (ø)
include/cantera/thermo/ConstCpPoly.h	66.66% <ø> (ø)
include/cantera/thermo/DebyeHuckel.h	100.00% <ø> (ø)
include/cantera/thermo/HMWSoln.h	33.33% <ø> (ø)
... and 115 more

---

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 b3c69ac...b5582e8. Read the comment docs.

[decaluwe]

This fix looks straightforward and correct, but I have a question about whether it is necessary. I'll post my thoughts over on the thread for #967 though.

[decaluwe]

Hi @speth this looks good. Just one question below, to make sure that we are intentionally also changing the falloff reaction formulation to use the physical concentrations, rather than the previously-used activity concentrations.

[ischoegl]

Reading through the comments, it appears that there is a consensus that concentrations of third body collision partners should use physical concentrations as implemented in this PR. However, there is a second point that was raised in #967, which pertains to reactions that explicitly state the colliding species, which would be treated in an inconsistent manner, i.e.

H+O2+N2<=>HO2+N2

As an alternative to @speth and @decaluwe's suggestion (i.e. rewrite YAML), the detection of those reactions without modification of YAML input would be relatively straight-forward (stoich coefficient equal to one on both sides). As long as this behavior is clearly documented on the science pages, I believe an 'auto-detection' mechanism as is done elsewhere may be the simplest solution.

One way forward would be to merge the current PR but leave the issue open, as there is some overlap with other ongoing work (#995). If a consensus can be found, I'd be happy to add a fix there.

[decaluwe]

Thanks, @ischoegl . One comment, one question:

1. I think we would need to search for a reaction where a species has any stoich coefficient on both sides, not a coefficient of one on both sides--see the example that @speth posted on Bug with the computation of [M] with real gases #967 (nu_fwd is 2 and nu_rev is 1 for species O2). Am I thinking about this correctly?
2. Regardless, agreed that an automatic detection would be preferable, if it can be robustly implemented. Regarding @speth's suggestion (make the user specify type: three-body, but automatically detect the collider species). Would there ever be a reason where a species is both a reactant and product, but does not serve as a third-body collider?

[ischoegl]

@decaluwe thanks ... absolutely true about H + 2 O2 <=> HO2 + O2! (although the implementation wouldn’t be much different). I believe that specifying three-body would generally not be necessary unless the efficiency is not equal to ‘1’ (similar rationale as for electrochemical reactions, which do not need to be labeled as such - you were able to convince me on that one). Per your question, I’m curious to hear other opinions, as I am not sure myself that my intuition - that this won’t happen - is actually correct at all times.

[ischoegl]

I'm adding my approval here (with the caveat that this does not completely resolve #967).

[speth]

I just pushed a revised commit message so this won't automatically close #967. GitHub's notation that I "dismissed decaluwe’s stale review" sounds way too judgemental 🤣 .

[decaluwe]

😤😤😤 My reviews are as fresh as they get, GitHub!

🤣

Okay, so the remaining issue, @ischoegl is the auto-detction issue for where the third body collider is explicitly listed, is that correct?

[ischoegl]

@decaluwe ... correct. Would create a short PR so that won’t get buried in 3.5k lines in #995, although it neatly fits with the work there 😉 ... science docs are another remaining issue.

Edit: as an alternative, I could create a new PR building on @speths's commits here, which will avoid leaving code in flux (although it presumably only affects real gas calculations). Presumably pushing here would work, but I haven't done that before.

[ischoegl]

@speth ... looks like pushing to this PR may be simpler than expected. Any objections if I add something here?

[speth]

Pushing to PR branches is indeed pretty straightforward. If you have an implementation for this idea ready, please go ahead and push it here.

[ischoegl]

@speth ... don’t have anything ready (yet), but I’d rather tackle this separate from (and before) #995, as merge conflicts will arise. This PR already clashes as is.

[ischoegl]

@speth, @decaluwe and @rwest

I pushed a draft for detection of three-body reactions (which mostly works). Fixing the few failing tests is simple, but I wanted to get some second opinions first:

• For automatically detected three-body reactions, some of the reaction equations are currently rewritten: the third body now is always in last place, and collision partners appear with the same coefficient on both sides. E.g. 2 O2 + H <=> HO2 + O2 would become H + O2 + O2 <=> HO2 + O2 ... this could either be an annoyance or a feature, and is obviously picked up by some regression tests.
• Some of the detection has to be limited (likely requiring a coefficient of one on one side, and a coefficient larger than one on the other). As an example, the CI dug up a soot mechanism with the reaction C6H5 + BIN5 => .9846153846 BIN5 + 1.53846154E-2 BIN6, which certainly shouldn't qualify.

[speth]

On the second point, would it make sense to just skip this for reactions where there are non-integer stoichiometries? I think that since they are not elementary reactions, all of the careful considerations here about what the third body means have likely gone out the window.

[ischoegl]

This is quite odd. All tests pass locally on my machine (on the non-rebased branch) - could there be interactions with #984? The only thing I can think of is the detection of units as some species are handled as third bodies rather than regular species. Before the merge, my test runs were consistent with the CI checks.

PS: looking through the code, I believe it’s due to the new Reaction::calculateRateCoeffUnits, which can no longer rely on third bodies being clearly marked.

[Edit: PPS: @speth ... I verified that there has been a functional clash with #984 (I rebased on a separate branch - ischoegl:fix-967-detect-thirdbodies). I am very hesitant to rebase here as it's not my PR, but a fix would require changing code that is not available on this branch (yet).]

[speth]

Yes, I'd be happy to take a look.

[ischoegl]

As the detection of third-bodies ended up to be more comprehensive than this original PR while not depending on the proposed change, I decided to propose it as a separate PR (#1015).