Skip to content
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

Accept AtomGroup alongwith Universe in the DistanceMatrix #3541

Merged
merged 29 commits into from
Apr 11, 2022
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
d373177
Accept AtomGroup instead of Universe in the DistanceMatrix
AnirG Feb 19, 2022
ae9e9df
Modified Tests
AnirG Feb 19, 2022
64fe2fe
Added a test to assert same values of distmatrix
AnirG Feb 20, 2022
3cb2544
Accept both AG and Universe
AnirG Feb 20, 2022
747e486
Resolve PEP8
AnirG Feb 20, 2022
571cfa5
Final changes
AnirG Mar 1, 2022
477400f
updated AUTHOR and CHANGELOG
AnirG Mar 15, 2022
910f48a
corrected and updated docstrings
AnirG Mar 16, 2022
5bbeb81
modified and displaced import statement
AnirG Mar 21, 2022
04b0a55
updated changelog and added self to authors
AnirG Mar 21, 2022
4335817
updated changelog and authors
AnirG Mar 24, 2022
2a50647
updated docstrings
AnirG Mar 24, 2022
9251e50
fixed typo and edited docstrings
AnirG Mar 24, 2022
af23f05
fixed merge issues with authors and changelog
AnirG Mar 24, 2022
86136ba
update docstring
AnirG Mar 24, 2022
09588f6
remove github id from previous release
AnirG Mar 24, 2022
2799ad9
tests on an updating AG
AnirG Mar 26, 2022
e6f0fe1
upstream changes merged
AnirG Mar 28, 2022
adda3f6
upstream changes merged
AnirG Apr 4, 2022
28cf0bd
upstream changes merged
AnirG Apr 4, 2022
f4a457c
revoked the redundant testcase and user warning if updating atomgroup…
AnirG Apr 5, 2022
f1b63be
merge
AnirG Apr 5, 2022
17415d5
minor docstring edit
AnirG Apr 5, 2022
f79e9c5
warning test for updating atomgroup
AnirG Apr 5, 2022
2afccf7
rebase
AnirG Apr 8, 2022
765f942
modified warning message
AnirG Apr 8, 2022
f366946
rebase
AnirG Apr 9, 2022
2670166
Merge remote-tracking branch 'upstream/develop' into issue-3486
AnirG Apr 11, 2022
b1dd5f4
added test where ag and select string is parsed
AnirG Apr 11, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package/AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ Chronological list of authors
- Henok Ademtew
- Uma D Kadam
- Tamandeep Singh
- Anirvinya G

External code
-------------
Expand Down
5 changes: 4 additions & 1 deletion package/CHANGELOG
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ The rules for this file:
* release numbers follow "Semantic Versioning" http://semver.org

------------------------------------------------------------------------------
??/??/?? IAlibay, BFedder, inomag, Agorfa, aya9aladdin, shudipto-amin, cbouy, HenokB, umak1106, tamandeeps
??/??/?? IAlibay, BFedder, inomag, Agorfa, aya9aladdin, shudipto-amin, cbouy, HenokB,
umak1106, tamandeeps, AnirG

* 2.2.0

Expand All @@ -30,6 +31,8 @@ Fixes
* Updated the badge in README of MDAnalysisTest to point to Github CI Actions

