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.

Sign up for GitHub

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
Open

Enabling of parallelization of analysis.atomicdistances.AtomicDistances #4822

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c79b7f0
Update atomicdistances.py
talagayev Dec 5, 2024
1bd64bc
Update test_atomicdistances.py
talagayev Dec 5, 2024
916a973
Update atomicdistances.py
talagayev Dec 5, 2024
cbb3e66
Update atomicdistances.py
talagayev Dec 5, 2024
16a0605
Update test_atomicdistances.py
talagayev Dec 5, 2024
0b58a73
Update CHANGELOG
talagayev Dec 5, 2024
9836575
Update atomicdistances.py
talagayev Dec 5, 2024
5d6ed62
Update test_atomicdistances.py
talagayev Dec 5, 2024
fd8163a
Update test_atomicdistances.py
talagayev Dec 5, 2024
2aad77d
Update CHANGELOG
talagayev Dec 5, 2024
102ff95
Merge branch 'develop' into atomicdistances_restructure
talagayev Dec 10, 2024
7b61370
Update CHANGELOG
talagayev Dec 10, 2024
fa52881
Update CHANGELOG
talagayev Dec 12, 2024
e7da740
Update atomicdistances.py
talagayev Dec 12, 2024
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
5 changes: 4 additions & 1 deletion package/CHANGELOG
View file
Open in desktop
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)
* Incorporation of `Results` into `analysis.atomicdistances.AtomicDistances`
Copy link
Member

Choose a reason for hiding this comment

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

This is a Breaking Change as it breaks code. If we put it in 2.9.0 then it has to go under Fix, though.

For right now, please put under Fixes and state clearly how it breaks existing code.

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.

If this PR is done without breaking changes (ie keeping self.results as array at the end) then do not reference the issue (which says that we need to have self.results.distances)

(Issue #4819)

Changes

Expand Down
20 changes: 16 additions & 4 deletions package/MDAnalysis/analysis/atomicdistances.py
View file
Open in desktop
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.

Implementation of `Results` into the class
for application in parallelization.
Copy link
Member

Choose a reason for hiding this comment

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

Need to be clearer about what was done:

Suggested change
Implementation of `Results` into the class
for application in parallelization.
Changed storage of results from :attr:`results` to
:attr:`results.distances`. This fix **breaks old
code** that expects to find the data array in
:attr:`results`.

... assuming a breaking change.

"""

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
View file
Open in desktop
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