Codecov Report

Merging #995 (952d113) into main (01bf417) will increase coverage by 0.33%.
The diff coverage is 93.78%.

@@            Coverage Diff             @@
##             main     #995      +/-   ##
==========================================
+ Coverage   72.59%   72.92%   +0.33%     
==========================================
  Files         356      357       +1     
  Lines       46508    47120     +612     
==========================================
+ Hits        33763    34363     +600     
- Misses      12745    12757      +12     

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 e60f513...952d113. Read the comment docs.

@speth ... I think this is getting close to where it should be. Benchmarks (YAML vs XML) are:

Average time of 100 simulation runs for 'gri30.yaml' (CH4)
- New framework (YAML): 155.06 ms (T_final=2736.60)
- One Python reaction: 163.27 ms (T_final=2736.60) ... +5.30%
- Old framework (XML): 148.09 ms (T_final=2736.60) ... -4.50%

So performance comparisons are still in line with #982.

There are a couple of items I am aware of but would rather tackle in combination with Falloff in a follow-up PR:

• A couple of performance tweaks that I am anticipating:

  • MultiRate can be improved by using C++ std::vector<std::pair<size_t, RateType>> Edit: done
  • ThirdBodyCalc can directly store indices in composition vector

• Some of the old-style Arrhenius objects are still exposed in the new Python interface that is based on ArrheniusRate; I am, however, waiting for feedback on the interfaces before finalizing this work

• Factory classes for ReactionRate and templated MultiRate objects are obviously missing

• Serialization will depend on Enable serialization of Cantera objects #984, but should be straight-forward. The new structure has clear delineations between Reaction and ReactionRate objects.

• Edit: In Python, the new reaction objects expose rate objects themselves, rather than making rate objects methods accessible through methods and properties defined for a Reaction. Some of the old method should probably be temporarily added for the transition.

Finally, one item that came up in the UG (Change A,b,Ea in memory) ... those values are directly mutable for the new framework - as a proof of concept, I implemented test_reaction.py::TestArrhenius::test_allow_negative_pre_exponential_factor2. It would be straight-forward to add setters ... Edit: vector::push_back creates copies (in MultiRate::add), so there are still distinct copies of ReactionRate objects. It is, however, a solvable problem (e.g. std::move is supported, or the new copy could replace the original version). Edit: done

PS: over the next couple of days, I'll likely go over docstrings one more time, but the implementation itself won't change at this point. Edit: done

One wart of the implementation thus far was that MultiRate creates a copy of a ReactionRate object that was not mutable in memory (same situation as previously with Rate1). With this update, it becomes possible to modify ArrheniusRate parameters in memory, e.g. from Python

gas.reaction(2).rate.pre_exponential_factor *= 2

which modifies both the rate attached to the Reaction object, as well as the rate handled by the MultiRate evaluator.
(Edit: leaving this for later to make the review easier)

@speth ... I believe this is now (finally) converged after deciding to incorporate the ability to modify reaction rate parameters in memory. At 3.5k lines, I believe there is a stopping point ...

PS: while CI passes, there is some glitch for the test-kinetics unit test if run by itself that I haven't been able to track down (not familiar enough with googletests: it doesn't shut down properly)

Rebased to include #984 and converted back to draft. Waiting to have #968, #987 and #1014 merged.

Had to rebase after a merge conflict due to #1049. The newly failing test is unrelated (see #1055) ... everything else is taken care of (I think). Unfortunately the rebase means that getting to some of the last discussion requires a couple of clicks as it is currently hidden.

@speth - thanks for catching those. Fixed the docstrings and squashed the last couple of commits as they just reshuffled approaches for a single feature.

Thanks, @speth!