distopia 0.3.0 compatibility changes #4734

hmacdope · 2024-10-15T10:24:39Z

Test implementation of distopia-0.3.0 distances API.

Changes made in this Pull Request:

Adds support for distopia 0.3.0 API

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--4734.org.readthedocs.build/en/4734/

pep8speaks · 2024-10-15T10:24:47Z

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

In the file package/MDAnalysis/lib/_distopia.py:

Line 64:80: E501 line too long (82 > 79 characters)
Line 90:80: E501 line too long (86 > 79 characters)
Line 96:80: E501 line too long (103 > 79 characters)
Line 98:80: E501 line too long (83 > 79 characters)
Line 102:80: E501 line too long (103 > 79 characters)
Line 105:80: E501 line too long (83 > 79 characters)
Line 109:80: E501 line too long (107 > 79 characters)
Line 111:80: E501 line too long (87 > 79 characters)
Line 113:1: E302 expected 2 blank lines, found 1
Line 114:80: E501 line too long (124 > 79 characters)
Line 116:80: E501 line too long (95 > 79 characters)
Line 118:1: E302 expected 2 blank lines, found 1
Line 119:80: E501 line too long (124 > 79 characters)
Line 121:80: E501 line too long (95 > 79 characters)
Line 123:1: E302 expected 2 blank lines, found 1
Line 130:80: E501 line too long (82 > 79 characters)
Line 132:80: E501 line too long (82 > 79 characters)
Line 134:1: E302 expected 2 blank lines, found 1
Line 135:80: E501 line too long (82 > 79 characters)
Line 137:80: E501 line too long (82 > 79 characters)
Line 139:1: E302 expected 2 blank lines, found 1
Line 144:1: E302 expected 2 blank lines, found 1
Line 149:1: E302 expected 2 blank lines, found 1
Line 153:1: W391 blank line at end of file

In the file package/MDAnalysis/lib/distances.py:

Line 77:53: W291 trailing whitespace
Line 344:1: W293 blank line contains whitespace
Line 348:44: W291 trailing whitespace
Line 374:80: E501 line too long (80 > 79 characters)
Line 457:44: W291 trailing whitespace
Line 483:80: E501 line too long (80 > 79 characters)
Line 1677:44: W291 trailing whitespace
Line 1703:80: E501 line too long (80 > 79 characters)
Line 1800:44: W291 trailing whitespace
Line 1827:80: E501 line too long (80 > 79 characters)
Line 1939:44: W291 trailing whitespace
Line 1963:80: E501 line too long (80 > 79 characters)

In the file testsuite/MDAnalysisTests/lib/test_distances.py:

Line 1095:32: E222 multiple spaces after operator
Line 1099:5: E303 too many blank lines (2)
Line 1106:32: E222 multiple spaces after operator

Comment last updated at 2025-01-02 07:42:51 UTC

github-actions · 2024-10-15T10:26:53Z

Linter Bot Results:

Hi @hmacdope! 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	✅ Passed

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/11344140762/job/31548247192

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

codecov · 2024-10-15T10:35:26Z

Codecov Report

Attention: Patch coverage is 90.14085% with 7 lines in your changes missing coverage. Please review.

Project coverage is 93.62%. Comparing base (b8fe34b) to head (730cee9).

Files with missing lines	Patch %	Lines
package/MDAnalysis/lib/distances.py	88.57%	2 Missing and 2 partials ⚠️
package/MDAnalysis/lib/_distopia.py	91.66%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4734      +/-   ##
===========================================
- Coverage    93.66%   93.62%   -0.04%     
===========================================
  Files          177      189      +12     
  Lines        21787    22909    +1122     
  Branches      3067     3079      +12     
===========================================
+ Hits         20406    21448    +1042     
- Misses         929     1007      +78     
- Partials       452      454       +2

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

hmacdope · 2024-10-17T03:29:35Z

