Reverse kinetics for sticking coefficient

[davidfarinajr]

Motivation or Problem

RMG had no method to reverse Sticking Coefficient kinetics. I believe this is related to this website issue #2079. Adding a method to reverse Sticking Coeffs could be useful to compare Sticking Coeff kinetics with literature rates that are expressed as Arrhenius in the reverse direction. This method could also be useful if we need to reverse sticking coefficient kinetics in training reactions.

Description of Changes

Added StickingCoefficient kinetics handling to reaction get_rate_coefficient method so that we can use this method for all types of kinetics.
Added a method to reverse StickingCoefficient kinetics with default surface site density.

Testing

Added a unit test for reversing StickingCoefficient kinetics

[rwest]

Try to assume default values as little as possible.
In the context of the website, some surface site density must be assumed (which should be reported on the website), but otherwise we should have it specified in the reaction simulation, and should use that value. By putting default values in the method definitions, we risk forgetting to use the actual value.
But the fact that we didn't need these methods suggests we won't actually use them during a simulation, so it's perhaps a moot point. Although if we do end up using it, as you suggest in the PR comment, for reversing training reactions, then getting the surface site density from the metal attributes database would presumably be the right thing to do 🤷 Although perhaps doing it per site would be better than per square centimeter anyway.

Anyway, in cases where you do need to use an assumed default, please make sure to specify it in the docstrings that this is happening.

[rwest]

I don't think this one is required, because it's set by the calling functions? I'm not very happy with having a default, in general, as it increases the chance of someone forgetting to pass it when they should. (There's no current bug, I'm just anticipating future bugs..)

[codecov]

Codecov Report

Merging #2080 (dcf656a) into master (7735643) will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2080      +/-   ##
==========================================
+ Coverage   47.54%   47.60%   +0.06%     
==========================================
  Files          89       89              
  Lines       23565    23565              
  Branches     6131     6131              
==========================================
+ Hits        11204    11219      +15     
+ Misses      11177    11166      -11     
+ Partials     1184     1180       -4     

Impacted Files	Coverage Δ
arkane/kinetics.py	12.24% <0.00%> (ø)
rmgpy/molecule/draw.py	53.26% <0.00%> (+1.28%)	⬆️

---

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 7735643...dcf656a. Read the comment docs.

[davidfarinajr]

Try to assume default values as little as possible.
In the context of the website, some surface site density must be assumed (which should be reported on the website), but otherwise we should have it specified in the reaction simulation, and should use that value. By putting default values in the method definitions, we risk forgetting to use the actual value.
But the fact that we didn't need these methods suggests we won't actually use them during a simulation, so it's perhaps a moot point. Although if we do end up using it, as you suggest in the PR comment, for reversing training reactions, then getting the surface site density from the metal attributes database would presumably be the right thing to do 🤷 Although perhaps doing it per site would be better than per square centimeter anyway.

Anyway, in cases where you do need to use an assumed default, please make sure to specify it in the docstrings that this is happening.

Thanks for your review Richard! Sounds good, I can take the default values out. One of the reasons for the default is that for all of the kinetics types except Sticking Coeff in ‘generate_reverse_rate_constant’, we don’t need the surface site density (so I don’t want to force the user to specify one). In that case, maybe I should remove Sticking Coeff from that function and we should just call ‘ reverse_sticking_coeff_rate’. It makes sense to have a separate function for this since it’s the only one dependent on surface site density. As for training reactions, I think most if not all of our families are in the surface adsorption direction (rather than the desorption direction) so we haven’t needed to reverse a sticking coeff yet, but if we do, I think it makes the most sense to use the surface densities in the Metal DB since we are also using atomic binding energies from there.

[rwest]

Agree that requiring a parameter that is not used would be bad. Could do something like have it default to zero and raise an exception (ValueError?) if it's needed and is zero. But not complain if it's not needed.

[mazeau]

This looks good to me! I like richard's suggestion of not having a default value.

[davidfarinajr]

With the last push, I added docstings and changed the default surface_site_density to 0 for methods that don't require a surface site density. That way, the user doesn't need to provide it, but if it is needed in the function to calc Surface Coefficient rate constant, we will raise a value error if a nonzero (and non negative) surface site density is not provided.

We would just need to provide a surface site density for the website for generate_reverse_rate_constant method

[rwest]

Looking good! thanks david. I just noticed one last thing.

[rwest]

Probably this type check with a nice polite TypeError should check k_forward before we assign it to kf which has already been declared a StickingCoefficient in cython. Currently, if k_forward is not StickingCoefficient I guess it'll crash on line 866 and never reach 867 ?

[davidfarinajr]

ah yes, good catch. I'll fix this

[mazeau]

It still looks good to me

[Tingchenlee]

I think I can understand this. Thanks, David! It looks good to me.