Add simple atomic distance analysis (#3654) by xhgchen · Pull Request #4105 · MDAnalysis/mdanalysis

xhgchen · 2023-03-30T00:04:33Z

Resolves add simple atomic distance analysis to analysis.distances #3654
Add class AtomicDistances to MDAnalysis.analysis.distances to calculate the distances
|ag1[i] - ag2[i]| for all i from 0 to n_atoms - 1 for each frame over the trajectory
Allow periodic boundary conditions to be considered or ignored in class AtomicDistances by setting kwarg pbc to True or False
Add unit tests for class AtomicDistances to test_distances.py

The class itself is inspired by this comment in #3310 mentioned in #3654. How would I go about crediting them if this PR gets approved? I changed the list to a NumPy array to improve performance and added the pbc kwarg, as outlined above.

PR Checklist

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

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

github-actions · 2023-03-30T00:07:23Z

Linter Bot Results:

Hi @xhgchen! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code.

Code Location	Outcome
main package	⚠️ Possible failure
testsuite	⚠️ Possible failure

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/4669873354/jobs/8268901006

Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

codecov · 2023-03-30T00:21:28Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (210e05c) 93.59% compared to head (0cdabcf) 93.59%.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #4105   +/-   ##
========================================
  Coverage    93.59%   93.59%           
========================================
  Files          192      193    +1     
  Lines        25134    25156   +22     
  Branches      4056     4058    +2     
========================================
+ Hits         23523    23546   +23     
  Misses        1092     1092           
+ Partials       519      518    -1

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/atomicdistances.py	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

richardjgowers

this is a good start!

richardjgowers · 2023-03-30T08:40:17Z

package/MDAnalysis/analysis/distances.py

+        conditions (PBCs). The unitcell dimensions of the system must be
+        orthogonal or triclinic for the calculations with PBCs to work.


We don't need to mention what the unitcell must be here as we've previously enforced this

richardjgowers · 2023-03-30T08:41:06Z

package/MDAnalysis/analysis/distances.py


 import warnings
 import logging
+from .base import AnalysisBase


It might be cleaner to create a new file for this analysis class. It feels a little weird to import base into distances. Once you get circular imports things get confusing fast.

richardjgowers · 2023-03-30T08:43:48Z

package/MDAnalysis/analysis/distances.py

+        self.results = np.zeros((self.n_frames, self._ag1.atoms.n_atoms))
+
+    def _single_frame(self):
+        if (self._pbc):


a fun hack is to set a variable (box) to either .dimensions or None depending on self._pbc, then the call to calc_bonds can remain the same, and it's clearer what's happening (imho)

e.g.

box = self._ag1.dimensions if self._pbc else None self.results = calc_bonds(self._ag1.positions, self._ag2.positions, box=box)

richardjgowers · 2023-03-30T08:44:34Z

package/MDAnalysis/analysis/distances.py

+    n_frames : int
+        Number of frames included in the analysis.
+    n_atoms : int
+        Number of atoms in each atom group.


I don't see where these variables are set?

Thanks for all the feedback! I learned a lot working through it, and I have pushed some more commits.

I added those variables because other classes seemed to have them (in the docstring) and I used those as reference.

results refers to self.results in def _prepare(self) and def _single_frame(self)
n_frames is used to make the NumPy array in def _prepare(self)
n_atoms is used to check that ag1 and ag2 have the same number of atoms and in def _prepare(self) to make the NumPy array

Would it be better to remove some/all of them (from the docstring)?

richardjgowers · 2023-03-30T08:45:52Z

testsuite/MDAnalysisTests/analysis/test_distances.py

+    def ad_u():
+        return MDAnalysis.Universe(GRO, XTC)


can we use a mda.Universe.empty() here to avoid having to read files for a test?

For its positions, you could either use a seeded random number, or just something like np.arange(natoms * 3).reshape(natoms, -1) to generate arbitrary coordinates

So in order to do this, I had to change the dist() tests to use calc_bonds() instead because the Universe does not have resids

richardjgowers · 2023-03-30T08:46:55Z

testsuite/MDAnalysisTests/analysis/test_distances.py

+
+    @staticmethod
+    @pytest.fixture()
+    def expected_scipy(ad_ag1, ad_ag2):


I'm not sure we need a check against scipy, the check against dist is fine

richardjgowers · 2023-03-30T08:47:57Z

testsuite/MDAnalysisTests/analysis/test_distances.py

+            # non-pbc x, y, z distances
+            dist = np.abs(ad_ag1.positions - ad_ag2.positions)
+
+            # box size (lx, ly, lz)
+            box = ad_ag1.dimensions[:3]
+
+            # apply minimum image convention i.e. take box - dist
+            # when dist > box / 2
+            dist = np.where(dist > box / 2, box - dist, dist)
+
+            # desired dist = sqrt((x2 - x1)^2 + (y2 - y1)^2 + (z2 - z1)^2)
+            expected[i] = np.sqrt(np.square(dist).sum(axis=1))


again just use dist here. we're not testing that our distance calculations work here (we do that elsewhere). We're testing that this class is correctly applying those functions.

xhgchen · 2023-04-01T00:53:01Z

Now that all the checks have passed, @richardjgowers could you take a look at the new files when you get the chance? I would appreciate input on the file name as well. I was torn between atomic_distance_analysis.py and atomicdistances.py but I went with the latter because the first option seemed like a mouthful.

xhgchen · 2023-04-04T05:12:29Z

Just updating the docs so they work properly :)

