Add cdf function to InterRDF #99
Conversation
Codecov Report
@@ Coverage Diff @@
## master #99 +/- ##
=========================================
Coverage ? 97.82%
=========================================
Files ? 11
Lines ? 644
Branches ? 78
=========================================
Hits ? 630
Misses ? 8
Partials ? 6
Continue to review full report at Codecov.
|
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
I don't like get_cdf(): either make it a function or calculate InterRDF.cdf and make it an attribute.
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
This looks ok.
However, you should also submit a PR to change the same thing in MDAnalysis so that the user interface stays as much the same as possible, namely MDAnalysis.analysis.rdf.InterRDF_s.get_cdf() – over there you'll need to deprecate the method. But we can discuss in the PR there.
|
(And please fix PEP8...) |
|
Please also address the remaining comments. |
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
Please make clear what was removed/changed/added. See comments.
Note that we are not bothering deprecating the get_cdf() method – PMDA does not have the same user base as MDA yet and we are in early development so we move quickly...
CHANGELOG
Outdated
| * add parallel density class (Issue #8) | ||
| * add `cdf` attribute to `pmda.rdf.InterRDF` for cumulative | ||
| distribution function | ||
| * deprecate `get_cdf` method in `pmda.rdf.InterRDF_s` and use |
There was a problem hiding this comment.
Put this in a separate category Changes
* The method rdf.InterRDF_s.get_cdf() was removed; just use the managed attribute
rdf.InterRDF_s.cdfThere was a problem hiding this comment.
In MDAnalysis we would tag it with a deprecation and keep it around for a while. In PMDA we can move a bit faster.
|
@orbeckst All required changes have been done. Please have a look. |
|
@VOD555 sorry, the PR fell off my radar. All looking good... will merge once Travis shows all green. |
|
The RDF tests actually failed. Not sure why. Can you please look into this, @VOD555 ? Perhaps changes in MDAnalysis? |
|
Other tests (on master https://travis-ci.org/MDAnalysis/pmda ...) also fail in RDF, so it's not a problem with your PR but the reference values are not correct anymore. Please have a look and decide what to do: either adjust the references or raise an issue for the upstream change that breaks our tests here. |
|
Hopefully the lint test passes now, too. |
|
I am rebasing to master and squashing commits locally. I will force push this branch with the rewritten history |
- update changelog - rename get_cdf to cdf and set it as a property - add version information for cdf - add more docs for cdf
- fix MDAnalysis#110 - The problem is due to the change in MDAnalysis that calculations within triclinic_box(), triclinic_vectors(), and box_volume() are performed in double precision. The test was calculating the RDF divided by the density for the molecule which was influenced by the precision of the volume value.
- disable pylint: unsubscriptable-object in leaflet
|
once this passes, do a simple merge |
2 participants
Changes made in this Pull Request:
PR Checklist