Enable fitting and reversing of negative reaction rates. #1834
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rwest
added
For Next Release
Status: Ready for Review
|
The Codacy complaints are about a few spaces missing after commas in some code generated by our own |
mliu49
reviewed
Dec 3, 2019
rmgpy/kinetics/arrhenius.pyx
Outdated
| @@ -152,6 +152,13 @@ cdef class Arrhenius(KineticsModel): | |||
| data. | |||
| """ | |||
| import scipy.stats | |||
| assert all(np.isfinite(klist)), "Rates must all be finite, not inf or NaN" | |||
| if any(klist<0): | |||
| assert all(klist<0), "Rates must all be positive or all be negative." | |||
There was a problem hiding this comment.
Could you convert these assertions to raise ValueError instead?
There was a problem hiding this comment.
Done. as a fixup, when rebasing to latest master.
This test fails, for me.
File "/Users/rwest/opt/anaconda3/envs/rmg3/lib/python3.7/site-packages/numpy/linalg/linalg.py", line 2156, in lstsq
x, resids, rank, s = gufunc(a, b, rcond, signature=signature, extobj=extobj)
File "/Users/rwest/opt/anaconda3/envs/rmg3/lib/python3.7/site-packages/numpy/linalg/linalg.py", line 101, in _raise_linalgerror_lstsq
raise LinAlgError("SVD did not converge in Linear Least Squares")
numpy.linalg.linalg.LinAlgError: SVD did not converge in Linear Least Squares
See #1833
The scipy version allows you to change the algorithm used, to work around some bugs in certain versions of MKL.
rwest
force-pushed
the
reversekinetics
branch
from
17bfe93 to
fc97fb2
Compare
Codecov Report
@@ Coverage Diff @@
## master #1834 +/- ##
=======================================
Coverage 43.91% 43.91%
=======================================
Files 83 83
Lines 21564 21564
Branches 5652 5652
=======================================
Hits 9469 9469
+ Misses 11048 11034 -14
- Partials 1047 1061 +14
Continue to review full report at Codecov.
|
Some MultiArrhenius and MultiPDepArrhenius expressions use negative A factors for one of the expressions. This is permitted as long as the overall sum is positive. This commit allows the Arrhenius.fit_to_data to work with negative rates. Probably solves #1833
This reverts commit c49521f. Having fixed the problem of trying to take logarithms of negative rates, I think the numpy algorithm is probably robust enough, and there is no need to use the scipy version. Though I have no idea which is "best".
Sometimes you have negative k and want a negative A... (as part of a MultiArrhenius expression)
rwest
force-pushed
the
reversekinetics
branch
from
fc97fb2 to
523cf48
Compare
This was referenced Dec 13, 2019
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation or Problem
Sometimes people represent a complex rate expression as the sum of Arrhenius, or sum of PDepArrhenius terms, one or more of which is negative. Although an overall negative rate expression is not allowed, one part of the sum may be.
When generating reverse rate expressions, these multiple sum type expressions are reversed one part at a time. The function to generate a reverse rate expression for an individual Arrhenius expression does so by evaluating the rate at some array of temperatures, calculating the reverses (from the equilibrium constants) then doing least squares regression to fit a new Arrhenius. But that last step involves taking logarithms of the rates, which leads to
nanif they're negative, which then messes up the least squares regression.This was the underlying cause of #1833 (not problems in my LAPACK implementation, as originally suspected)
Description of Changes
If trying to fit a negative rate, negate it then fit it then negate the A factor.
Also ensure that all k(T) have the same sign, and are all finite.
Also added a couple of unit tests, that would fail without this fix.
Testing
Unit tests have been added.
(The second one doesn't really test the result, just tries not to crash. But it used to crash)