This is the first time I have ever written docs, but it was fun.

Edit: The 2 failing checks both seem to be from an issue uploading to Codecov. I would expect them to work with a re-run. Here is a direct link to the preview docs page for the new module: https://mdanalysis--4105.org.readthedocs.build/en/4105/documentation_pages/analysis/atomicdistances.html

Edit 2: Very minor wording fix. Hopefully the checks will run correctly this time.

* Resolves MDAnalysis#3654 * Add class `AtomicDistances` to `MDAnalysis.analysis.distances` to calculate the distances `|ag1[i] - ag2[i]|` for all `i` from `0` to `n_atoms - 1` for each frame over the trajectory * Allow periodic boundary conditions to be considered or ignored in class `AtomicDistances` by setting kwarg `pbc` to `True` or `False` * Add unit tests for class `AtomicDistances` to `test_distances.py`

* Add `atomicdistances.py` for class `AtomicDistances` * Add `test_atomicdistances.py` to test class `AtomicDistances` * Modify docs for clarity * Use `box` variable to handle PBCs in `AtomicDistances` for clarity * Remove file imports for tests and replace with mda.Universe.empty(), use calc_bonds() instead of dist() for corresponding tests b/c no resid * Remove unnecessary distance calculations in tests (SciPy, positions)

* Restore `distances.py` and `test_distances.py` to their original state

* Modify docs so they can build successfully * Change formatting and wording to look pleasant and consistent

* Add "the distances" before the math expression in first paragraph

xhgchen · 2023-04-11T16:43:07Z

Since it has been a while since this was last reviewed, I was wondering when it might be looked at again? I realize that everyone is likely busy reviewing GSoC applications in addition to their other commitments and I am not in a hurry; it would just help me follow up more promptly when the time comes.

Also, the last push was just to resolve conflicts.

github-actions bot added the Component-Analysis label Mar 30, 2023

xhgchen force-pushed the simple-atomic-distance-analysis branch 2 times, most recently from 4d87b70 to e575a99 Compare March 30, 2023 00:28

richardjgowers requested changes Mar 30, 2023

View reviewed changes

richardjgowers self-assigned this Mar 30, 2023

xhgchen force-pushed the simple-atomic-distance-analysis branch from e575a99 to c3298f3 Compare March 31, 2023 23:49

xhgchen requested a review from richardjgowers April 1, 2023 00:53

xhgchen added 10 commits April 11, 2023 10:35

Fix linting issues in test_distances.py

f24ece9

Revert changes in old distance files

377d6f0

* Restore `distances.py` and `test_distances.py` to their original state

Add simple usage example in docs

a39603e

Update CHANGELOG

edc42d3

Modify usage example to include pbc=False

a26261d

Fix atom number typos in doc example

1e19455

Make docs accessible through TOC tree

09988dc

* Modify docs so they can build successfully * Change formatting and wording to look pleasant and consistent

Minor wording fix in docs

0cdabcf

* Add "the distances" before the math expression in first paragraph

xhgchen force-pushed the simple-atomic-distance-analysis branch from 4d68b34 to 0cdabcf Compare April 11, 2023 16:35

Merge branch 'develop' into simple-atomic-distance-analysis

ee6411e

richardjgowers approved these changes May 26, 2023

View reviewed changes

richardjgowers merged commit 99db4e4 into MDAnalysis:develop May 26, 2023

IAlibay added the new-feature label Sep 21, 2023

		conditions (PBCs). The unitcell dimensions of the system must be
		orthogonal or triclinic for the calculations with PBCs to work.

Conversation

xhgchen commented Mar 30, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

github-actions bot commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Linter Bot Results:

Uh oh!

codecov bot commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

richardjgowers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xhgchen Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xhgchen commented Apr 1, 2023

Uh oh!

xhgchen commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xhgchen commented Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xhgchen commented Mar 30, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Mar 30, 2023 •

edited

Loading

codecov bot commented Mar 30, 2023 •

edited

Loading

xhgchen Mar 31, 2023 •

edited

Loading

xhgchen commented Apr 4, 2023 •

edited

Loading

xhgchen commented Apr 11, 2023 •

edited

Loading