Enabling of parallelization of `analysis.atomicdistances.AtomicDistances` #4822

talagayev · 2024-12-05T00:59:22Z

Changes made in this Pull Request:

added self.results and self.results.distances in analysis.atomicdistances.AtomicDistances for the use of Results

PR Checklist

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

Developers certificate of origin

I certify that this contribution is covered by the LGPLv2.1+ license as defined in our LICENSE and adheres to the Developer Certificate of Origin.

📚 Documentation preview 📚: https://mdanalysis--4822.org.readthedocs.build/en/4822/

changed to be applicable with Results

adjusted pytestst

removed unnecessary results

pep8speaks · 2024-12-05T00:59:28Z

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-12-12 23:12:15 UTC

added documentation and adjusted output

removed the .distances

adjusted changelog

pep fixes

codecov · 2024-12-05T01:31:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.66%. Comparing base (a27591c) to head (e7da740).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4822      +/-   ##
===========================================
- Coverage    93.68%   93.66%   -0.03%     
===========================================
  Files          177      189      +12     
  Lines        21743    22813    +1070     
  Branches      3055     3055              
===========================================
+ Hits         20370    21367     +997     
- Misses         927     1000      +73     
  Partials       446      446

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

package/CHANGELOG

tylerjereddy · 2024-12-05T03:46:14Z

package/MDAnalysis/analysis/atomicdistances.py

+
+    def _conclude(self):
+        # adjust self.results to self.results.distances
+        self.results = self.results.distances


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?

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 :)

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.

orbeckst

I was confused because the PR supposedly fixes #4819 but it does not.

Instead it provides a workaround for the problem identified in PR #4808 that we cannot currently parallelize AtomicDistances.

