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

Enabling of parallelization of analysis.atomicdistances.AtomicDistances #4822

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
5 changes: 4 additions & 1 deletion package/CHANGELOG
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ The rules for this file:


-------------------------------------------------------------------------------
??/??/?? IAlibay, ChiahsinChu, RMeli
??/??/?? IAlibay, ChiahsinChu, RMeli, talagayev


* 2.9.0

Expand All @@ -23,6 +24,8 @@ Fixes
Enhancements
* Add check and warning for empty (all zero) coordinates in RDKit converter (PR #4824)
* Added `precision` for XYZWriter (Issue #4775, PR #4771)
* Enables parallelization of `analysis.atomicdistances.AtomicDistances`
(Issue #4819)

Changes

Expand Down
20 changes: 16 additions & 4 deletions package/MDAnalysis/analysis/atomicdistances.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@

import logging
from .base import AnalysisBase
from .results import Results

logger = logging.getLogger("MDAnalysis.analysis.atomicdistances")

Expand Down Expand Up @@ -145,6 +146,10 @@ class AtomicDistances(AnalysisBase):


.. versionadded:: 2.5.0

.. versionchanged:: 2.9.0
Copy link
Member

Choose a reason for hiding this comment

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

Update the docs above!!!!

Copy link
Member

Choose a reason for hiding this comment

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

... assuming a breaking change.

Enables parallelization through
the use of `self.results.distances`.
"""

def __init__(self, ag1, ag2, pbc=True, **kwargs):
Expand All @@ -163,14 +168,21 @@ def __init__(self, ag1, ag2, pbc=True, **kwargs):
self._ag1 = ag1
self._ag2 = ag2
self._pbc = pbc
self.results = Results()

def _prepare(self):
# initialize NumPy array of frames x distances for results
self.results = np.zeros((self.n_frames, self._ag1.atoms.n_atoms))
self.results.distances = np.zeros((
self.n_frames, self._ag1.atoms.n_atoms
))

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)
self.results.distances[self._frame_index] = calc_bonds(
self._ag1.positions, self._ag2.positions, box
)

def _conclude(self):
# adjust self.results to self.results.distances
self.results = self.results.distances
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm just confused but it looks to me like the tests haven't actually changed yet (just formatting changes as far as I can tell?) and this just restored the original behavior of self.results storing a NumPy array again?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, first I modified it to have the output would be self.results.distances, but then I thought that for the convenience to not modify the docs, where it uses my_dists.results to my_dists.results.distances I could define self.results as self.results.distances to not have to change the docs and still be able to implement the parallelization of the class.

As for the pytest changes, first I had the self.results.distances output and thus the tests needed to have also the .distances. The Idea to have self.results came after creating the PR 🙈, but yes I will then remove the mention of the pytest modification, was quite late when I finished up the PR, so didn't think about that :)

Copy link
Member

Choose a reason for hiding this comment

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

That's a hack!

If this the way we want to go then you have to write more commentary to say what you're doing and why, with references to issue and a note that this needs to be changed for 3.0.

7 changes: 4 additions & 3 deletions testsuite/MDAnalysisTests/analysis/test_atomicdistances.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,9 @@ def test_ad_pairwise_dist(self, ad_ag1, ad_ag2,
expected_dist):
'''Ensure that pairwise distances between atoms are
correctly calculated without PBCs.'''
pairwise_no_pbc = (ad.AtomicDistances(ad_ag1, ad_ag2,
pbc=False).run())
pairwise_no_pbc = ad.AtomicDistances(
ad_ag1, ad_ag2, pbc=False
).run()
actual = pairwise_no_pbc.results

# compare with expected values from dist()
Expand All @@ -129,7 +130,7 @@ 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())
pairwise_pbc = ad.AtomicDistances(ad_ag1, ad_ag2).run()
actual = pairwise_pbc.results

# compare with expected values from dist()
Expand Down
Loading