-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add InterRDF_s #70
Conversation
@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." |
For the linter error: use the same trick as in Line 90 in a718250
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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
|
||
@pytest.fixture(scope='module') | ||
def rdf_s(u, sels): | ||
return InterRDF_s(u, sels).run() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
pmda/test/test_rdf_s.py
Outdated
# 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) |
There was a problem hiding this comment.
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
@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) |
There was a problem hiding this 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]) |
There was a problem hiding this comment.
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?
pmda/test/test_rdf_s.py
Outdated
|
||
|
||
@pytest.fixture(scope='module') | ||
def rdf_s(u, sels): |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.)
Gets error with =========================================================================================== 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes #60
Changes made in this Pull Request:
PR Checklist