Make classes in rdf use the Results class by VOD555 · Pull Request #3284 · MDAnalysis/mdanalysis

VOD555 · 2021-05-07T03:48:17Z

Fixes #3276

Changes made in this Pull Request:

use self.results instead of self. to save data

PR Checklist

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

pep8speaks · 2021-05-07T03:48:19Z

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 523:27: E221 multiple spaces before operator
Line 524:80: E501 line too long (83 > 79 characters)
Line 541:80: E501 line too long (92 > 79 characters)

Comment last updated at 2021-05-11 12:19:07 UTC

codecov · 2021-05-07T05:21:06Z

Codecov Report

Merging #3284 (c814829) into develop (6d5ef34) will decrease coverage by 0.73%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #3284      +/-   ##
===========================================
- Coverage    93.56%   92.83%   -0.74%     
===========================================
  Files          176      171       -5     
  Lines        22837    22386     -451     
  Branches      3195     3195              
===========================================
- Hits         21368    20781     -587     
+ Misses        1418     1063     -355     
- Partials        51      542     +491

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/rdf.py	`100.00% <100.00%> (ø)`
package/MDAnalysis/analysis/encore/covariance.py	`75.38% <0.00%> (-12.31%)`	⬇️
...ckage/MDAnalysis/analysis/encore/confdistmatrix.py	`67.41% <0.00%> (-10.12%)`	⬇️
package/MDAnalysis/coordinates/DMS.py	`88.40% <0.00%> (-7.25%)`	⬇️
...nalysis/analysis/hydrogenbonds/hbond_autocorrel.py	`87.66% <0.00%> (-7.21%)`	⬇️
package/MDAnalysis/converters/OpenMMParser.py	`92.85% <0.00%> (-7.15%)`	⬇️
package/MDAnalysis/coordinates/GMS.py	`85.61% <0.00%> (-6.85%)`	⬇️
package/MDAnalysis/lib/transformations.py	`78.56% <0.00%> (-6.76%)`	⬇️
package/MDAnalysis/topology/HoomdXMLParser.py	`78.72% <0.00%> (-6.39%)`	⬇️
package/MDAnalysis/coordinates/CRD.py	`79.20% <0.00%> (-5.95%)`	⬇️
... and 93 more

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 6d5ef34...c814829. Read the comment docs.

PicoCentauri

Good changes and thanks for helping with the results attribute. The code is also easier readable now. I have some comments for improving the docs.

Please add a .. versionchanged:: 2.0.0 describing that the attributes are now stored in the
results attribute. If you want you can put a .. code-block:: python for decorating the examples.

PicoCentauri · 2021-05-07T07:31:47Z

package/MDAnalysis/analysis/rdf.py

+      .. deprecated:: 2.0.0
+          This attribute will be removed in 3.0.0.
+          Use :attr:`results.bins` instead.
+


PicoCentauri · 2021-05-07T07:32:11Z

package/MDAnalysis/analysis/rdf.py

+      .. deprecated:: 2.0.0
+          This attribute will be removed in 3.0.0.
+          Use :attr:`results.rdf` instead.
+


PicoCentauri · 2021-05-07T07:32:17Z

package/MDAnalysis/analysis/rdf.py

+      .. deprecated:: 2.0.0
+          This attribute will be removed in 3.0.0.
+          Use :attr:`results.cdf` instead.


PicoCentauri · 2021-05-07T07:32:38Z

package/MDAnalysis/analysis/rdf.py

+         .. deprecated:: 2.0.0
+             This attribute will be removed in 3.0.0.
+             Use :attr:`results.rdf` instead.
+


PicoCentauri · 2021-05-07T07:32:53Z

package/MDAnalysis/analysis/rdf.py

+       .. deprecated:: 2.0.0
+           This attribute will be removed in 3.0.0.
+           Use :attr:`results.bins` instead.
+


Should we remove the attribute descriptions given here and put them directly in the docstring of the class? As for example in the GNMAnalysis:

