New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Using metal attributes #2025
Using metal attributes #2025
Conversation
RMG currently (?):
is RMG should (assuming it loads thermo first? not sure the order):
To do this:
Speculative ToDo List:
getting really confused as to which metal will refer to what... could either be NOTE: this does NOT change the thermo groups from using adsorption pt111 as a default/is currently hard-coded in -- also, do we want to load in all the thermo libraries and scale to where (the BEs) we want the model generated on, and then use the already scaled thermo to scale the thermo for the kinetics training reactions (probably easiest) or use original, non-scaled thermo to scale from? it shouldn't matter either way.... Also need to update saving methods in rmgpy/data/kinetics.py for saving training reactions if there is a metal attribute and probably same with rmgpy/data/thermo.py |
a0899d7
to
162305d
Compare
Codecov Report
@@ Coverage Diff @@
## master #2025 +/- ##
==========================================
+ Coverage 47.54% 47.58% +0.03%
==========================================
Files 88 89 +1
Lines 23304 23499 +195
Branches 6062 6105 +43
==========================================
+ Hits 11080 11181 +101
- Misses 11065 11146 +81
- Partials 1159 1172 +13
Continue to review full report at Codecov.
|
This pull request introduces 4 alerts when merging 34aa030 into 3c13a22 - view on LGTM.com new alerts:
|
4ed71a9
to
44f02cb
Compare
This pull request introduces 4 alerts when merging 44f02cb into 3c13a22 - view on LGTM.com new alerts:
|
This pull request introduces 5 alerts and fixes 3 when merging 751af74 into 3c13a22 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 5 alerts and fixes 3 when merging 82b6783 into 3c13a22 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 5 alerts and fixes 3 when merging e344e20 into 3c13a22 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 2 alerts when merging 0e857da into 3c13a22 - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging 3426502 into 3c13a22 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 79a6263 into 3c13a22 - view on LGTM.com new alerts:
|
Something to note: Katrin mentioned that the surfaceThermoPt111 things were calculated on binding energies from https://github.com/ReactionMechanismGenerator/RMG-Py/pull/1975/files the BEEF-vdW calculations from Karlsruhe - (2019 paper) so I just added them back in as hard coded, because these conflict with the new pt111 binding energies in the database. |
This pull request introduces 2 alerts when merging d644803 into 3c13a22 - view on LGTM.com new alerts:
|
Current RMG-Py test failures:
|
This pull request introduces 1 alert when merging 6702156 into 3c13a22 - view on LGTM.com new alerts:
|
All old failures fixed, now I have these:
|
This pull request introduces 1 alert when merging c760868 into 3c13a22 - view on LGTM.com new alerts:
|
c760868
to
0283e3d
Compare
This pull request introduces 1 alert when merging 0283e3d into 3c13a22 - view on LGTM.com new alerts:
|
get_training_set: adding metal entry attribute trying to pass training reaction metal attribute to thermo calculations add_rules_from_training: adding metal entry attribute passing through metal training entry attribute during reverse rate thermo calculations FIXUP: if there is no facet, change NoneType to an empty string FIXUP: family.py, condensing code to fix string and None adding error
The Pt111 values in the metal database are not the same as the binding energies used to calculate the Pt111 surface thermo library, so they are hard coded in here FIXUP: thermo.py now correctly passes metal into correct_binding_energy FIXUP: thermo.py all tests pass now!
…e example. This commit generalizes the test and makes it also test the write-read loop for a surface mechanism (which was copied from RMG-database/input/kinetics/libraries/Surface/Example. The test however is not good enough, as it doesn't actually enforce the written library is the same as the input. For example, the `metal` attribute in the original is not duplicated in the copy, and yet the test still passes. This may indicate a problem with a __repr__ method somewhere as well as the saving.
Load it once and refer to it. Fixup initializing ThermoDatabase: At this point there's no help in calling set_binding_energies, and the metals database hasn't even been loaded yet.
When generating a model on Ni, we should preferentially use the Ni thermo data when it exists. Fall back to the Pt because there's not much Ni. This Metal Attributes branch should take care of scaling the thermo so they both end up on the right metal (Ni).
forgot to update some of the many save entries
tests to make sure that metal attributes are always strings
In a unit test that we can save a database library, we need a blank folder to put it in. Previously, it would fail if that folder existed. Now it just wipes it.
be69e63
c35c6ea
to
be69e63
Compare
Thanks @mazeau and @kblondal for this contribution, and David and all the other reviewers. Requires an update to RMG-Py, which was pull request ReactionMechanismGenerator/RMG-Py#2025 merged in 6146e3e1c90b583bcc3b15d0afea98947e1bd1ef
Motivation or Problem
Currently, RMG can only use surfaceThermoPt111 as it assumes the reference point is Pt111. We want to be able to:
This also adds a new metal database so users don't have to look up binding energies for specific metals of interest
Reviewer Tips
I just created this PR so we would have a place for discussion.
Sister database PR is #387