Standardize atomicdistances to use results#5347
Standardize atomicdistances to use results#5347charity-g wants to merge 4 commits intoMDAnalysis:developfrom
Conversation
BradyAJohnston
left a comment
There was a problem hiding this comment.
Some suggestions that should fix the issues with running tests.
The black formatter is also failing. You can run it like this:
uvx black~=24.0 package| # 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 |
There was a problem hiding this comment.
This is what's causing your tests to fail - this mdanalysis.package.MDAnalysis only exists on local file system. You should always just import directly from MDAnalysis - which will become available if you us pip install -e . or uv pip install -e ..
|
|
||
| """ | ||
|
|
||
| from mdanalysis.package.MDAnalysis.analysis.results import ( |
There was a problem hiding this comment.
same issue with imports as in other comment.
| Changes | ||
| * The msd.py inside analysis is changed, and ProgressBar is implemented inside | ||
| _conclude_simple and _conclude_fft functions instead of tqdm (Issue #5144, PR #5153) | ||
| * `MDAnalysis.analysis.atomicdistances.AtomicDistances` results are now consistent with expected`analysis` documentation data type = Results (Issue #4819) |
There was a problem hiding this comment.
needs a space between "expected analysis"
There was a problem hiding this comment.
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.
Break lines so that it's not longer than 79 characters.
There was a problem hiding this comment.
Add a line "NOTE: This fix is backwards-incompatible." to your CHANGELOG note.
|
@charity-g based on discussion in #4822 (comment) we will continue with this PR. Have a look at @BradyAJohnston 's comments and feel free to take it out of draft mode. |
orbeckst
left a comment
There was a problem hiding this comment.
The first thing is to make the code (and tests) run, following Brady's comments. Then please address my inline comments. Focus this PR just on the API change. CHANGELOG and docs must be very clear.
| Changes | ||
| * The msd.py inside analysis is changed, and ProgressBar is implemented inside | ||
| _conclude_simple and _conclude_fft functions instead of tqdm (Issue #5144, PR #5153) | ||
| * `MDAnalysis.analysis.atomicdistances.AtomicDistances` results are now consistent with expected`analysis` documentation data type = Results (Issue #4819) |
There was a problem hiding this comment.
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)
| Number of atoms in each atom group. | ||
|
|
||
|
|
||
| .. versionadded:: 2.5.0 |
There was a problem hiding this comment.
do not remove any of the versionchanged/versionadded; just add the new one below
There was a problem hiding this comment.
and do not remove empty lines – they are necessary for the proper formatting
|
|
||
|
|
||
| .. versionadded:: 2.5.0 | ||
| .. versionchanged:: 2.11.0 |
There was a problem hiding this comment.
Add a comment that describes the change, such as
| .. versionchanged:: 2.11.0 | |
| .. versionchanged:: 2.11.0 | |
| Distance data are now made available in :attr:`results.distances` instead | |
| of :attr:`results` and :attr:`results` is now a | |
| :class:`~MDAnalysis.analysis.results.Results` instance; this fixes an API issue | |
| (see `Issue #4819`_) in a *backwards-incompatible* manner. | |
| .. _Issue #4819`: https://github.com/MDAnalysis/mdanalysis/issues/4819 | |
You can just accept my suggested changes.
| def _get_aggregator(self): | ||
| return ResultsGroup(lookup={"distances": ResultsGroup.ndarray_vstack}) No newline at end of file |
There was a problem hiding this comment.
Remove and make this PR just about the API change.
Add parallelization later.
| Changes | ||
| * The msd.py inside analysis is changed, and ProgressBar is implemented inside | ||
| _conclude_simple and _conclude_fft functions instead of tqdm (Issue #5144, PR #5153) | ||
| * `MDAnalysis.analysis.atomicdistances.AtomicDistances` results are now consistent with expected`analysis` documentation data type = Results (Issue #4819) |
There was a problem hiding this comment.
Break lines so that it's not longer than 79 characters.
| Changes | ||
| * The msd.py inside analysis is changed, and ProgressBar is implemented inside | ||
| _conclude_simple and _conclude_fft functions instead of tqdm (Issue #5144, PR #5153) | ||
| * `MDAnalysis.analysis.atomicdistances.AtomicDistances` results are now consistent with expected`analysis` documentation data type = Results (Issue #4819) |
There was a problem hiding this comment.
Add a line "NOTE: This fix is backwards-incompatible." to your CHANGELOG note.
Fixes #4819
Changes made in this Pull Request:
MDAnalysis.analysis.atomicdistances.AtomicDistancesresults are now consistent with expectedanalysisdocumentation data type = Results (Issueanalysis.atomicdistances.AtomicDistancesdoes not use Results #4819)LLM / AI generated code disclosure
LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: no
PR Checklist
package/CHANGELOGfile updated?package/AUTHORS? (If it is not, add it!)Developers Certificate of Origin
I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.
📚 Documentation preview 📚: https://mdanalysis--5347.org.readthedocs.build/en/5347/