SurfaceArrhenius <=> StickingCoefficient #2148
Conversation
davidfarinajr
added
Topic: Kinetics
Topic: Catalysis
Status: WIP
|
I also ran a test(which included |
Codecov Report
@@ Coverage Diff @@
## master #2148 +/- ##
==========================================
+ Coverage 47.84% 47.89% +0.05%
==========================================
Files 102 102
Lines 27120 27069 -51
Branches 6958 6946 -12
==========================================
- Hits 12976 12966 -10
+ Misses 12753 12716 -37
+ Partials 1391 1387 -4
Continue to review full report at Codecov.
|
|
These are useful methods to have, to convert back and forth, and we should keep them, but I feel like training reactions should all be in the same format in the database, and the person adding them to the database should convert at that point, rather than when loading the database at run time. This will greatly simplify the averaging, and the interpretation of averaging, and Matt's auto tree generation, etc. So I suggest a unit test added to the database checks to ensure all training reactions in adsorption reaction families are in StickingCoefficient form (and perhaps prints the helpful conversion in the error report if it's in SurfaceArrhenius, to facilitate changing it). It could also warn you if the converted value exceeds 1.0 at some temperatures. |
|
Worth checking we do this: things entered in the reverse direction (i.e. desorption) will be a |
mazeau
left a comment
mazeau
left a comment
There was a problem hiding this comment.
This looks good! I just am not a fan of the hard coded stuff
rmgpy/data/kinetics/family.py
Outdated
| elif isinstance(data, SurfaceArrhenius): | ||
| if has_sticking_coeff: | ||
| if surface_site_density is None: | ||
| surface_site_density = 2.483e-09 |
There was a problem hiding this comment.
I think instead of having this hard coded in, you should load it from the metal database
There was a problem hiding this comment.
don't remember what the units are in the db, so this might affect your reaction.py additions
rmgpy/data/kinetics/family.py
Outdated
| data = SurfaceArrheniusBEP( | ||
| if has_sticking_coeff: | ||
| if surface_site_density is None: | ||
| surface_site_density = 2.483e-09 |
davidfarinajr
force-pushed
the
arrh_to_sticking
branch
from
c563ab2 to
97c9569
Compare
davidfarinajr
force-pushed
the
arrh_to_sticking
branch
from
97c9569 to
46141b1
Compare
ChrisBNEU
left a comment
ChrisBNEU
left a comment
There was a problem hiding this comment.
I left some nitpicky comments, but these two new functions are tools that are not used in the main RMG model building code, so I think it is good to go.
| if adsorbate is None: | ||
| adsorbate = r | ||
| else: | ||
| logging.error("Error in kinetics for reaction {0!s}: " |
There was a problem hiding this comment.
nitpicky, but do we have adsorption reactions that have multiple adsorbates? I think this is really testing if the reaction is the correct type (i.e. adsorption). maybe the text could say "multiple adsorbates detected, reaction probably isn't adsorption"?
| sticking_coefficient = self.kinetics.get_rate_coefficient(Tlist[i]) | ||
| sticking_coefficient /= math.sqrt(constants.kB * Tlist[i] / (2 * math.pi * molecular_weight_kg)) | ||
| klist[i] = sticking_coefficient | ||
| for i in range(number_of_sites): |
There was a problem hiding this comment.
nitpicky, but this could just be klist *= surface_site_density**number_of_sites.
davidfarinajr
force-pushed
the
arrh_to_sticking
branch
from
46141b1 to
bc78dbb
Compare
|
This pull request is being automatically marked as stale because it has not received any interaction in the last 90 days. Please leave a comment if this is still a relevant pull request, otherwise it will automatically be closed in 30 days. |
github-actions
bot
added
the
abandoned
Motivation or Problem
Some surface reaction families have training data with mixed kinetics types (SurfaceArrhenius and StickingCoeff). We need methods to convert SurfaceArrhenius to StickingCoeff and vice versa so that we can convert the training data to the same kinetics type so that we can average them.
Description of Changes
Testing