Standardize atomicdistances to use results#5347
Standardize atomicdistances to use results#5347charity-g wants to merge 11 commits intoMDAnalysis:developfrom
Conversation
BradyAJohnston
left a comment
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.
|
@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
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.
| 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
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
…github.com/charity-g/mdanalysis into standardize-atomicdistances-to-use-results
| * Fixes the verbose=False in EinsteinMSD, and only shows progress bar when | ||
| verbose=True (Issue #5144, PR #5153) | ||
| * Fix incorrect assignment of topology_format to format (and vice versa) | ||
| * Fix incorrect assignment of topology_format to format (and vexice versa) |
There was a problem hiding this comment.
Not sure why this is changed.
| import numpy as np | ||
|
|
||
| from MDAnalysis.lib.distances import calc_bonds | ||
| from MDAnalysis.analysis.results import ( |
There was a problem hiding this comment.
Could probably remove the comma and parenthesis to truncate into one line
| 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( |
There was a problem hiding this comment.
https://github.com/MDAnalysis/mdanalysis/actions/runs/23987727902/job/69962349164?pr=5347#step:8:518
Tests show that an all zeros array is passed back in both tests, suggesting the actual numbers/results are not saved properly into the Results object. This is the most important part of the PR(!) so please triple check.
6 participants
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/