Fix InterRDF_s when given density=True by VOD555 · Pull Request #2812 · MDAnalysis/mdanalysis

VOD555 · 2020-07-01T20:58:47Z

Fixes #2811

Changes made in this Pull Request:

Fix number of atom pairs in InterRDF_s.

PR Checklist

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

codecov · 2020-07-01T23:37:28Z

Codecov Report

Merging #2812 into develop will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #2812      +/-   ##
===========================================
- Coverage    92.22%   92.21%   -0.01%     
===========================================
  Files          184      184              
  Lines        24141    24138       -3     
  Branches      3123     3123              
===========================================
- Hits         22263    22260       -3     
  Misses        1813     1813              
  Partials        65       65

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/rdf.py	`100.00% <100.00%> (ø)`

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 3e56b70...9046bf5. Read the comment docs.

orbeckst

Please add an entry to CHANGELOG.

Could you add note in the docs for InterRDF_s what the normalization is, namely, the mathematical equation for the site RDF?

package/MDAnalysis/analysis/rdf.py

orbeckst · 2020-07-02T02:02:58Z

Btw, there's an error in the reST for Suppose we have two AtomGroups A and B. A contains atom A1, A2, and B contains B1, B2. Give A and B to class:InterRDF, t (the "class" is missing a leading colon), see https://www.mdanalysis.org/docs/documentation_pages/analysis/rdf.html#average-radial-distribution-function

Could you please fix it while you're at it? Thanks.

pep8speaks · 2020-07-07T16:15:45Z

Hello @VOD555! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file package/MDAnalysis/analysis/rdf.py:

Line 417:80: E501 line too long (84 > 79 characters)

In the file testsuite/MDAnalysisTests/analysis/test_rdf_s.py:

Line 116:1: W391 blank line at end of file

Comment last updated at 2020-07-10 23:50:35 UTC

orbeckst · 2020-07-08T20:22:06Z

Add a section or paragraph at the top where you explain InterRDF_s with the equation.

…

Am 08.07.2020 um 11:05 schrieb Shujie Fan ***@***.***>: @VOD555 commented on this pull request. In package/MDAnalysis/analysis/rdf.py: > indices.append([ag1.indices, ag2.indices]) # Average number density box_vol = self.volume / self.n_frames - density = N / box_vol Where should I put the equation, at the top below the g_ab(r) or in the texts for InterRDF_s? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

orbeckst

There's only a horizontal rule that needs to stay in CHANGELOG.

While you're at it, can you please fix/update some of the reST? Thanks.