Enhancements
* `DistanceMatrix` class in MDAnalysis.analysis.diffusionmap now also
accepts `AtomGroup` as a valid argument (Issue #3486, PR #3541)
* Improves the RDKitConverter's accuracy (PR #3044): AtomGroups containing
monatomic ion charges or edge cases with nitrogen, sulfur, phosphorus and
conjugated systems should now have correctly assigned bond orders and
Expand Down
27 changes: 20 additions & 7 deletions package/MDAnalysis/analysis/diffusionmap.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@
import numpy as np

from MDAnalysis.core.universe import Universe
from MDAnalysis.core.groups import AtomGroup, UpdatingAtomGroup
from .rms import rmsd
from .base import AnalysisBase

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

Parameters
----------
universe : `~MDAnalysis.core.universe.Universe`
universe : `~MDAnalysis.core.universe.Universe` or `~MDAnalysis.core.groups.AtomGroup`
The MD Trajectory for dimension reduction, remember that
computational cost of eigenvalue decomposition
scales at O(N^3) where N is the number of frames.
Expand Down Expand Up @@ -243,12 +244,20 @@ class DistanceMatrix(AnalysisBase):
.. versionchanged:: 2.0.0
:attr:`dist_matrix` is now stored in a
:class:`MDAnalysis.analysis.base.Results` instance.

.. versionchanged:: 2.2.0
:class:`DistanceMatrix` now also accepts `AtomGroup`.
"""
def __init__(self, universe, select='all', metric=rmsd, cutoff=1E0-5,
jbarnoud marked this conversation as resolved.
Show resolved Hide resolved
weights=None, **kwargs):
# remember that this must be called before referencing self.n_frames
super(DistanceMatrix, self).__init__(universe.trajectory, **kwargs)
super(DistanceMatrix, self).__init__(universe.universe.trajectory,
IAlibay marked this conversation as resolved.
Show resolved Hide resolved
**kwargs)

if isinstance(universe, UpdatingAtomGroup):
wmsg = ("U must be a static AtomGroup. Parsing an updating AtomGroup "
"will result in a static AtomGroup with the current frame "
"atom selection.")
warnings.warn(wmsg)

self.atoms = universe.select_atoms(select)
self._metric = metric
Expand Down Expand Up @@ -309,14 +318,18 @@ class DiffusionMap(object):
transform(n_eigenvectors, time)
Perform an embedding of a frame into the eigenvectors representing
the collective coordinates.


.. versionchanged:: 2.2.0
:class:`DiffusionMap` now also accepts `AtomGroup`.
"""

def __init__(self, u, epsilon=1, **kwargs):
"""
Parameters
-------------
u : MDAnalysis Universe or DistanceMatrix object
Can be a Universe, in which case one must supply kwargs for the
u : MDAnalysis Universe or AtomGroup or DistanceMatrix object.
Can be a Universe or AtomGroup, in which case one must supply kwargs for the
initialization of a DistanceMatrix. Otherwise, this can be a
DistanceMatrix already initialized. Either way, this will be made
into a diffusion kernel.
Expand All @@ -328,12 +341,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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change does not appear in the docstring just above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies! I have updated the docstrings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@AnirG AnirG Mar 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add in a test that uses an UpdatingAtomGroup?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, will add a separate test for updating atomgroups.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self._dist_matrix = DistanceMatrix(u, **kwargs)
elif isinstance(u, DistanceMatrix):
self._dist_matrix = u
else:
raise ValueError("U is not a Universe or DistanceMatrix and"
raise ValueError("U is not a Universe or AtomGroup or DistanceMatrix and"
" so the DiffusionMap has no data to work with.")
self._epsilon = epsilon

Expand Down
21 changes: 18 additions & 3 deletions testsuite/MDAnalysisTests/analysis/test_diffusionmap.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import numpy as np
import pytest
from MDAnalysisTests.datafiles import PDB, XTC
from numpy.testing import assert_array_almost_equal
from numpy.testing import assert_array_almost_equal, assert_allclose


@pytest.fixture(scope='module')
Expand Down Expand Up @@ -69,6 +69,14 @@ def test_dist_weights(u):
[.707, -.707, 0, 0]]), 2)


def test_distvalues_ag_universe(u):
dist_universe = diffusionmap.DistanceMatrix(u, select='backbone').run()
ag = u.select_atoms('backbone')
dist_ag = diffusionmap.DistanceMatrix(ag).run()
IAlibay marked this conversation as resolved.
Show resolved Hide resolved
assert_allclose(dist_universe.results.dist_matrix,
dist_ag.results.dist_matrix)


def test_different_steps(u):
dmap = diffusionmap.DiffusionMap(u, select='backbone')
dmap.run(step=3)
Expand All @@ -92,9 +100,16 @@ def test_long_traj(u):
dmap.run()


def test_not_universe_error(u):
def test_updating_atomgroup(u):
with pytest.warns(UserWarning, match='U must be a static AtomGroup'):
resid_select = 'around 5 resname ALA'
ag = u.select_atoms(resid_select, updating=True)
dmap = diffusionmap.DiffusionMap(ag)
dmap.run()

def test_not_universe_atomgroup_error(u):
trj_only = u.trajectory
with pytest.raises(ValueError, match='U is not a Universe'):
with pytest.raises(ValueError, match='U is not a Universe or AtomGroup'):
diffusionmap.DiffusionMap(trj_only)


Expand Down