-
Notifications
You must be signed in to change notification settings - Fork 747
Enabling of parallelization of analysis.atomicdistances.AtomicDistances
#4822
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 9 commits
c79b7f0
1bd64bc
916a973
cbb3e66
16a0605
0b58a73
9836575
5d6ed62
fd8163a
2aad77d
102ff95
7b61370
fa52881
e7da740
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 | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -114,6 +114,7 @@ | |||||||||||||
|
|
||||||||||||||
| import logging | ||||||||||||||
| from .base import AnalysisBase | ||||||||||||||
| from .results import Results | ||||||||||||||
|
|
||||||||||||||
| logger = logging.getLogger("MDAnalysis.analysis.atomicdistances") | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -145,6 +146,10 @@ class AtomicDistances(AnalysisBase): | |||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| .. versionadded:: 2.5.0 | ||||||||||||||
|
|
||||||||||||||
| .. versionchanged:: 2.9.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. Update the docs above!!!!
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. ... assuming a breaking change. |
||||||||||||||
| 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`. |
... assuming a breaking change.
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.
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?
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.
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 :)
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.
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.
Uh oh!
There was an error while loading. Please reload this page.