ND Hindered Rotor Improvements #1849
Merged
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
mjohnson541
added
Topic: Performance
Arkane
Status: Ready for Review
mjohnson541
force-pushed
the
Ndhr_improvements
branch
from
fe9db41
to
d16ff11
Compare
Codecov Report
@@ Coverage Diff @@
## master #1849 +/- ##
==========================================
+ Coverage 44.22% 44.25% +0.02%
==========================================
Files 83 83
Lines 21547 21551 +4
Branches 5646 5648 +2
==========================================
+ Hits 9530 9537 +7
+ Misses 10946 10944 -2
+ Partials 1071 1070 -1
Continue to review full report at Codecov.
|
mjohnson541
force-pushed
the
Ndhr_improvements
branch
from
288ca8e
to
84be1b6
Compare
alongd
reviewed
Dec 13, 2019
rmgpy/statmech/ndTorsions.py
Outdated
| return self.rootD(*phis) * np.exp(-self.V(*phis) / (constants.R * T)) | ||
|
|
||
| intg = inte.nquad(f, ranges=rngs)[0] | ||
| return (self.rootD(*phis)) * np.exp(-self.V(*phis) / (constants.R * T)) |
There was a problem hiding this comment.
remove redundant parentheses around self.rootD(*phis)?
I ran into issues related to negative predicted moments of inertia with this interpolant This commit replaces that interpolant (the 2D case) with the LinearNDInterpolator
The old scheme using nquad only worked fast because the value of the integral was below the absolute tolerance, and it wasn't very accurate after fixing the numerical issue nquad takes impractically long since the integration is over a fit to data points we don't even know the function well enough for an nquad calculation to make sense for this reason I've used simpsons rule for integration which is much faster and should be just as accurate we still fit and use function evaluations for convenience
mjohnson541
force-pushed
the
Ndhr_improvements
branch
from
84be1b6
to
2b16fdc
Compare
alongd
reviewed
Dec 16, 2019
rmgpy/statmech/conformer.pyx
Outdated
| @@ -384,7 +384,8 @@ cdef class Conformer(RMGObject): | |||
| elif pivots[0] in top1 and pivots[1] in top1: | |||
| raise ValueError('Both pivot atoms included in top; you must specify only one pivot atom that belongs' | |||
| ' with the specified top.') | |||
|
|
|||
| elif 0 in top1: | |||
| raise ValueError('Top must be zero indexed.') | |||
There was a problem hiding this comment.
Should the message say "should be one-indexed" instead?
Maybe also print top1 to point the user to the problem source
mjohnson541
force-pushed
the
Ndhr_improvements
branch
from
2b16fdc
to
0910964
Compare
This was referenced Dec 16, 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.
This has a number of numerical improvements for the calculation of ND classical and semiclassical hindered rotor partition functions. I've removed use of SmoothBivarateSpline interpolation as this resulted in some negative fitted internal reduced moment of inertia values and I've fixed issues with the integration routine.
Also includes miscellaneous Arkane improvements: lets Arkane deal with RateUncertainty objects, makes the semiclassicalND rotors example consistent with the other Toulene examples, and adds a error trigger in internal reduced moment of inertia calculations that prevents use of 0-indexed atom numbers that result in all kinds of weird outputs from the function.