Support reversing PDepArrhenius objects containing MultiArrhenius rates#1659
Support reversing PDepArrhenius objects containing MultiArrhenius rates#1659amarkpayne merged 5 commits intomasterfrom
Conversation
mliu49
added
Type: Error
Status: Ready for Review
mliu49
force-pushed
the
reverse_pdep_multi
branch
from
ce81424 to
c60691b
Compare
Codecov Report
@@ Coverage Diff @@
## master #1659 +/- ##
=========================================
+ Coverage 41.68% 41.7% +0.01%
=========================================
Files 176 176
Lines 29342 29341 -1
Branches 6050 6049 -1
=========================================
+ Hits 12232 12237 +5
+ Misses 16240 16235 -5
+ Partials 870 869 -1
Continue to review full report at Codecov.
|
amarkpayne
left a comment
amarkpayne
left a comment
There was a problem hiding this comment.
The code itself looks good, thanks for the PR! I have a couple of quick comments but this looks mostly ready to go. Before you rebase, please swap the order of the commits so that the updated code that allows PDepArrhenius to be reversed comes before the unit test.
Just to document it, @mliu49 and I discussed the order of commits offline. It makes more sense as is, as having the test first shows that we have a bug in the old implementation, and then the next commit finally fixes the bug. |
amarkpayne
left a comment
amarkpayne
left a comment
There was a problem hiding this comment.
Everything looks good to me. I'll merge this in after the Travis tests pass.
Silence numpy warnings and use new default behavior From numpy documentation: Changed in version 1.14.0: If not set, a FutureWarning is given. The previous default of -1 will use the machine precision as rcond parameter, the new default will use the machine precision times max(M, N). To silence the warning and use the new default, use rcond=None, to keep using the old behavior, use rcond=-1.
mliu49
force-pushed
the
reverse_pdep_multi
branch
from
19f9566 to
d99d95f
Compare
2 participants
Motivation or Problem
This really should have been fixed a long time ago, but better late than never.
#329 implemented the capability for the PDepArrhenius class to hold MultiArrhenius rates instead of just Arrhenius rates. This is commonly seen in literature rates, especially from MESS calculations, e.g.
As noted in #797, RMG could not reverse these rates and would print the following error:
Description of Changes
This PR adds the ability to reverse such rates along with a unit test. Since RMG can reverse MultiArrhenius rates, this simply reverses the MultiArrhenius rate associated with each pressure.
This also adds
rcond=Noneto all calls tonumpy.linalg.lstsq. In numpy 1.14, the default behavior for thercondparameter changed. Passingrcond=Nonesilences the warnings printed by numpy and accepts the new default behavior.https://docs.scipy.org/doc/numpy/reference/generated/numpy.linalg.lstsq.html
Testing
Check that tests pass. Jobs containing such rates (e.g. from the Klippenstein_Glarborg2016 kinetics library) will crash during the check collision limit violators step, which is another way to test.