@orbeckst @richardjgowers this is ready for a first look over. The failing tests are for a 180 degree dihedral where numpy returns np.pi and distopia returns -np.pi (equivalent in polar coordinates). Are we ok to change the test to account for this? There are also options for changing in distopia but at the cost of a lot of performance improvement.

orbeckst · 2024-10-17T05:06:02Z

I'd be ok with changing tests but adding a note to docs.

package/MDAnalysis/lib/_distopia.py

orbeckst

I am not sure if the tests are actually run — if we can confirm that they run then I have no blockers.

ALso updated CHANGELOG, please.

package/MDAnalysis/lib/distances.py

package/MDAnalysis/lib/_distopia.py

orbeckst · 2024-10-18T21:45:28Z

FYI, upstream distopia 0.3.0 release is currently breaking all tests, see #4739 .

package/MDAnalysis/lib/distances.py

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

…ompat

orbeckst

Awesome to also have support for distance_array/self_distance_array!

I have a few comments on the docs (see inline) and minor suggestion for the test.

My major issue at the moment is that codecov does not mark the distance_array functions as tested. Given that it sees the bonds and angles, I wonder if we are really testing distopia-accelerated distance_array. Can you please double check? Thanks!

package/MDAnalysis/lib/distances.py

package/CHANGELOG

orbeckst · 2024-10-23T16:43:02Z

testsuite/MDAnalysisTests/lib/test_distances.py

-            import MDAnalysis.lib._distopia
-        assert not MDAnalysis.lib._distopia.HAS_DISTOPIA
+    sys.modules.pop("MDAnalysis.lib._distopia", None)   
+    if HAS_DISTOPIA:


For the test I'd ignore HAS_DISTOPIA and not add the conditional. In principle we always want to test explicitly that the version check is working. By using the conditional you implicitly assume that HAS_DISTOPIA was set to False because of the version check.

orbeckst · 2024-10-23T16:56:37Z

testsuite/MDAnalysisTests/lib/test_distances.py

-        assert not MDAnalysis.lib._distopia.HAS_DISTOPIA
+    sys.modules.pop("MDAnalysis.lib._distopia", None)   
+    if HAS_DISTOPIA:
+        with patch('distopia.__version__', '0.1.0'):


This only works if we can actually import distopia so I now understand why you need the conditional.

I'd prefer the test to work regardless of distopia being present, which can be achieved with the previous approach of creating a mocked module.

background on patch()