mdanalysis/package/MDAnalysis/analysis/gnm.py

Lines 225 to 232 in da46e84

Attributes

----------

results.times : numpy.ndarray

simulation times used in analysis

results.eigenvalues : numpy.ndarray

calculated eigenvalues

results.eigenvectors : numpy.ndarray

calculated eigenvectors

I think long term yes, but given the current short deadlines, I'd be inclined to just say let's keep things here and then we can re-format in 2.1.0.

@PicoCentauri , if you want to raise or find an issue regarding consistent formatting of analysis classes (which will presumably also aid mdacli in documentation extraction) then please do so and start adding TODOs...

PicoCentauri · 2021-05-07T07:37:10Z

package/MDAnalysis/analysis/rdf.py

-        vol = np.power(self.edges[1:], 3) - np.power(self.edges[:-1], 3)
-        vol *= 4/3.0 * np.pi
+        vols = np.power(self.edges, 3)
+        vol = 4/3.0 * np.pi * (vols[1:] - vols[:-1])


A 3 is enough in Py3

PicoCentauri · 2021-05-07T07:41:02Z

package/MDAnalysis/analysis/rdf.py

        count, edges = np.histogram([-1], **self.rdf_settings)
-        count_list = [np.zeros((ag1.n_atoms, ag2.n_atoms, len(count)), dtype=np.float64)
+        l = len(count)
+        count_ini = [np.zeros((ag1.n_atoms, ag2.n_atoms, l), dtype=np.float64)


Maybe declare this directly as self.count?

PicoCentauri · 2021-05-07T07:42:38Z

package/MDAnalysis/analysis/rdf.py

        # Results stored in self.cdf
        # self.cdf is a list of cdf between pairs of AtomGroups in ags


# Results stored in self.results.cdf # self.resulrs.cdf is a list of cdf between pairs of AtomGroups in ags

IAlibay

Kinda echo @PicoCentauri although I'd suggest putting versionadded results.attributes entries and then a versionchanged in the class itself.

IAlibay · 2021-05-07T10:22:54Z

package/MDAnalysis/analysis/rdf.py

+       .. deprecated:: 2.0.0
+           This attribute will be removed in 3.0.0.
+           Use :attr:`results.bins` instead.
+


I think long term yes, but given the current short deadlines, I'd be inclined to just say let's keep things here and then we can re-format in 2.1.0.

IAlibay · 2021-05-07T10:25:20Z

package/MDAnalysis/analysis/rdf.py

+           This attribute will be removed in 3.0.0.
+           Use :attr:`results.bins` instead.
+
   .. attribute:: edges


Shouldn't edges and count be results? (I'm not super familiar with the MDA RDF classes so maybe someone like @orbeckst or @fiona-naughton can comment here)

Yes, you need them for plotting and downstream processing.

package/MDAnalysis/analysis/rdf.py

PicoCentauri

I got just two suggestions to squash the code a bit. Otherwise I'm happy.

PicoCentauri · 2021-05-07T22:39:54Z

testsuite/MDAnalysisTests/analysis/test_rdf.py

+def test_rdf_attr_warning(sels):
+    s1, s2 = sels
+    rdf = InterRDF(s1, s2).run()
+
+    wmsg = "The `rdf` attribute was deprecated in MDAnalysis 2.0.0"
+    with pytest.warns(DeprecationWarning, match=wmsg):
+        assert_equal(rdf.rdf, rdf.results.rdf)
+
+    wmsg = "The `bins` attribute was deprecated in MDAnalysis 2.0.0"
+    with pytest.warns(DeprecationWarning, match=wmsg):
+        assert_equal(rdf.bins, rdf.results.bins)
+
+    wmsg = "The `edges` attribute was deprecated in MDAnalysis 2.0.0"
+    with pytest.warns(DeprecationWarning, match=wmsg):
+        assert_equal(rdf.edges, rdf.results.edges)
+
+    wmsg = "The `count` attribute was deprecated in MDAnalysis 2.0.0"
+    with pytest.warns(DeprecationWarning, match=wmsg):
+        assert_equal(rdf.count, rdf.results.count)


To save some space you can exchange this with

@pytest.mark.parametrize("attr", ("rdf", "bins", "edges", "count")) def test_rdf_attr_warning(sels, attr): s1, s2 = sels rdf = InterRDF(s1, s2).run() wmsg = f"The `{attr}` attribute was deprecated in MDAnalysis 2.0.0" with pytest.warns(DeprecationWarning, match=wmsg): getattr(rdf, attr) is rdf.results.[attr]

PicoCentauri · 2021-05-07T22:42:01Z

testsuite/MDAnalysisTests/analysis/test_rdf_s.py

+def test_rdf_attr_warning(rdf):
+    wmsg = "The `rdf` attribute was deprecated in MDAnalysis 2.0.0"
+    with pytest.warns(DeprecationWarning, match=wmsg):
+        assert_equal(rdf.rdf, rdf.results.rdf)
+
+    wmsg = "The `bins` attribute was deprecated in MDAnalysis 2.0.0"
+    with pytest.warns(DeprecationWarning, match=wmsg):
+        assert_equal(rdf.bins, rdf.results.bins)
+
+    wmsg = "The `edges` attribute was deprecated in MDAnalysis 2.0.0"
+    with pytest.warns(DeprecationWarning, match=wmsg):
+        assert_equal(rdf.edges, rdf.results.edges)
+
+    wmsg = "The `count` attribute was deprecated in MDAnalysis 2.0.0"
+    with pytest.warns(DeprecationWarning, match=wmsg):
+        assert_equal(rdf.count, rdf.results.count)
+
+    rdf.get_cdf()
+    wmsg = "The `cdf` attribute was deprecated in MDAnalysis 2.0.0"
+    with pytest.warns(DeprecationWarning, match=wmsg):
+        assert_equal(rdf.cdf, rdf.results.cdf)


See above

@pytest.mark.parametrize("attr", ("rdf", "bins", "edges", "count", "cdf")) def test_rdf_attr_warning(rdf, attr): if attr == "cdf": rdf.get_cdf() wmsg = f"The `{attr}` attribute was deprecated in MDAnalysis 2.0.0" with pytest.warns(DeprecationWarning, match=wmsg): getattr(rdf, attr) is rdf.results.[attr]

PicoCentauri

LGTM. Let's wait for @orbeckst and @IAlibay.

IAlibay

Sorry for the short messages, only have time for a really quick review.

package/MDAnalysis/analysis/rdf.py

IAlibay · 2021-05-07T23:55:16Z

package/MDAnalysis/analysis/rdf.py

      ``(len(A), len(B))``, i.e., a stack of RDFs. For example,
-      ``rdf[i][0, 2]`` is the RDF between atoms ``A[0]`` and ``B[2]``.
+      ``results.rdf[i][0, 2]`` is the RDF between atoms ``A[0]``
+      and ``B[2]``.


IAlibay · 2021-05-07T23:55:24Z

package/MDAnalysis/analysis/rdf.py

-      counts.
+      raw counts, for all :attr:`results.bins`. The data have the same
+      structure as :attr:`results.rdf` except that the arrays contain
+      the raw counts.


IAlibay · 2021-05-07T23:55:33Z

package/MDAnalysis/analysis/rdf.py

+      have the same structure as :attr:`results.rdf` except that the arrays
+      contain the cumulative counts.

      This attribute only exists after :meth:`get_cdf` has been run.


IAlibay · 2021-05-07T23:56:59Z

package/MDAnalysis/analysis/rdf.py

       removed. These should instead be passed to :meth:`InterRDF.run`.

+    .. versionchanged:: 2.0.0
+       Use :class:`~MDAnalysis.analysis.AnalysisBase` as parent class and


Was AnalysisBase not the parent class before 2.0.0?

IAlibay · 2021-05-07T23:58:07Z

package/MDAnalysis/analysis/rdf.py

       removed. These should instead be passed to :meth:`InterRDF_s.run`.

+    .. versionchanged:: 2.0.0
+       Use :class:`~MDAnalysis.analysis.AnalysisBase` as parent class and


Same question as above.

* Fixes #3290 * Switch DistanceMatrix to Results class * add tests for deprecations * Updated CHANGELOG

orbeckst · 2021-05-08T01:02:40Z

Something weird happended: I tried to resolve conflicts between develop and this branch. As usual, CHANGELOG collisions but importantly, restoring a lost CHANGELOG entry 51fa4e4#r50540088 .

However, GitHub did not let me commit and warned me that this would have gone to "develop". It is possible that it got confused because the original branch is VOD555/mdanalysis:develop. It gave me the option to rename and I thought it would create a PR onto the PR but it seems it created a new branch VOD555/mdanalysis:merge-rdf-results-develop and substituted it for the original branch of this PR. @VOD555 , I am sorry if this messed up your development environment. I think the way forward for you is to fetch this branch and continue working on it. Apparently, GitHub does not like using the protected branch name, even on forks.

IAlibay · 2021-05-08T08:55:55Z

However, GitHub did not let me commit and warned me that this would have gone to "develop".

Yeah I think that's just picking up the word develop from the incoming branch. I think you can just dismiss the warning (I remember having to do this a few times during GSoC application periods).

IAlibay

Just the one comment to maybe fix, otherwise lgtm

IAlibay · 2021-05-08T09:02:26Z

package/MDAnalysis/analysis/rdf.py

        # Maybe exclude same molecule distances
        if self._exclusion_block is not None:
-            idxA, idxB = pairs[:, 0]//self._exclusion_block[0], pairs[:, 1]//self._exclusion_block[1]
+            idxA = pairs[:, 0]//self._exclusion_block[0],


is the , necessary here?

I think it's actually incorrect because now idxA is a tuple. If this still passes tests then I'd like to know why.

orbeckst

The comma that @IAlibay found looks problematic. My other comments are minor.

orbeckst · 2021-05-08T12:23:20Z

package/MDAnalysis/analysis/rdf.py

+       .. deprecated:: 2.0.0
+           This attribute will be removed in 3.0.0.
+           Use :attr:`results.bins` instead.
+


@PicoCentauri , if you want to raise or find an issue regarding consistent formatting of analysis classes (which will presumably also aid mdacli in documentation extraction) then please do so and start adding TODOs...

orbeckst · 2021-05-08T12:27:49Z

package/MDAnalysis/analysis/rdf.py

        # Maybe exclude same molecule distances
        if self._exclusion_block is not None:
-            idxA, idxB = pairs[:, 0]//self._exclusion_block[0], pairs[:, 1]//self._exclusion_block[1]
+            idxA = pairs[:, 0]//self._exclusion_block[0],


I think it's actually incorrect because now idxA is a tuple. If this still passes tests then I'd like to know why.

package/MDAnalysis/analysis/rdf.py

testsuite/MDAnalysisTests/analysis/test_rdf.py

testsuite/MDAnalysisTests/analysis/test_rdf_s.py

…sis into merge-rdf-results-develop

tylerjereddy · 2021-05-09T03:20:07Z

package/MDAnalysis/analysis/rdf.py

-        vol = np.power(self.edges[1:], 3) - np.power(self.edges[:-1], 3)
-        vol *= 4/3.0 * np.pi
+        vols = np.power(self.results.edges, 3)
+        vol = 4/3 * np.pi * (vols[1:] - vols[:-1])


np.diff(vols) is also an option

…-rdf-results-develop

using self.results instead of self

f3facae

update tests

13d93ca

VOD555 added 5 commits May 6, 2021 23:06

add tests for DeprecationWarning

fa9161e

add test for cdf deprecationwarning

5078014

update docs

1512c05

fix pep8 errors

1b9dbfe

update cdf test

697b36f

PicoCentauri requested changes May 7, 2021

View reviewed changes

IAlibay requested changes May 7, 2021

View reviewed changes

orbeckst assigned IAlibay and PicoCentauri May 7, 2021

VOD555 added 4 commits May 7, 2021 12:15

Merge branch 'develop' of github.com:MDAnalysis/mdanalysis into develop

5af6366

use results.count and results.edges

8f9359b

update docs with new attributes

2c5092e

Merge branch 'develop' of github.com:MDAnalysis/mdanalysis into develop

63bb0ec

PicoCentauri approved these changes May 7, 2021

View reviewed changes

VOD555 added 2 commits May 7, 2021 15:54

squash code for warning tests

8a5a7d4

Merge branch 'develop' of github.com:MDAnalysis/mdanalysis into develop

53badfc

PicoCentauri approved these changes May 7, 2021

View reviewed changes

orbeckst added API Component-Analysis labels May 7, 2021

IAlibay requested changes May 8, 2021

View reviewed changes

VOD555 added 2 commits May 7, 2021 17:08

add version info for new attributes

80b87c4

update CHANGELOG

36de73a

orbeckst referenced this pull request May 8, 2021

Switch DistanceMatrix to Results class (#3288)

51fa4e4

* Fixes #3290 * Switch DistanceMatrix to Results class * add tests for deprecations * Updated CHANGELOG

Merge branch 'develop' into merge-rdf-results-develop

4063444

orbeckst mentioned this pull request May 8, 2021

Move results attributes of helix_analysis to Results #3279

Merged

4 tasks

merge develop

81e4a14

IAlibay approved these changes May 8, 2021

View reviewed changes

orbeckst added 2 commits May 8, 2021 05:18

Merge branch 'develop' into merge-rdf-results-develop

6c8941b

removed duplicate entry from CHANGELOG

931b520

orbeckst requested changes May 8, 2021

View reviewed changes

VOD555 added 3 commits May 8, 2021 07:41

modify indentation

ac760ce

remove import assert_equal

713a1ce

Merge branch 'merge-rdf-results-develop' of github.com:VOD555/mdanaly…

fe56fc9

…sis into merge-rdf-results-develop

orbeckst approved these changes May 8, 2021

View reviewed changes

tylerjereddy reviewed May 9, 2021

View reviewed changes

VOD555 and others added 4 commits May 8, 2021 23:41

use np.diff to calculate shell volume

022728e

Merge branch 'develop' of github.com:MDAnalysis/mdanalysis into merge…

51d2989

…-rdf-results-develop

Merge branch 'develop' into merge-rdf-results-develop

90715fb

Merge branch 'develop' into merge-rdf-results-develop

c814829

IAlibay approved these changes May 11, 2021

View reviewed changes

IAlibay merged commit 8618b7d into MDAnalysis:develop May 11, 2021

IAlibay added the enhancement label Sep 25, 2023

	Attributes
	----------
	results.times : numpy.ndarray
	simulation times used in analysis
	results.eigenvalues : numpy.ndarray
	calculated eigenvalues
	results.eigenvectors : numpy.ndarray
	calculated eigenvectors

		# Results stored in self.cdf
		# self.cdf is a list of cdf between pairs of AtomGroups in ags

Conversation

VOD555 commented May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

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

Uh oh!

Comment last updated at 2021-05-11 12:19:07 UTC

Uh oh!

codecov bot commented May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

PicoCentauri 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!

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!

IAlibay 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!

Uh oh!

PicoCentauri 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!

PicoCentauri left a comment

Choose a reason for hiding this comment

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

orbeckst commented May 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IAlibay commented May 8, 2021

Uh oh!

IAlibay left a comment

VOD555 commented May 7, 2021 •

edited

Loading

pep8speaks commented May 7, 2021 •

edited

Loading

codecov bot commented May 7, 2021 •

edited

Loading

orbeckst commented May 8, 2021 •

edited

Loading