-
Notifications
You must be signed in to change notification settings - Fork 831
Standardize atomicdistances to use results #5347
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -74,10 +74,10 @@ | |||||||||||||||||||
| >>> ag2 = u.atoms[4000:4005] | ||||||||||||||||||||
|
|
||||||||||||||||||||
| We can run the calculations using any variable of choice such as | ||||||||||||||||||||
| ``my_dists`` and access our results using ``my_dists.results``: :: | ||||||||||||||||||||
| ``my_dists`` and access our results using ``my_dists.results.distances``: :: | ||||||||||||||||||||
|
|
||||||||||||||||||||
| >>> my_dists = ad.AtomicDistances(ag1, ag2).run() | ||||||||||||||||||||
| >>> my_dists.results | ||||||||||||||||||||
| >>> my_dists.results.distances | ||||||||||||||||||||
| array([[37.80813681, 33.2594864 , 34.93676414, 34.51183299, 34.96340209], | ||||||||||||||||||||
| [27.11746625, 31.19878079, 31.69439435, 32.63446126, 33.10451345], | ||||||||||||||||||||
| [23.27210749, 30.38714688, 32.48269361, 31.91444505, 31.84583838], | ||||||||||||||||||||
|
|
@@ -94,7 +94,7 @@ | |||||||||||||||||||
| in this case: :: | ||||||||||||||||||||
|
|
||||||||||||||||||||
| >>> my_dists_nopbc = ad.AtomicDistances(ag1, ag2, pbc=False).run() | ||||||||||||||||||||
| >>> my_dists_nopbc.results | ||||||||||||||||||||
| >>> my_dists_nopbc.results.distances | ||||||||||||||||||||
| array([[37.80813681, 33.2594864 , 34.93676414, 34.51183299, 34.96340209], | ||||||||||||||||||||
| [27.11746625, 31.19878079, 31.69439435, 32.63446126, 33.10451345], | ||||||||||||||||||||
| [23.27210749, 30.38714688, 32.482695 , 31.91444505, 31.84583838], | ||||||||||||||||||||
|
|
@@ -108,6 +108,10 @@ | |||||||||||||||||||
|
|
||||||||||||||||||||
| """ | ||||||||||||||||||||
|
|
||||||||||||||||||||
| from mdanalysis.package.MDAnalysis.analysis.results import ( | ||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same issue with imports as in other comment. |
||||||||||||||||||||
| Results, | ||||||||||||||||||||
| ResultsGroup, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| import numpy as np | ||||||||||||||||||||
|
|
||||||||||||||||||||
| from MDAnalysis.lib.distances import calc_bonds | ||||||||||||||||||||
|
|
@@ -134,17 +138,15 @@ class AtomicDistances(AnalysisBase): | |||||||||||||||||||
|
|
||||||||||||||||||||
| Attributes | ||||||||||||||||||||
| ---------- | ||||||||||||||||||||
| results : :class:`numpy.ndarray` | ||||||||||||||||||||
| results.distances : :class:`numpy.ndarray` | ||||||||||||||||||||
| The distances :math:`|ag1[i] - ag2[i]|` for all :math:`i` | ||||||||||||||||||||
| from :math:`0` to `n_atoms` :math:`- 1` for each frame over | ||||||||||||||||||||
| the trajectory. | ||||||||||||||||||||
| n_frames : int | ||||||||||||||||||||
| Number of frames included in the analysis. | ||||||||||||||||||||
| n_atoms : int | ||||||||||||||||||||
| Number of atoms in each atom group. | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| .. versionadded:: 2.5.0 | ||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do not remove any of the versionchanged/versionadded; just add the new one below
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and do not remove empty lines – they are necessary for the proper formatting |
||||||||||||||||||||
| .. versionchanged:: 2.11.0 | ||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a comment that describes the change, such as
Suggested change
You can just accept my suggested changes. |
||||||||||||||||||||
| """ | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def __init__(self, ag1, ag2, pbc=True, **kwargs): | ||||||||||||||||||||
|
|
@@ -167,11 +169,15 @@ def __init__(self, ag1, ag2, pbc=True, **kwargs): | |||||||||||||||||||
|
|
||||||||||||||||||||
| def _prepare(self): | ||||||||||||||||||||
| # initialize NumPy array of frames x distances for results | ||||||||||||||||||||
| self.results = np.zeros((self.n_frames, self._ag1.atoms.n_atoms)) | ||||||||||||||||||||
| distances = np.zeros((self.n_frames, self._ag1.atoms.n_atoms)) | ||||||||||||||||||||
| self.results = Results(distances=distances) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def _single_frame(self): | ||||||||||||||||||||
| # if PBCs considered, get box size | ||||||||||||||||||||
| box = self._ag1.dimensions if self._pbc else None | ||||||||||||||||||||
| self.results[self._frame_index] = calc_bonds( | ||||||||||||||||||||
| self._ag1.positions, self._ag2.positions, box | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def _get_aggregator(self): | ||||||||||||||||||||
| return ResultsGroup(lookup={"distances": ResultsGroup.ndarray_vstack}) | ||||||||||||||||||||
|
Comment on lines
+182
to
+183
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove and make this PR just about the API change. Add parallelization later. |
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |
| # MDAnalysis: A Toolkit for the Analysis of Molecular Dynamics Simulations. | ||
| # J. Comput. Chem. 32 (2011), 2319--2327, doi:10.1002/jcc.21787 | ||
| # | ||
| from mdanalysis.package.MDAnalysis.analysis.results import Results | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is what's causing your tests to fail - this |
||
| import pytest | ||
|
|
||
| import MDAnalysis as mda | ||
|
|
@@ -121,15 +122,19 @@ def test_ad_pairwise_dist(self, ad_ag1, ad_ag2, expected_dist): | |
| correctly calculated without PBCs.""" | ||
| pairwise_no_pbc = ad.AtomicDistances(ad_ag1, ad_ag2, pbc=False).run() | ||
| actual = pairwise_no_pbc.results | ||
|
|
||
| assert isinstance(actual, Results) | ||
|
|
||
| distances = actual.distances | ||
| # compare with expected values from dist() | ||
| assert_allclose(actual, expected_dist) | ||
| assert_allclose(distances, expected_dist) | ||
|
|
||
| def test_ad_pairwise_dist_pbc(self, ad_ag1, ad_ag2, expected_pbc_dist): | ||
| """Ensure that pairwise distances between atoms are | ||
| correctly calculated with PBCs.""" | ||
| pairwise_pbc = ad.AtomicDistances(ad_ag1, ad_ag2).run() | ||
| actual = pairwise_pbc.results | ||
| assert isinstance(actual, Results) | ||
|
|
||
| distances = actual.distances | ||
| # compare with expected values from dist() | ||
| assert_allclose(actual, expected_pbc_dist) | ||
| assert_allclose(distances, expected_pbc_dist) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs a space between "expected
analysis"There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this to the top of Fixes.
(We have to consider it a fix under Semantic Versioning, see #4819 (comment) , because if this is a "Change" then we need to wait until 3.0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Break lines so that it's not longer than 79 characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a line "NOTE: This fix is backwards-incompatible." to your CHANGELOG note.