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

Revamping of PMF #327

Merged
merged 64 commits into from
Feb 19, 2020
Merged

Revamping of PMF #327

merged 64 commits into from
Feb 19, 2020

Conversation

mrshirts
Copy link
Collaborator

@mrshirts mrshirts commented Feb 9, 2020

Here's my proposed revamping of PMF functionality. I'd love to get comments so we can merge this in for pymbar4. I anticipate there may be some requested changes, so I'd love to get started on them.

@Lnaden
Copy link
Contributor

Lnaden commented Feb 13, 2020

Be more specific what I should look at when you say "check the test coverage".

I mean we should make sure the tests are updated in a way to maximally hit lines of code, especially since this is a new module. The existing tests I think just run through the PMF computation, but not necessarily all the functions and code paths.

We can do this in another PR to avoid making this one too large, especially since I want to change from Nose -> Pytest

@mrshirts
Copy link
Collaborator Author

We can do this in another PR to avoid making this one too large, especially since I want to change from Nose -> Pytest

OK, added issues to switch testing frameworks (#330, @Lnaden ), and after to add more PMF tests (#331 @mrshirts )

@Lnaden
Copy link
Contributor

Lnaden commented Feb 13, 2020

I'm of the opinion lets get this in, then we can work on creating tests and whatnot after I handle #330. I have some time today so I'd like to get the test framework switched over, and start on #332 as well.

Since this is all on the pymbar4 branch, I feel safe making rapid changes now, improve the CI/test/coverage framework, then going back and ensuring we're covering all our bases.

@mrshirts
Copy link
Collaborator Author

@jchodera Do you want to take an overall look at how this is structured? There's a couple of things that need to be done with the usability of the MCMC sampling of parameters, but it would be good to know if there are any showstoppers about the general usability and way it is presented. Check the examples to see how PMF's are now generated for most users.

@mrshirts
Copy link
Collaborator Author

mrshirts commented Feb 16, 2020

mbar_computePMF test obviously failing, since the function doesn't exist.

Should be completely fixed in the near future by adding a test_pmf.py file (test_bar.py and test_exp.py need to be done as well!), but for now I have removed the broken function call.

@Lnaden
Copy link
Contributor

Lnaden commented Feb 17, 2020

Great! If this is good to go in your book, go ahead and merge!

@mrshirts
Copy link
Collaborator Author

Note: address issue with a deprecated KDE functionality being used.

@codecov
Copy link

codecov bot commented Feb 19, 2020

Codecov Report

❗ No coverage uploaded for pull request base (pymbar4@adadee3). Click here to learn what that means.
The diff coverage is 41.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##             pymbar4     #327   +/-   ##
==========================================
  Coverage           ?   42.92%           
==========================================
  Files              ?       14           
  Lines              ?     2947           
  Branches           ?        0           
==========================================
  Hits               ?     1265           
  Misses             ?     1682           
  Partials           ?        0
Flag Coverage Δ
#unittests 42.92% <41.93%> (?)
Impacted Files Coverage Δ
pymbar/testsystems/gaussian_work.py 12.5% <ø> (ø)
pymbar/exp.py 26% <ø> (ø)
pymbar/old_mbar.py 20.91% <0%> (ø)
pymbar/bar.py 9.43% <0%> (ø)
pymbar/confidenceintervals.py 3.43% <0%> (ø)
pymbar/mbar_solvers.py 86.66% <100%> (ø)
pymbar/utils.py 66.66% <20%> (ø)
pymbar/pmf.py 45.13% <57.14%> (ø)
pymbar/mbar.py 73.37% <61.9%> (ø)
pymbar/utils_for_testing.py 89.13% <83.33%> (ø)

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 adadee3...070ec1e. Read the comment docs.

@mrshirts mrshirts mentioned this pull request Feb 19, 2020
@mrshirts
Copy link
Collaborator Author

OK, failed because the conda-recipe (I think?) doesn't have sklearn. So it will need to be added to run kernel density calculations.

@mrshirts
Copy link
Collaborator Author

Let's go with merging this now with basic tests now implemented?

@Lnaden Lnaden merged commit 59c0d72 into pymbar4 Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants