Adding capability to have reverse surfaceArrhenius training reactions #1884
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1884 +/- ##
==========================================
- Coverage 44.45% 43.41% -1.05%
==========================================
Files 83 83
Lines 21645 21650 +5
Branches 5654 5656 +2
==========================================
- Hits 9622 9399 -223
- Misses 10963 11215 +252
+ Partials 1060 1036 -24
Continue to review full report at Codecov.
|
| Tlist = 1.0 / np.arange(0.0005, 0.0034, 0.0001) | ||
| # Determine the values of the reverse rate coefficient k_r(T) at each temperature | ||
| klist = np.zeros_like(Tlist) | ||
| for i in range(len(Tlist)): |
There was a problem hiding this comment.
is there a reason this isn't
for i, T in enumerate(Tlist):
then use the T directly instead of looking up Tlist[i] twice?
There was a problem hiding this comment.
I just copied this directly from the reverse_arrhenius_rate and that's how it is there
|
what unit tests would you like to see? |
mjohnson541
left a comment
mjohnson541
left a comment
There was a problem hiding this comment.
I don't understand the purpose of the last commit, can you describe the problem this avoids in the commit message?
|
The last commit specifically treats the surface migration family differently, because it is a special case where a surface species changes where it is bonded to the surface. It does not subtract switch from m3/mol to m2/mol, because it was mistakenly switching it even though it was the only surface species. |
|
Found a better way to fix the two problems that were earlier avoided by skipping unit tests. I rebased it, so the first two commits on this branch are actually in #1894 And I edited the last commit, so it fixes the test rather than skipping it for a hard-coded family name. |
|
This now looks ok to me. Would be nice to see #1894 merged, then this rebased on top, then I'm fine with it. But I was the last to edit it so probably someone else should review it too :) |
amarkpayne
left a comment
amarkpayne
left a comment
There was a problem hiding this comment.
Since @rwest and @mjohnson541 have taken a look at this, I think this can get merged in as soon as #1894 gets dealt with.
Since you will have to rebase anyways, do you mind making the following changes before you rebase? If time permits it would also be nice to have a unit test for the reverse_surface_arrhenius_rate function just in case future changes accidentally break this functionality. But if you don't have the time I don't want to hold up this PR any longer
Surface families that have only one surface species and nothing else, start with units of inverse seconds, and so you can't decrease the power of metres by zero, because there is no power of meters to decrease. Instead of subtracting zero from a dictionary value which didn't exist, we now subtract one from it zero times, thus avoiding the key error!
Previously, if there was an non-isomorphism present in the labeled atoms, then the mapping would be invalid, but this is not (currently) checked by the is_isomorphic method, so two non-isomorphic molecules would be found isomorphic, and would fail the test. Now we ensure that the mapping is valid, before passing it to is_isomorphic. This should work whatever the outcome of the discussion around PR ReactionMechanismGenerator#1894
|
Thanks for the changes @mazeau ! Matt is looking over the isomorphism change now |
… for surface arrhenius
|
I've added in the new unit test |
4 participants
I was having issues with surfaceArrhenius training reactions that were in the reverse direction, as they would all be overwritten as ArrheniusEP instead of SurfaceArrheniusBEP. This change fixes that and all tests pass locally and I am able to successfully generate models!