Accept AtomGroup alongwith Universe in the DistanceMatrix #3541

AnirG · 2022-02-19T22:16:56Z

Changes made in this Pull Request:

Accepts AtomGroup instead of Universe in the DistanceMatrix.
Modified tests to account for the modified input parameter.

PR Checklist

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

codecov · 2022-02-19T22:32:59Z

Codecov Report

Merging #3541 (b1dd5f4) into develop (8eda52f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #3541   +/-   ##
========================================
  Coverage    94.23%   94.23%           
========================================
  Files          190      190           
  Lines        24754    24758    +4     
  Branches      3329     3330    +1     
========================================
+ Hits         23326    23330    +4     
  Misses        1380     1380           
  Partials        48       48

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/diffusionmap.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 8eda52f...b1dd5f4. Read the comment docs.

richardjgowers · 2022-02-20T09:10:43Z

package/MDAnalysis/analysis/diffusionmap.py

@@ -245,12 +246,13 @@ class DistanceMatrix(AnalysisBase):
         :class:`MDAnalysis.analysis.base.Results` instance.

    """
-    def __init__(self, universe, select='all', metric=rmsd, cutoff=1E0-5,
+    def __init__(self, atomGroup, select='all', metric=rmsd, cutoff=1E0-5,


This is missing the point of the change. Currently a Universe and selection string are both passed to construct an AtomGroup. The desired change is for this selection to happen outside of the function, so these two arguments are replaced by just an AtomGroup. To not break things abruptly, this will therefore require for a while accepting either U+selection (and warning about the change) OR AG.

I see. I thought we'd have AG along with selection anyways should the need arise. I'll make the necessary changes to accept AG along with Universe and selection.

pep8speaks · 2022-02-20T12:53:51Z

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

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

Line 175:80: E501 line too long (90 > 79 characters)
Line 257:80: E501 line too long (82 > 79 characters)
Line 332:80: E501 line too long (88 > 79 characters)
Line 349:80: E501 line too long (85 > 79 characters)

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

Line 86:1: W293 blank line contains whitespace
Line 118:1: E302 expected 2 blank lines, found 1

Comment last updated at 2022-04-11 07:51:20 UTC

AnirG · 2022-02-22T13:07:21Z

@richardjgowers does it seem fine? please let me know if I should add any more tests or any modification is required.

tylerjereddy · 2022-02-26T01:01:59Z

testsuite/MDAnalysisTests/analysis/test_diffusionmap.py

+def test_distvalues_ag_universe(u, ag):
+    dist_universe = diffusionmap.DistanceMatrix(u, select='backbone').run()
+    dist_ag = diffusionmap.DistanceMatrix(ag).run()
+    assert_array_almost_equal(dist_universe.results.dist_matrix,


For new tests we may as well use assert_allclose if the matrices contain floats per https://numpy.org/doc/stable/reference/generated/numpy.testing.assert_array_almost_equal.html

sure, will update it.

Maybe we should consider replacing assert_array_almost_equal in other places as well as per the above link. I've updated it for only this test function.

At this moment in time our approach is to only change tests we touch and make sure new tests use assert_allclose. Making large blanket changes makes it very hard to review PRs and also understand why changes were made based on history. If we need to do a blanket change we will do this in a specific PR in the future.

tylerjereddy · 2022-02-26T01:04:18Z

testsuite/MDAnalysisTests/analysis/test_diffusionmap.py

@@ -93,7 +105,7 @@ def test_long_traj(u):


 def test_not_universe_error(u):
-    trj_only = u.trajectory
+    trj_only = u.universe.trajectory


can you briefly explain what this change is for? The u fixture returns a universe and now we need another .universe call?

yeah, since u can also be an AtomGroup. I should've also updated match as match='U is not a Universe or AtomGroup'. I will do that.

@tylerjereddy is correct here, u.trajectory == u.universe.trajectory, there's no need to make this change, either way you are testing what happens if you pass a trajectory through (neither a universe or an atomgroup)

package/MDAnalysis/analysis/diffusionmap.py

IAlibay

thanks @AnirG just a couple of small additional comments (on top of the doc changes Richard asked for).

IAlibay · 2022-02-27T17:13:40Z

testsuite/MDAnalysisTests/analysis/test_diffusionmap.py

@@ -93,7 +105,7 @@ def test_long_traj(u):


 def test_not_universe_error(u):
-    trj_only = u.trajectory
+    trj_only = u.universe.trajectory


@tylerjereddy is correct here, u.trajectory == u.universe.trajectory, there's no need to make this change, either way you are testing what happens if you pass a trajectory through (neither a universe or an atomgroup)

testsuite/MDAnalysisTests/analysis/test_diffusionmap.py

AnirG · 2022-03-02T09:48:21Z

Hi, I'm not sure as to why the one codecov test failed.

AnirG · 2022-03-03T13:17:57Z

@tylerjereddy Hi looks like even #3527 failed similar tests on macOS like I'm facing. How should that be taken care of?

IAlibay · 2022-03-03T13:19:18Z

This is related to #3550 and some further issues upstream - we are currently dealing with this, there is nothing to do about this in this specific PR.

AnirG · 2022-03-05T19:06:48Z

Hi @IAlibay! Can you please suggest any more changes? I'm looking forward to taking up more issues once I get done with this one, and possibly would love to hear some general practices I might've missed here.

AnirG · 2022-03-13T16:10:13Z

@IAlibay Hi! Do the final changes seem fine? Would make the necessary changes otherwise.

jbarnoud

I restarted the tests as they seem to have failed on an unrelated issue. Assuming the tests do pass now, I think all the comments have been addressed appropriately. Thank you.

A couple of things are missing:

you need to update the CHANGELOG
you need to add yourself to the AUTHORS file

We hadn't release the 2.1.0 version when you last modified your pull request, so you will need to merge the develop branch or rebase against it to edit the most up to date version of these files.

Also, could you introduce yourself on the mailing list? We are discussing changes that will require us to contact all the contributors so we try to make sure we know how to reach new contributors. Thank you. (If you are interested in the Google Summer of Code, please say so explicitly; many people are introducing themselves at the moment in that context and we need to know who's a candidate and who's not 😃)

AnirG · 2022-03-15T13:21:22Z

@jbarnoud sure, will do that.

jbarnoud

I missed some tiny details during my previous readthrough. Once they are fixed, it should be good to go, assuming the other reviewers are satisfied.

jbarnoud · 2022-03-16T07:53:46Z

package/MDAnalysis/analysis/diffusionmap.py

@@ -171,7 +172,7 @@ class DistanceMatrix(AnalysisBase):

    Parameters
    ----------
-    universe : `~MDAnalysis.core.universe.Universe`
+    universe : `~MDAnalysis.core.universe.Universe or ~MDAnalysis.core.groups.AtomGroup`


Suggested change

universe : `~MDAnalysis.core.universe.Universe or ~MDAnalysis.core.groups.AtomGroup`

universe : `~MDAnalysis.core.universe.Universe` or `~MDAnalysis.core.groups.AtomGroup`

jbarnoud · 2022-03-16T07:55:19Z

package/MDAnalysis/analysis/diffusionmap.py

@@ -328,12 +330,12 @@ def __init__(self, u, epsilon=1, **kwargs):
            Parameters to be passed for the initialization of a
            :class:`DistanceMatrix`.
        """
