Skip to content
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

ND Hindered Rotor Improvements #1849

Merged
merged 6 commits into from
Dec 16, 2019
Merged

ND Hindered Rotor Improvements #1849

merged 6 commits into from
Dec 16, 2019

Conversation

mjohnson541
Copy link
Contributor

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.

@codecov
Copy link

codecov bot commented Dec 13, 2019

Codecov Report

Merging #1849 into master will increase coverage by 0.02%.
The diff coverage is 90%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
rmgpy/statmech/ndTorsions.py 61% <90%> (+1.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe25b90...0910964. Read the comment docs.

Copy link
Member

@alongd alongd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good, I have some minor comments

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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove redundant parentheses around self.rootD(*phis)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

rmgpy/statmech/ndTorsions.py Show resolved Hide resolved
rmgpy/statmech/conformer.pyx Show resolved Hide resolved
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
@@ -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.')
Copy link
Member

@alongd alongd Dec 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the message say "should be one-indexed" instead?
Maybe also print top1 to point the user to the problem source

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XD...yes done

Copy link
Member

@alongd alongd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@alongd alongd merged commit 7aa8bfd into master Dec 16, 2019
@alongd alongd deleted the Ndhr_improvements branch December 16, 2019 19:44
This was referenced Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants