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

Update global uncertainty module to work with MUQ 2 #1738

Merged
merged 8 commits into from
Dec 10, 2019
Merged

Update global uncertainty module to work with MUQ 2 #1738

merged 8 commits into from
Dec 10, 2019

Conversation

mliu49
Copy link
Contributor

@mliu49 mliu49 commented Oct 1, 2019

Motivation or Problem

The global uncertainty module of RMG previously used MUQ 1, relying on a semi-functional conda binary which only worked on Linux. MUQ 1 has since been deprecated in favor of MUQ 2.

Instead of updating the MUQ 1 binary to work with Python 3, I instead chose to build a new MUQ 2 binary from scratch.

Description of Changes

This PR implements Python 3, RMG py3, and MUQ 2 transition changes in the rmgpy.tools.globaluncertainty module (renamed from rmgpy.tools.muq).

This also updates the related IPython notebook.

Note: This is currently based on #1735.

Testing

I have run a couple of examples and the IPython notebook, so everything appears to be working.

I have not yet done any benchmarks to see if the results are the same as before.

To Do

  • Compile mac binaries and test on mac

@mliu49 mliu49 self-assigned this Oct 1, 2019
@mliu49 mliu49 mentioned this pull request Oct 1, 2019
17 tasks
@codecov
Copy link

codecov bot commented Oct 1, 2019

Codecov Report

Merging #1738 into master will decrease coverage by <.01%.
The diff coverage is 7.55%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1738      +/-   ##
=========================================
- Coverage    44.2%   44.2%   -0.01%     
=========================================
  Files          83      83              
  Lines       21533   21529       -4     
  Branches     5645    5644       -1     
=========================================
- Hits         9519    9517       -2     
+ Misses      10961   10945      -16     
- Partials     1053    1067      +14
Impacted Files Coverage Δ
rmgpy/rmg/input.py 42.64% <ø> (ø) ⬆️
rmgpy/rmg/main.py 21.62% <0%> (ø) ⬆️
rmgpy/tools/globaluncertainty.py 7.72% <7.72%> (ø)
rmgpy/data/statmech.py 42.2% <0%> (ø) ⬆️
rmgpy/rmg/pdep.py 12.21% <0%> (ø) ⬆️
arkane/kinetics.py 12.14% <0%> (ø) ⬆️
rmgpy/data/kinetics/family.py 49.06% <0%> (ø) ⬆️
rmgpy/statmech/ndTorsions.py 59.78% <0%> (ø) ⬆️
rmgpy/yml.py 15.71% <0%> (ø) ⬆️
arkane/sensitivity.py 10% <0%> (ø) ⬆️
... and 2 more

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 c46befa...2d5332a. Read the comment docs.

Copy link
Contributor

@mjohnson541 mjohnson541 left a comment

Choose a reason for hiding this comment

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

This looks mostly fine to me. Doesn't the documentation need updated as a result of the uncertainty option changes?

Copy link
Member

@amarkpayne amarkpayne left a comment

Choose a reason for hiding this comment

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

A couple of minor things, but the notebook appears to work as promised, so I think this is mostly good to go. I also think that the documentation might need updating. In the future, we should also add some unit tests for the global uncertainty module

rmgpy/tools/globaluncertainty.py Outdated Show resolved Hide resolved
ipython/global_uncertainty.ipynb Outdated Show resolved Hide resolved
ipython/global_uncertainty.ipynb Outdated Show resolved Hide resolved
ipython/global_uncertainty.ipynb Outdated Show resolved Hide resolved
ipython/global_uncertainty.ipynb Show resolved Hide resolved
rmgpy/tools/globaluncertainty.py Outdated Show resolved Hide resolved
rmgpy/tools/globaluncertainty.py Outdated Show resolved Hide resolved
rmgpy/tools/globaluncertainty.py Outdated Show resolved Hide resolved
rmgpy/tools/globaluncertainty.py Outdated Show resolved Hide resolved
rmgpy/tools/globaluncertainty.py Outdated Show resolved Hide resolved
@mliu49 mliu49 force-pushed the muq2 branch 2 times, most recently from 580a9df to 924a1c4 Compare December 9, 2019 18:40
@amarkpayne
Copy link
Member

Everything except the extraneous character in the docstring looks good. Rebase and I'll merge

@mliu49
Copy link
Contributor Author

mliu49 commented Dec 9, 2019

Rebased.

@amarkpayne amarkpayne merged commit 0d63c33 into master Dec 10, 2019
@amarkpayne amarkpayne deleted the muq2 branch December 10, 2019 02:05
@amarkpayne
Copy link
Member

Thanks for the PR @mliu49 !

This was referenced Dec 13, 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.

3 participants