-        if isinstance(u, Universe):
+        if isinstance(u, AtomGroup) or isinstance(u, Universe):


The change does not appear in the docstring just above.

Apologies! I have updated the docstrings.

IAlibay

Couple of extra things, you'll need to also resolve conflicts (which I think are related to the recent 2.1.0 -> 2.2.0 change)

IAlibay · 2022-03-17T12:07:56Z

package/CHANGELOG

@@ -28,7 +28,7 @@ Deprecations

 03/06/22 IAlibay, melomcr, mdd31, ianmkenney, richardjgowers, hmacdope,
         orbeckst, scal444, p-j-smith, edisj, Atharva7K, ojeda-e, jbarnoud,
-         yangtcai
+         yangtcai, AnirG


Version 2.1.0 has now been released, you'll need to update your branch and add your changelog entry to 2.2.0 instead.

Sorry, didn't realize 2.1.0 was out already. I have revoked the changes and have updated on 2.2.0.

IAlibay · 2022-03-17T12:10:14Z

package/MDAnalysis/analysis/diffusionmap.py

@@ -152,6 +152,7 @@
 """
 import logging
 import warnings
+from MDAnalysis import AtomGroup


Imports are usually grouped as "python built-ins", "external libraries", "internal library", see the PEP8 guide for more details: https://peps.python.org/pep-0008/#source-file-encoding

Can you move this to after line 159 please?

IAlibay · 2022-03-21T21:38:53Z

package/MDAnalysis/analysis/diffusionmap.py

@@ -328,12 +332,12 @@ def __init__(self, u, epsilon=1, **kwargs):
            Parameters to be passed for the initialization of a
            :class:`DistanceMatrix`.
        """