(I had to read up on patch to understand how it does its magic so in case anyone else is interested, here's the background.)

The docs for patch() say

unittest.mock.patch(target, new=DEFAULT, spec=None, create=False, spec_set=None, autospec=None, new_callable=None, **kwargs)
...
target should be a string in the form 'package.module.ClassName'. The target is imported and the specified object replaced with the new object, so the target must be importable from the environment you are calling patch() from.

(my emphasis).

Just to confirm: patch needs an importable module if one relies on the default behavior:

In [2]: from unittest.mock import patch In [3]: import distopia --------------------------------------------------------------------------- ModuleNotFoundError Traceback (most recent call last) Cell In[3], line 1 ----> 1 import distopia ModuleNotFoundError: No module named 'distopia' In [4]: with patch('distopia.__version__', '0.1.0'): ...: import distopia ...: print(distopa.__version__) ...: --------------------------------------------------------------------------- ModuleNotFoundError Traceback (most recent call last) Cell In[4], line 1 ----> 1 with patch('distopia.__version__', '0.1.0'): 2 import distopia 3 print(distopa.__version__) ... ModuleNotFoundError: No module named 'distopia'

package/MDAnalysis/lib/_distopia.py

orbeckst · 2024-10-23T17:02:29Z

package/MDAnalysis/lib/_distopia.py

+) -> None:
+    distopia.calc_self_distance_array_no_box(coords, results=results)
+
+def calc_self_distance_array_ortho(


Not tested + docs of self_distance_array

package/MDAnalysis/lib/_distopia.py

package/MDAnalysis/lib/distances.py

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

hmacdope · 2024-10-24T03:53:06Z

testsuite/MDAnalysisTests/lib/test_distances.py

@@ -946,6 +939,19 @@ def test_bonds(self, box, backend, dtype, pos, request):
        assert_almost_equal(dists_pbc[3], 3.46410072, self.prec,
                            err_msg="PBC check #w with box")

+    @pytest.mark.parametrize("dtype", (np.float32, np.float64))
+    @pytest.mark.parametrize("backend", distopia_conditional_backend())
+    def test_results_inplace_all_backends(self,  backend, dtype,):


Problematic test here.

package/MDAnalysis/lib/distances.py

package/MDAnalysis/lib/_distopia.py

orbeckst · 2024-10-24T22:15:15Z

package/MDAnalysis/lib/distances.py

+        # distopia requires that all the input arrays are the same type,
+        # while MDAnalysis allows for mixed types, this should be changed
+        # pre 0.3.0 release see issue #3707 
+        distances = distances.astype(np.float32)


This may create a copy.

orbeckst

With the astype required for distopia you may be creating a copy so that the top-level in-place operation is not actually in-place anymore. ... I think.

orbeckst · 2024-10-24T22:15:23Z

package/MDAnalysis/lib/distances.py

+    if backend == 'distopia':
+        # mda expects the result to be in float64, so we need to convert it back
+        # to float64, change for 3.0, see #3707
+        distances = distances.astype(np.float64)


This may create a copy.

You may have to instead put the numbers back into the original array to make in-place work.

result[:] = distances ... return result

or something of that kind.

I used this strategy

orbeckst · 2024-11-21T00:58:05Z

@hmacdope did you look further into the copy vs in-place issue?

hmacdope · 2024-12-02T21:06:57Z

@orbeckst I'll try get back to this over holidays, sorry for delay.

orbeckst · 2024-12-10T21:48:21Z

Ok! I think getting this one working will be important for a MDA paper.

orbeckst · 2024-12-10T22:04:57Z

@hmacdope I resolved the merge conflicts but please double-check that I didn't miss anything.

…s into distopia_0.3.0_compat

…ompat

hmacdope · 2025-01-02T09:20:37Z

@orbeckst this is probably ready for another look.

hmacdope added 6 commits October 15, 2024 16:02

start of docs

2ee12e5

improve distopia stup

9bf0cf7

add backend conditionals

f8d1295

fix wrong backend

badc778

shims

e97317b

working except for dihedrals

e0ce653

hmacdope added 3 commits October 16, 2024 21:07

move to 0.3.0

63162df

bump ci

0ec2057

bump ci

2392101

hmacdope marked this pull request as ready for review October 17, 2024 03:26

hmacdope requested review from orbeckst and richardjgowers October 17, 2024 03:30

IAlibay reviewed Oct 17, 2024

View reviewed changes

package/MDAnalysis/lib/_distopia.py Show resolved Hide resolved

orbeckst requested changes Oct 17, 2024

View reviewed changes

package/MDAnalysis/lib/distances.py Outdated Show resolved Hide resolved

package/MDAnalysis/lib/_distopia.py Show resolved Hide resolved

orbeckst mentioned this pull request Oct 18, 2024

Pin to distopia < 0.3 ahead of v2.8.0 release #4739

Closed

orbeckst mentioned this pull request Oct 18, 2024

Deprecate encore #4737

Merged

5 tasks

IAlibay requested changes Oct 18, 2024

View reviewed changes

package/MDAnalysis/lib/distances.py Show resolved Hide resolved

orbeckst mentioned this pull request Oct 18, 2024

pin distopia<0.3.0 #4740

Merged

8 tasks

hmacdope and others added 6 commits October 21, 2024 11:11

Update package/MDAnalysis/lib/distances.py

2cb04d2

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

Merge remote-tracking branch 'upstream/develop' into distopia_0.3.0_c…

e4b8a10

…ompat

fix versioning

517c676

add version checking

36de301

remove erroneous triclinic note

b49b028

remove unnesecary import

d895d99

orbeckst requested changes Oct 23, 2024

View reviewed changes

hmacdope and others added 5 commits October 24, 2024 09:39

Apply suggestions from code review

10356f7

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

add more tests

3b0024a

improve docs

9c149ec

improve docs again

8cae6f8

hmmm

50efeb9

hmacdope commented Oct 24, 2024

View reviewed changes

package/MDAnalysis/lib/distances.py Show resolved Hide resolved

orbeckst reviewed Oct 24, 2024

View reviewed changes

package/MDAnalysis/lib/_distopia.py Outdated Show resolved Hide resolved

orbeckst reviewed Oct 24, 2024

View reviewed changes

orbeckst self-assigned this Nov 21, 2024

Merge branch 'develop' into distopia_0.3.0_compat

2c070b7

add type for calc_bond_distance_ortho()

d4b9e9d

orbeckst added this to the Release 3.0 milestone Dec 12, 2024

hmacdope added 9 commits December 31, 2024 15:29

just stuff answers back in if we used a buffer

2621552

Merge branch 'distopia_0.3.0_compat' of github.com:hmacdope/mdanalysi…

154d389

…s into distopia_0.3.0_compat

add cram for distance array also

c75dce2

fix changelogs

02a3d93

Merge remote-tracking branch 'upstream/develop' into distopia_0.3.0_c…

07f1438

…ompat

black

9101d81

imporve mock test

447beac

fix typo

0a60016

add debug printing

730cee9

hmacdope requested a review from orbeckst January 2, 2025 07:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

distopia 0.3.0 compatibility changes #4734

distopia 0.3.0 compatibility changes #4734

hmacdope commented Oct 15, 2024 •

edited

Loading

pep8speaks commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 15, 2024

codecov bot commented Oct 15, 2024 •

edited

Loading

hmacdope commented Oct 17, 2024

orbeckst commented Oct 17, 2024

orbeckst left a comment •

edited

Loading

orbeckst commented Oct 18, 2024

orbeckst left a comment

orbeckst Oct 23, 2024

orbeckst Oct 23, 2024

orbeckst Oct 23, 2024

hmacdope Oct 24, 2024

orbeckst Oct 24, 2024

orbeckst left a comment

orbeckst Oct 24, 2024

orbeckst Oct 24, 2024

hmacdope Jan 2, 2025

orbeckst commented Nov 21, 2024

hmacdope commented Dec 2, 2024

orbeckst commented Dec 10, 2024

orbeckst commented Dec 10, 2024

hmacdope commented Jan 2, 2025

distopia 0.3.0 compatibility changes #4734

Are you sure you want to change the base?

distopia 0.3.0 compatibility changes #4734

Conversation

hmacdope commented Oct 15, 2024 • edited Loading

PR Checklist

Developers certificate of origin

pep8speaks commented Oct 15, 2024 • edited Loading

Comment last updated at 2025-01-02 07:42:51 UTC

github-actions bot commented Oct 15, 2024

Linter Bot Results:

codecov bot commented Oct 15, 2024 • edited Loading

Codecov Report

hmacdope commented Oct 17, 2024

orbeckst commented Oct 17, 2024

orbeckst left a comment • edited Loading

Choose a reason for hiding this comment

orbeckst commented Oct 18, 2024

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

background on patch()

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

orbeckst commented Nov 21, 2024

hmacdope commented Dec 2, 2024

orbeckst commented Dec 10, 2024

orbeckst commented Dec 10, 2024

hmacdope commented Jan 2, 2025

hmacdope commented Oct 15, 2024 •

edited

Loading

pep8speaks commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading

orbeckst left a comment •

edited

Loading

background on `patch()`