Custom filter criteria per reaction family

[ajocher]

Motivation or Problem

By filtering reactions we add a pre-filtering step before the step of reacting species together (slow), which prevents species from reacting together when the reactions are expected to be negligible throughout the simulation. Currently, unimolecularThreshold, bimolecularThreshold, and trimolecularThreshold are binary arrays storing flags for whether a species or a pair of species are above a reaction threshold. For a unimolecular rate, this threshold is set to True if the unimolecular rate of reaction 𝑘 reaction k for a species A

at any given time 𝑡 t in the reaction system, where

For a bimolecular reaction occuring between species A and B, this threshold is set to True if the bimolecular rate

where 𝑘𝑡ℎ𝑟𝑒𝑠ℎ𝑜𝑙𝑑=filterThreshold is set by the user in the input file and its default value is
.
Similarly, for a trimolecular reaction, the following expression is used:

where

The threshold values can be refined for each reaction family to speed reaction generation up.

Description of Changes

In this pull request we will refine the threshold values for uni-/bi- and trimolecular reactions by evaluating an Arrhenius fit for each reaction family at the user defined reactor temperature. The Arrhenius fits were obtained by extracting the highest reaction rates from all training reactions of one family for a discrete set of temperatures. The ipython notebook is RMG-Py/ipython/kinetics_training_analysis.ipynb.

Testing

Tested superminimal and minimal example with reaction filtering turned on. The latter one also with pdep.

Reviewer Tips

This pull request shouldn't dramatically change the core model of your own test case, but speed reaction generation up.

Modified Travis to simultaneously update RMG-Py and RMG-database. When merging, the code reviewer should remove the temporary commit on RMG-Py.

[codecov]

Codecov Report

Merging #1577 into master will decrease coverage by 0.03%.
The diff coverage is 38.95%.

@@            Coverage Diff            @@
##           master   #1577      +/-   ##
=========================================
- Coverage   44.24%   44.2%   -0.04%     
=========================================
  Files          83      83              
  Lines       21551   21627      +76     
  Branches     5648    5668      +20     
=========================================
+ Hits         9535    9561      +26     
- Misses      10958   11010      +52     
+ Partials     1058    1056       -2

Impacted Files	Coverage Δ
arkane/explorer.py	7.85% <0%> (-0.05%)	⬇️
rmgpy/rmg/settings.py	82.6% <100%> (+0.38%)	⬆️
rmgpy/rmg/input.py	42.57% <20%> (-0.07%)	⬇️
rmgpy/rmg/main.py	21.91% <4.93%> (-0.23%)	⬇️
rmgpy/rmg/react.py	81.96% <71.42%> (-4.58%)	⬇️
rmgpy/data/kinetics/database.py	50.44% <85.36%> (-0.17%)	⬇️
rmgpy/data/kinetics/common.py	68.02% <0%> (-1.02%)	⬇️

---

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 7b3ab69...e034790. Read the comment docs.

[amarkpayne]

Quick comments to help fix your codacy issues

[mliu49]

I skimmed through the reaction generation code again, and I'm fairly sure that it's safe to not copy species before reacting. I added a unit test to confirm that a couple notable Species and Molecule attributes are not affected by the reaction generation process. Let me know if you think there are other attributes we should check.

Based on that, I removed species copying from react_species. I also removed a redundant resonance structure generation step in ensure_independent_atom_ids. Finally, I restored the prior behavior where family splitting is only employed when running in parallel, which reduces the number species tuples created for reaction generation.

To summarize the runtime differences:

stage	runtime	change
master	442 s	--
custom filter criteria v1	1270 s	+828
optimize filter threshold	780 s	-490
remove species copy	509 s	-271
remove res gen	442 s	-67
change family splitting	339 s	-103

Two of the changes I added make custom filter criteria more efficient, while two are unrelated and could also be applied on master. That means that for this particular example, custom filtering still hurts performance. I think that additional profiling would be a good idea, probably with longer jobs.

[ajocher]

Thanks for helping, I really appreciate it. My untested suspicion is that some of the Numpy vectorization operations we have added, especially in the .pyx files might not be as fast as the for loops that were there before.

[amarkpayne]

Thanks for helping, I really appreciate it. My untested suspicion is that some of the Numpy vectorization operations we have added, especially in the .pyx files might not be as fast as the for loops that were there before.

I agree with @ajocher . Specifically, I think my recommendations here were bad and should be reverted back to for loops

[JacksonBurns]

This PR is attempting to merge into master, which is abandoned. Closing this PR as stale and deleting the branch.

If there are commits on this branch that should be merged into main, please restore the branch, cherry pick them, and open a new PR.