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

Add InterRDF_s #70

Merged
merged 20 commits into from
Nov 2, 2018
Merged

Add InterRDF_s #70

merged 20 commits into from
Nov 2, 2018

Conversation

VOD555
Copy link
Collaborator

@VOD555 VOD555 commented Oct 10, 2018

Fixes #60

Changes made in this Pull Request:

  • Add parallel version of InterRDF_s(in pmda.rdf)
  • Add test for InterRDF_s

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@orbeckst
Copy link
Member

@VOD555 if Travis fails, please check. This time the reason was missing cython for MDA devel installations. I merged master and this might fix it but in general, say something if CI fails due to issues outside the PR so that this can be fixed and your PR be moved along. In general, no-one bothers reviewing if there's still a red CI fail mark because experienced PR authors are expected to fix all their failures on their own. Exceptions are "cries for help" along the lines "Please have a look, I need more eyes to find this issue or solve this problem."

@orbeckst
Copy link
Member

For the linter error: use the same trick as in

# pylint: disable=redefined-builtin

@codecov
Copy link

codecov bot commented Oct 27, 2018

Codecov Report

Merging #70 into master will increase coverage by 0.39%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
+ Coverage   96.75%   97.14%   +0.39%     
==========================================
  Files           8        8              
  Lines         431      491      +60     
  Branches       60       70      +10     
==========================================
+ Hits          417      477      +60     
  Misses          8        8              
  Partials        6        6
Impacted Files Coverage Δ
pmda/rdf.py 100% <100%> (ø) ⬆️

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 aaa478c...60a000d. Read the comment docs.

@VOD555 VOD555 requested a review from orbeckst October 27, 2018 03:13
@VOD555 VOD555 self-assigned this Oct 27, 2018
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Looking good. Minor comments

  • add to CHANGELOG
  • test with different n_blocks

pmda/rdf.py Show resolved Hide resolved
pmda/rdf.py Show resolved Hide resolved

@pytest.fixture(scope='module')
def rdf_s(u, sels):
return InterRDF_s(u, sels).run()
Copy link
Member

Choose a reason for hiding this comment

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

Can you test with different n_blocks?

I don't know if this is actually testing multiple blocks or not. We need to know that the "accumulator" is working with multiple blocks.

Copy link
Member

Choose a reason for hiding this comment

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

Should work with @pytest.mark.parametrize.

# should see same results from analysis.rdf.InterRDF_s
# and pmda.rdf.InterRDF_s
nrdf = rdf.InterRDF_s(u, sels).run()
prdf = InterRDF_s(u, sels).run(n_jobs=3)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@orbeckst Here is one that is testing multiple blocks. Maybe I should add one test for "n_blocks=1" case

@VOD555
Copy link
Collaborator Author

VOD555 commented Oct 29, 2018