-        if isinstance(u, Universe):
+        if isinstance(u, AtomGroup) or isinstance(u, Universe):


Quick question here - what happens if you pass an UpdatingAtomGroup, does the code still work?

yes, it works fine. PFA example code snippet I tried on.
example.zip

Could you add in a test that uses an UpdatingAtomGroup?

sure, will add a separate test for updating atomgroups.

Hi, I'm not very sure about what kind of test do we want here. Do I make a test out of the example.zip that I attached before and compare the DistMatrix constructed by updating atom group with just universe and selection string of the atom group?

@IAlibay I did the above for updating AG checks. Please let me know if it is fine.

IAlibay

Two documentation things and a test request.

package/CHANGELOG

package/MDAnalysis/analysis/diffusionmap.py

IAlibay · 2022-03-23T14:43:57Z

package/MDAnalysis/analysis/diffusionmap.py

@@ -328,12 +332,12 @@ def __init__(self, u, epsilon=1, **kwargs):
            Parameters to be passed for the initialization of a
            :class:`DistanceMatrix`.
        """
-        if isinstance(u, Universe):
+        if isinstance(u, AtomGroup) or isinstance(u, Universe):


Could you add in a test that uses an UpdatingAtomGroup?

IAlibay

Couple of small extra things. Not sure why CI isn't triggering properly at the moment.

package/MDAnalysis/analysis/diffusionmap.py

IAlibay · 2022-03-24T10:27:51Z

package/MDAnalysis/analysis/diffusionmap.py

        Constructs an anisotropic diffusion kernel and performs eigenvalue
        decomposition on it.
    transform(n_eigenvectors, time)
        Perform an embedding of a frame into the eigenvectors representing
        the collective coordinates.
+
+    .. versionchanged: 2.2.0


See here: https://mdanalysis--3541.org.readthedocs.build/en/3541/documentation_pages/analysis/diffusionmap.html#MDAnalysis.analysis.diffusionmap.DiffusionMap.transform

You'll need two empty lines for this to work as intended.

Suggested change

.. versionchanged: 2.2.0

.. versionchanged: 2.2.0

package/MDAnalysis/analysis/diffusionmap.py

IAlibay · 2022-03-27T20:04:55Z

testsuite/MDAnalysisTests/analysis/test_diffusionmap.py

+
+def test_distvalues_updating_ag(u):
+    selection = 'resname ALA'
+    ag = u.select_atoms(selection, updating=True)


I'm not really understanding how this checks that an updating selection works, maybe I'm missing something obvious though.

Since universes are static, there shouldn't be fewer or more atoms that match "resname ALA" within the lifetime of a trajectory. Instead of you should aim to analyse something dynamic, like "ALA residues within a given distance of residue N".

yes my bad, I didn't give much attention to the atom selection. I was more concerned about if it worked when AG is an instance of UpdatingAtomGroup. I will update it such that selection is dynamic over all frames and the final selection is parsed over to DistanceMatrix. In my opinion, this test case is kinda redundant as we are checking if it's working for the final AG after all.

So the point of passing and UpdatingAtomGroup through and checking that everything still works as expected is because most Analysis methods in MDAnalysis use static defined numpy arrays. So if you end up with more or fewer entries than you'd expect, things can go wrong. Having a test that we can check works as expected is very important.

@IAlibay I have a doubt here. Say, you make a selection as selection = "around 5 resname ALA". For frame 1, let's assume this selection has 10 atoms, and for frame 50, it has 20 atoms. Do you expect RMSD(frame 1, frame 50) and RMSD(frame 50, frame 1) in the DistanceMatrix to be different since the selection is being updated dynamically due to the UpdatingAtomGroup? If this is what was implied here https://github.com/MDAnalysis/mdanalysis/pull/3541/files/2799ad9cf2b9f12bf673a217985d781535f802a9#r831565393 , I believe it doesn't work this way.
What can be done is package all these dynamic selections into one atomgroup and run our DistanceMatrix analysis on this AG (finally a static selection).

@IAlibay Is my understanding correct?

@AnirG this is essentially the crux of what I was leading to with my original question on this, I was trying to make you think about the cases where a user passed in an UpdatingAtomgroup expecting that the there would be more / fewer atoms depending on the frames - would things work as expected?

Because of the way metric works, there is an expectation that the i_ref and j_ref arrays are of equal lengths (see

mdanalysis/package/MDAnalysis/analysis/diffusionmap.py

Lines 185 to 191 in 009c467

metric : function, optional

Maps two numpy arrays to a float, is positive definite and

symmetric. The API for a metric requires that the arrays must have

equal length, and that the function should have weights as an

optional argument. Weights give each index value its own weight for

the metric calculation over the entire arrays. Default: metric is

set to rms.rmsd().

), so as you observed above, this is unlikely to work.

It's fine in this case for the solution to just be "UpdatingAtomGroups are not accepted". Creating a complex solution involving the eventual generation of a static selection is probably not worth it.

The next steps here are then - what code changes are necessary here to avoid users passing UpdatinAtomGroups?

As @lilyminium just reminded me,

mdanalysis/package/MDAnalysis/analysis/diffusionmap.py

Line 256 in e6f0fe1

self.atoms = universe.select_atoms(select)

essentially means that all UpdatingAtomGroups will be squashed to AtomGroups on class initilisation. Probably all you need here is to have a check on the input to see if it's an UpdatingAtomGroup and warn users that it is being converted to a static AtomGroup based on the current frame selection.

@IAlibay thanks for clarifying, I'll make the necessary changes.

… is parsed

IAlibay

Just the one last thing, otherwise everything looks good to me thanks!

IAlibay · 2022-04-08T16:31:18Z

package/MDAnalysis/analysis/diffusionmap.py

+
+        if isinstance(universe, UpdatingAtomGroup):
+            wmsg = ("U must be a static AtomGroup. Parsing an updating AtomGroup "
+                    "will result in a static AtomGroup with the first frame "


Please double check this, but I'm pretty sure it'll freeze it at the current frame, so if someone did:

u.trajectory[10] uag = u.select_atoms('resname HOH and within 4 resname LIG', updating=True) dmatrix = DistanceMatrix(ag)

Then you're doing uag.select_atoms('all') on the current frame, rather than the first frame.

Suggested change

"will result in a static AtomGroup with the first frame "

"will result in a static AtomGroup with the current frame "

yes. you're right. It's the current frame.

IAlibay · 2022-04-08T16:51:58Z

Please do fix merge conflicts too.

IAlibay · 2022-04-08T17:14:43Z

@jbarnoud if you can also have a re-review that'd be great

IAlibay

lgtm, thanks for all the work @AnirG.

Just waiting on @jbarnoud to re-review.

jbarnoud

Sorry, I'll ask for one last small test, and it is all good.

Users may provide both an AtomGroup and the select argument. From what I read, it should work well with your implementation, but I would like to see a test for it. That test does not have to be complicated, just to use both an AtomGroup and the select argument. I would write it as the same as test_distvalues_ag_universe but with

ag = u.select_atoms('protein'')
dist_ag = diffusionmap.DistanceMatrix(ag, select='backbone').run()

jbarnoud · 2022-04-11T07:52:57Z

Thank you for having taken care of the conflicts as well. I'll merge as soon as the CI is done and passing :)

AnirG · 2022-04-11T07:56:07Z

@IAlibay @jbarnoud thanks a lot for your patience and guidance :) felt quite satisfying starting out on this. Will hopefully solve more issues as I use the package more and get a better understanding over time.

jbarnoud

🎉

jbarnoud · 2022-04-11T08:59:15Z

Congratulations! Thank you for addressing the issue :D

github-actions bot added the Component-Analysis label Feb 19, 2022

AnirG changed the title ~~Issue 3486~~ Accept AtomGroup instead of Universe in the DistanceMatrix Feb 19, 2022

richardjgowers requested changes Feb 20, 2022

View reviewed changes

tylerjereddy reviewed Feb 26, 2022

View reviewed changes

richardjgowers requested changes Feb 27, 2022

View reviewed changes

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

IAlibay requested changes Feb 27, 2022

View reviewed changes

AnirG force-pushed the issue-3486 branch 2 times, most recently from 8b79261 to ab98e21 Compare March 1, 2022 12:54

AnirG changed the title ~~Accept AtomGroup instead of Universe in the DistanceMatrix~~ Accept AtomGroup alongwith Universe in the DistanceMatrix Mar 1, 2022

AnirG requested review from richardjgowers and IAlibay March 2, 2022 09:46

jbarnoud requested changes Mar 15, 2022

View reviewed changes

AnirG force-pushed the issue-3486 branch from 90d86ba to 9e6d304 Compare March 15, 2022 19:11

jbarnoud reviewed Mar 16, 2022

View reviewed changes

IAlibay requested changes Mar 17, 2022

View reviewed changes

IAlibay reviewed Mar 21, 2022

View reviewed changes

IAlibay requested changes Mar 23, 2022

View reviewed changes

richardjgowers approved these changes Mar 23, 2022

View reviewed changes

IAlibay requested changes Mar 24, 2022

View reviewed changes

AnirG added 2 commits March 24, 2022 21:30

Accept AtomGroup instead of Universe in the DistanceMatrix

d373177

Modified Tests

ae9e9df

AnirG force-pushed the issue-3486 branch from 7ea3c8c to 9251e50 Compare March 24, 2022 21:56

AnirG added 4 commits March 25, 2022 03:30

fixed merge issues with authors and changelog

af23f05

update docstring

86136ba

remove github id from previous release

09588f6

tests on an updating AG

2799ad9

IAlibay requested changes Mar 27, 2022

View reviewed changes

AnirG added 7 commits March 28, 2022 19:05

upstream changes merged

e6f0fe1

upstream changes merged

adda3f6

upstream changes merged

28cf0bd

revoked the redundant testcase and user warning if updating atomgroup…

f4a457c

… is parsed

merge

f1b63be

minor docstring edit

17415d5

warning test for updating atomgroup

f79e9c5

AnirG requested a review from IAlibay April 7, 2022 05:48

IAlibay requested changes Apr 8, 2022

View reviewed changes

AnirG added 2 commits April 9, 2022 00:36

rebase

2afccf7

modified warning message

765f942

IAlibay approved these changes Apr 8, 2022

View reviewed changes

rebase

f366946

jbarnoud requested changes Apr 11, 2022

View reviewed changes

AnirG added 2 commits April 11, 2022 13:19

Merge remote-tracking branch 'upstream/develop' into issue-3486

2670166

added test where ag and select string is parsed

b1dd5f4

jbarnoud approved these changes Apr 11, 2022

View reviewed changes

jbarnoud merged commit 0eb42ca into MDAnalysis:develop Apr 11, 2022

IAlibay added the enhancement label Sep 25, 2023

	universe : `~MDAnalysis.core.universe.Universe or ~MDAnalysis.core.groups.AtomGroup`
	universe : `~MDAnalysis.core.universe.Universe` or `~MDAnalysis.core.groups.AtomGroup`

	metric : function, optional
	Maps two numpy arrays to a float, is positive definite and
	symmetric. The API for a metric requires that the arrays must have
	equal length, and that the function should have weights as an
	optional argument. Weights give each index value its own weight for
	the metric calculation over the entire arrays. Default: metric is
	set to rms.rmsd().

	"will result in a static AtomGroup with the first frame "
	"will result in a static AtomGroup with the current frame "

Accept AtomGroup alongwith Universe in the DistanceMatrix #3541

Accept AtomGroup alongwith Universe in the DistanceMatrix #3541

Conversation

AnirG commented Feb 19, 2022 • edited Loading

PR Checklist

codecov bot commented Feb 19, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

AnirG Feb 20, 2022 • edited Loading

Choose a reason for hiding this comment

pep8speaks commented Feb 20, 2022 • edited Loading

Comment last updated at 2022-04-11 07:51:20 UTC

AnirG commented Feb 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnirG commented Mar 2, 2022

AnirG commented Mar 3, 2022

IAlibay commented Mar 3, 2022 • edited Loading

AnirG commented Mar 5, 2022

AnirG commented Mar 13, 2022

jbarnoud left a comment

Choose a reason for hiding this comment

AnirG commented Mar 15, 2022

jbarnoud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnirG Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnirG Mar 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnirG Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

IAlibay commented Apr 8, 2022

IAlibay commented Apr 8, 2022

IAlibay left a comment

Choose a reason for hiding this comment

jbarnoud left a comment

Choose a reason for hiding this comment

jbarnoud commented Apr 11, 2022

AnirG commented Apr 11, 2022

jbarnoud left a comment

Choose a reason for hiding this comment

jbarnoud commented Apr 11, 2022

AnirG commented Feb 19, 2022 •

edited

Loading

codecov bot commented Feb 19, 2022 •

edited

Loading

AnirG Feb 20, 2022 •

edited

Loading

pep8speaks commented Feb 20, 2022 •

edited

Loading

IAlibay commented Mar 3, 2022 •

edited

Loading

AnirG Mar 22, 2022 •

edited

Loading

AnirG Mar 28, 2022 •

edited

Loading

AnirG Apr 8, 2022 •

edited

Loading