for MDAnalysis.analysis.rdf.InterRDF_s.get_cdf and make sure that the math and Returns display properly (needs a r""" at the beginning and perhaps a blank line).
add a section Attributes to InterRDF and InterRDF_s and list the attributes in which results are stored bins, rdf, edges, count and bins, rdf, edges, count, indices, cdf (I think). See our doc guidelines or examples how to add Attributes.

The other comments can be addressed after this PR is merged.

package/CHANGELOG

orbeckst · 2020-07-10T17:12:45Z

package/MDAnalysis/analysis/rdf.py

+In :class:`InterRDF_s`, we provide an option `density`. When `density` is
+`True`, it will return the RDF :math:`g_{ab}(r)`; when `density` is False`,
+it will return the density of particle :math:`b` in a shell at distance
+:math:`r` around a :math:`a` particle, which is
+
+.. math::
+   n_{ab}(r) = \rho g_{ab}(r)


This is confusing: if we set density=False then we get a density?!?

~~We should then change the kwarg setting to density=True to get the density.~~

I want to add the fix to the backport PR #2798 so we cannot change the defaults as this can break people's code.

We can make a second PR to change this.

orbeckst · 2020-07-10T17:16:09Z

package/MDAnalysis/analysis/rdf.py

+          Calculate :math:`g_{ab}(r)` if set to ``True``; or calculate
+          :math:`n_{ab}(r)` if set to ``false``; the default is
+          ``True``.


~~Reverse so that density=True calculates the number density n_ab(r).~~

I want to add the fix to the backport PR #2798 so we cannot change the defaults as this can break people's code.

We can make a second PR to change this.

orbeckst · 2020-07-10T20:43:51Z

To be used for squashed commit message

* Fixes #2811
* enable hitherto undocumented kwarg density for InterRDF_s (but changed 
   meaning of True/False):
   If you used density=False since 0.19.0 to calculate a single particle density then you 
   now have to use density=True. The default remains the same: calculate the RDF and not
   the density.
* Fix normalization for InterRDF_s(..., density=True)
* add test to compare InterRDF and InterRDF_s
* add description for density option
* update CHANGELOG

orbeckst · 2020-07-10T20:46:06Z

@VOD555, I'll quickly add the doc fix ups while I am merging...

orbeckst

@VOD555 can you please have a look at my comments and reply? I'll block the merge until you're ok with my changes and we're clear on what we're doing.

orbeckst · 2020-07-10T22:20:15Z

package/CHANGELOG

+  * rdf.InterRDF_s density keyword documented and now gives correct results for
+    density=True; the keyword was available since 0.19.0 but with incorrect
+    semantics and not documented and did not produce correct results (Issue
+    #2811, PR #2812)


I changed the meaning of the density keyword so that it makes more sense. Given that we never documented it, this should be ok and I will classify it as a fix so that it can go into 1.0.1 PR #2798

orbeckst · 2020-07-10T22:21:02Z

package/MDAnalysis/analysis/rdf.py

+In :class:`InterRDF_s`, we provide an option `density`. When `density` is
+``False``, it will return the RDF :math:`g_{ab}(r)`; when `density` is
+``True``, it will return the density of particle :math:`b` in a shell at
+distance :math:`r` around a :math:`a` particle, which is


Note that density=False now means "calculate g(r)" and density=True means "calculate density".

So we still need another PR to change the default density, right?

No, this PR switched the behavior.

See comment above #2812 (comment)

package/MDAnalysis/analysis/rdf.py

orbeckst · 2020-07-10T22:25:47Z

package/MDAnalysis/analysis/rdf.py

+        This is the :ref:`cumulative count<equation-countab>` within a given
+        radius, i.e., :math:`N_{ab}(r)`.


Why are we calculating the counts and calling it CDF? This is confusing...

I made it clear in the text but still: why did we do this instead of having separate methods that just do the thing that they are named after?

Yes, we should have seperate functions to give CDF and cummulative N.

Ok, new issue.

@VOD555 can you raise issues for MDAnalysis and PMDA, please?

testsuite/MDAnalysisTests/analysis/test_rdf_s.py

- fix #2811 (see also MDAnalysis/pmda#120) - add test to compare InterRDF and InterRDF_s - add description for density option (was not documented since 0.19.0) - updated docs for rdf - updated docs with equationi - attributes for InterRDF and InterRDF_s - reference equations - minor grammar/style fixes - remove ill-advised usage of InterRDF_s from docs - update CHANGELOG

- fix (because it was not documented before) - density=True now means "calculate the single site density"; default is still "calculate RDF" (now meaning density=False) - update docs - update tests (add test for default behavior) - update CHANGELOG

orbeckst · 2020-07-10T23:50:59Z

I rewrote the history so that it can be nicely merged.

VOD555 changed the title ~~Fix rdf density~~ Fix InterRDF_s when given density=True Jul 1, 2020

orbeckst requested changes Jul 2, 2020

View reviewed changes

package/MDAnalysis/analysis/rdf.py Show resolved Hide resolved

orbeckst requested changes Jul 10, 2020

View reviewed changes

orbeckst approved these changes Jul 10, 2020

View reviewed changes

orbeckst added this to the 1.0.x milestone Jul 10, 2020

orbeckst self-assigned this Jul 10, 2020

orbeckst requested changes Jul 10, 2020

View reviewed changes

VOD555 and others added 2 commits July 10, 2020 16:48

orbeckst approved these changes Jul 10, 2020

View reviewed changes

orbeckst merged commit 8704511 into MDAnalysis:develop Jul 12, 2020

orbeckst mentioned this pull request Jul 12, 2020

Backport 1.0.1 #2798

Merged

4 tasks

yuxuanzhuang mentioned this pull request Aug 19, 2020

Refactor pmda after universe can be serialized MDAnalysis/pmda#132

Open

13 tasks

VOD555 mentioned this pull request Aug 23, 2020

cdf function in rdf.InterRDF_s does not give cummulative distribution function MDAnalysis/pmda#130

Open

fiona-naughton added defect Component-Analysis labels Sep 25, 2023

		This is the :ref:`cumulative count<equation-countab>` within a given
		radius, i.e., :math:`N_{ab}(r)`.

Conversation

VOD555 commented Jul 1, 2020 • edited by orbeckst Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

codecov bot commented Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

orbeckst commented Jul 2, 2020

Uh oh!

pep8speaks commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-07-10 23:50:35 UTC

Uh oh!

orbeckst commented Jul 8, 2020 via email

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orbeckst commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orbeckst commented Jul 10, 2020

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

orbeckst commented Jul 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

VOD555 commented Jul 1, 2020 •

edited by orbeckst

Loading

codecov bot commented Jul 1, 2020 •

edited

Loading

pep8speaks commented Jul 7, 2020 •

edited

Loading

orbeckst commented Jul 10, 2020 •

edited

Loading