@orbeckst An error is raised on the use of the get= keyword in the new version of dask(http://docs.dask.org/en/latest/changelog.html)

@orbeckst
Copy link
Member

orbeckst commented Oct 29, 2018

That happened quickly!

So we now HAVE to finish PR #66!

EDIT: "quickly" because PR #66 talked about updated to Dask 0.18 and now we have 0.20 and it's an error.

EDIT 2: Fix referenced PR: not #60 but #66

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

More testing, otherwise looking good.

# and pmda.rdf.InterRDF_s
nrdf = rdf.InterRDF_s(u, sels).run()
prdf = InterRDF_s(u, sels).run(n_blocks=n_blocks)
assert_almost_equal(nrdf.count[0][0][0], prdf.count[0][0][0])
Copy link
Member

Choose a reason for hiding this comment

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

Why compare just one element? Shouldn't

assert_almost_equal(nrdf.count, prdf.count)

work?



@pytest.fixture(scope='module')
def rdf_s(u, sels):
Copy link
Member

Choose a reason for hiding this comment

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

Test with different schedulers:

def rdf_s(u, sels, scheduler)

Copy link
Member

Choose a reason for hiding this comment

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

(The scheduler fixture is defined in conftest.py and should run this calculation with all dask schedulers.)

@VOD555
Copy link
Collaborator Author

VOD555 commented Nov 1, 2018

Gets error with distributed

=========================================================================================== test session starts ============================================================================================
platform linux -- Python 3.6.5, pytest-3.6.2, py-1.5.3, pluggy-0.6.0 -- /home/shujie/anaconda3/envs/py36/bin/python
cachedir: ../../.pytest_cache
rootdir: /home/shujie/pmda, inifile: setup.cfg
plugins: pep8-1.0.6, cov-2.5.1, hypothesis-3.66.1
collected 38 items                                                                                                                                                                                         

test_rdf_s.py::test_nbins PASSED                                                                                                                                                                     [  2%]
test_rdf_s.py::test_range PASSED                                                                                                                                                                     [  5%]
test_rdf_s.py::test_count_size[distributed-1] ERROR                                                                                                                                                  [  7%]
test_rdf_s.py::test_count[distributed-1] ERROR                                                                                                                                                       [ 10%]
test_rdf_s.py::test_double_run[distributed-1] ERROR                                                                                                                                                  [ 13%]
test_rdf_s.py::test_cdf[distributed-1] ERROR                                                                                                                                                         [ 15%]
test_rdf_s.py::test_reduce[distributed-1] ERROR                                                                                                                                                      [ 18%]
test_rdf_s.py::test_count_size[multiprocessing-1] PASSED                                                                                                                                             [ 21%]
test_rdf_s.py::test_count[multiprocessing-1] PASSED                                                                                                                                                  [ 23%]
test_rdf_s.py::test_double_run[multiprocessing-1] PASSED                                                                                                                                             [ 26%]
test_rdf_s.py::test_cdf[multiprocessing-1] PASSED                                                                                                                                                    [ 28%]
test_rdf_s.py::test_reduce[multiprocessing-1] PASSED                                                                                                                                                 [ 31%]
test_rdf_s.py::test_count_size[multiprocessing-2] PASSED                                                                                                                                             [ 34%]
test_rdf_s.py::test_count[multiprocessing-2] PASSED                                                                                                                                                  [ 36%]
test_rdf_s.py::test_double_run[multiprocessing-2] PASSED                                                                                                                                             [ 39%]
test_rdf_s.py::test_cdf[multiprocessing-2] PASSED                                                                                                                                                    [ 42%]
test_rdf_s.py::test_reduce[multiprocessing-2] PASSED                                                                                                                                                 [ 44%]
test_rdf_s.py::test_count_size[distributed-2] ERROR                                                                                                                                                  [ 47%]
test_rdf_s.py::test_count[distributed-2] ERROR                                                                                                                                                       [ 50%]
test_rdf_s.py::test_double_run[distributed-2] ERROR                                                                                                                                                  [ 52%]
test_rdf_s.py::test_cdf[distributed-2] ERROR                                                                                                                                                         [ 55%]
test_rdf_s.py::test_reduce[distributed-2] ERROR                                                                                                                                                      [ 57%]
test_rdf_s.py::test_count_size[single-threaded-2] PASSED                                                                                                                                             [ 60%]
test_rdf_s.py::test_count[single-threaded-2] PASSED                                                                                                                                                  [ 63%]
test_rdf_s.py::test_double_run[single-threaded-2] PASSED                                                                                                                                             [ 65%]
test_rdf_s.py::test_cdf[single-threaded-2] PASSED                                                                                                                                                    [ 68%]
test_rdf_s.py::test_reduce[single-threaded-2] PASSED                                                                                                                                                 [ 71%]
test_rdf_s.py::test_count_size[single-threaded-1] PASSED                                                                                                                                             [ 73%]
test_rdf_s.py::test_count[single-threaded-1] PASSED                                                                                                                                                  [ 76%]
test_rdf_s.py::test_double_run[single-threaded-1] PASSED                                                                                                                                             [ 78%]
test_rdf_s.py::test_cdf[single-threaded-1] PASSED                                                                                                                                                    [ 81%]
test_rdf_s.py::test_reduce[single-threaded-1] PASSED                                                                                                                                                 [ 84%]
test_rdf_s.py::test_same_result[1] PASSED                                                                                                                                                            [ 86%]
test_rdf_s.py::test_same_result[2] PASSED                                                                                                                                                            [ 89%]
test_rdf_s.py::test_same_result[3] PASSED                                                                                                                                                            [ 92%]
test_rdf_s.py::test_same_result[4] PASSED                                                                                                                                                            [ 94%]
test_rdf_s.py::test_density[True-13275.775528444701] PASSED                                                                                                                                          [ 97%]
test_rdf_s.py::test_density[False-0.021915460340071267] PASSED                                                                                                                                       [100%]

================================================================================================== ERRORS ==================================================================================================

pmda/rdf.py Outdated
super(InterRDF_s, self).__init__(u, atomgroups)

# List of pairs of AtomGroups
self.ags = ags
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, the failure is due to this line. distributed cannot pass atomgroups to blocks.

Copy link
Member

Choose a reason for hiding this comment

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

Seems related to PR #65 .

(Also, thanks for raising #79 )

@orbeckst orbeckst merged commit f3bbd13 into MDAnalysis:master Nov 2, 2018
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