-
Notifications
You must be signed in to change notification settings - Fork 230
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
Support reversing PDepArrhenius objects containing MultiArrhenius rates #1659
Conversation
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
19f9566
to
d99d95f
Compare
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=None
to all calls tonumpy.linalg.lstsq
. In numpy 1.14, the default behavior for thercond
parameter changed. Passingrcond=None
silences 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.