I would be ok with this hack to make parallelization work for 2.9.0 and then make the switch (fix for #4819) before 3.0. I'd like this PR to be clearer about what it does.

I'd also like to hear some other opinions, e.g., @tylerjereddy @marinegor @IAlibay .

orbeckst · 2024-12-12T22:37:24Z

package/CHANGELOG

@@ -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`


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.

... 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)

orbeckst · 2024-12-12T22:39:24Z

package/MDAnalysis/analysis/atomicdistances.py

+        Implementation of `Results` into the class
+        for application in parallelization.


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.

orbeckst · 2024-12-12T22:40:06Z

package/MDAnalysis/analysis/atomicdistances.py

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


    .. versionadded:: 2.5.0
+
+    .. versionchanged:: 2.9.0


Update the docs above!!!!

... assuming a breaking change.

orbeckst · 2024-12-12T22:45:03Z

package/MDAnalysis/analysis/atomicdistances.py

+
+    def _conclude(self):
+        # adjust self.results to self.results.distances
+        self.results = self.results.distances


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.

talagayev · 2024-12-12T23:02:38Z

I was confused because the PR supposedly fixes #4819 but it does not.

Instead it provides a workaround for the problem identified in PR #4808 that we cannot currently parallelize AtomicDistances.

I would be ok with this hack to make parallelization work for 2.9.0 and then make the switch (fix for #4819) before 3.0. I'd like this PR to be clearer about what it does.

I'd also like to hear some other opinions, e.g., @tylerjereddy @marinegor @IAlibay .

Yes my bad, I thought that it would fix #4819 but I think I missunderstood it, since I thought having it work with parallelization would fix the Issue, since it would have something that the ResultsGroup could use for the parallelization and thus use Results 🙈

talagayev · 2024-12-12T23:09:02Z

I was confused because the PR supposedly fixes #4819 but it does not.
Instead it provides a workaround for the problem identified in PR #4808 that we cannot currently parallelize AtomicDistances.
I would be ok with this hack to make parallelization work for 2.9.0 and then make the switch (fix for #4819) before 3.0. I'd like this PR to be clearer about what it does.
I'd also like to hear some other opinions, e.g., @tylerjereddy @marinegor @IAlibay .

Yes my bad, I thought that it would fix #4819 but I think I missunderstood it, since I thought having it work with parallelization would fix the Issue, since it would have something that the ResultsGroup could use for the parallelization and thus use Results 🙈

I will then rename the PR more like enabling the parallelization of analysis.atomicdistances.AtomicDistances and also remove the mention of the fix, change the changelog to it and the docs and wait for further instructions how to procede

renamed change

changed docs

tylerjereddy · 2024-12-13T01:04:04Z

I'd also like to hear some other opinions, e.g., @tylerjereddy @marinegor @IAlibay .

Yes, this is why I went silent on my review activity--I couldn't decide if I was just too out of the loop on the parallel business or if this didn't do what the targeted ticket asked for. I couldn't work out how the changes addressed the original problem so figured I may have just not understood.

My original understanding was that there was supposed to be some kind of results-related container object that may contain "slots" for specific results, and it was being incorrectly used to store a specific result itself.

If I understand this correctly, is it really that much more work to do it the "right" way? Or just a compatibility concern?

orbeckst · 2024-12-13T07:32:06Z

It would be easy to do it right, just as you say — just not add the line that you commented on (self.results = self.results.distances). The problem is that the fix (for #4819) would break the previous behavior (so it's a "compatibility concern"). There were some concerns by @IAlibay #4819 (comment) to make the change. @talagayev is proposing a workaround that will allow (1) implementing the parallelization of the class (see discussion on #4808 why that would be good), (2) avoid breaking the user-facing code (and then properly fix in 3.0).

My opinion (as stated in #4819 (comment) ) is that I'd rather fix the API ("doing it right") and call the break a fix.

marinegor · 2024-12-14T21:27:26Z

I second Oliver on this -- I'd rather fix the non-compliant class and add the new functionality later, than add some temporary hacks.

If it's urgent to add the distances parallelization right now (which I doubt), it's always possible to add it as an mdakit which you'll deprecate when the upstream fixes the issue.

IAlibay · 2024-12-14T22:06:33Z

Just wanted to say that I see the ping here but I haven't read anything and won't until the latter half of next week - apologies it's a busy end of year period.

talagayev added 3 commits December 5, 2024 01:36

Update atomicdistances.py

c79b7f0

changed to be applicable with Results

Update test_atomicdistances.py

1bd64bc

adjusted pytestst

Update atomicdistances.py

916a973

removed unnecessary results

talagayev added 6 commits December 5, 2024 02:08

Update atomicdistances.py

cbb3e66

added documentation and adjusted output

Update test_atomicdistances.py

16a0605

removed the .distances

Update CHANGELOG

0b58a73

adjusted changelog

Update atomicdistances.py

9836575

pep fixes

Update test_atomicdistances.py

5d6ed62

pep fixes

Update test_atomicdistances.py

fd8163a

talagayev marked this pull request as ready for review December 5, 2024 01:50

tylerjereddy added the Component-Analysis label Dec 5, 2024

tylerjereddy reviewed Dec 5, 2024

View reviewed changes

Update CHANGELOG

2aad77d

github-actions bot removed the Component-Analysis label Dec 5, 2024

talagayev added 2 commits December 10, 2024 18:01

Merge branch 'develop' into atomicdistances_restructure

102ff95

Update CHANGELOG

7b61370

talagayev requested a review from tylerjereddy December 11, 2024 00:50

orbeckst requested changes Dec 12, 2024

View reviewed changes

orbeckst self-assigned this Dec 12, 2024

talagayev changed the title ~~Implementation of Results into analysis.atomicdistances.AtomicDistances~~ Enabling of parallelization of analysis.atomicdistances.AtomicDistances Dec 12, 2024

talagayev added 2 commits December 13, 2024 00:10

Update CHANGELOG

fa52881

renamed change

Update atomicdistances.py

e7da740

changed docs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabling of parallelization of `analysis.atomicdistances.AtomicDistances` #4822

Enabling of parallelization of `analysis.atomicdistances.AtomicDistances` #4822

talagayev commented Dec 5, 2024 •

edited

Loading

pep8speaks commented Dec 5, 2024 •

edited

Loading

codecov bot commented Dec 5, 2024 •

edited

Loading

tylerjereddy Dec 5, 2024

talagayev Dec 5, 2024

orbeckst Dec 12, 2024

orbeckst left a comment

orbeckst Dec 12, 2024

orbeckst Dec 12, 2024

orbeckst Dec 12, 2024

orbeckst Dec 12, 2024

orbeckst Dec 12, 2024

orbeckst Dec 12, 2024

talagayev commented Dec 12, 2024

talagayev commented Dec 12, 2024

tylerjereddy commented Dec 13, 2024

orbeckst commented Dec 13, 2024

marinegor commented Dec 14, 2024

IAlibay commented Dec 14, 2024

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

-        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`.

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


		.. versionadded:: 2.5.0

		.. versionchanged:: 2.9.0

Enabling of parallelization of analysis.atomicdistances.AtomicDistances #4822

Are you sure you want to change the base?

Enabling of parallelization of analysis.atomicdistances.AtomicDistances #4822

Conversation

talagayev commented Dec 5, 2024 • edited Loading

PR Checklist

Developers certificate of origin

pep8speaks commented Dec 5, 2024 • edited Loading

Comment last updated at 2024-12-12 23:12:15 UTC

codecov bot commented Dec 5, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talagayev commented Dec 12, 2024

talagayev commented Dec 12, 2024

tylerjereddy commented Dec 13, 2024

orbeckst commented Dec 13, 2024

marinegor commented Dec 14, 2024

IAlibay commented Dec 14, 2024

Enabling of parallelization of `analysis.atomicdistances.AtomicDistances` #4822

Enabling of parallelization of `analysis.atomicdistances.AtomicDistances` #4822

talagayev commented Dec 5, 2024 •

edited

Loading

pep8speaks commented Dec 5, 2024 •

edited

Loading

codecov bot commented Dec 5, 2024 •

edited

Loading