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 87:80: E501 line too long (86 > 79 characters)
Line 93:80: E501 line too long (103 > 79 characters)
Line 95:80: E501 line too long (83 > 79 characters)
Line 99:80: E501 line too long (103 > 79 characters)
Line 102:80: E501 line too long (83 > 79 characters)
Line 106:80: E501 line too long (107 > 79 characters)
Line 108:80: E501 line too long (87 > 79 characters)
Line 110:1: E302 expected 2 blank lines, found 1
Line 111:80: E501 line too long (124 > 79 characters)
Line 113:80: E501 line too long (95 > 79 characters)
Line 115:1: E302 expected 2 blank lines, found 1
Line 116:80: E501 line too long (124 > 79 characters)
Line 118:80: E501 line too long (95 > 79 characters)
Line 120:1: E302 expected 2 blank lines, found 1
Line 127:80: E501 line too long (82 > 79 characters)
Line 129:80: E501 line too long (82 > 79 characters)
Line 131:1: E302 expected 2 blank lines, found 1
Line 132:80: E501 line too long (82 > 79 characters)
Line 134:80: E501 line too long (82 > 79 characters)
Line 136:1: E302 expected 2 blank lines, found 1
Line 141:1: E302 expected 2 blank lines, found 1
Line 146:1: E302 expected 2 blank lines, found 1
Line 150:1: W391 blank line at end of file

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

Line 77:53: W291 trailing whitespace
Line 315:1: W293 blank line contains whitespace
Line 319:44: W291 trailing whitespace
Line 339:80: E501 line too long (80 > 79 characters)
Line 417:44: W291 trailing whitespace
Line 437:80: E501 line too long (80 > 79 characters)
Line 1527:44: W291 trailing whitespace
Line 1553:80: E501 line too long (80 > 79 characters)
Line 1645:44: W291 trailing whitespace
Line 1666:80: E501 line too long (80 > 79 characters)
Line 1774:44: W291 trailing whitespace
Line 1794:80: E501 line too long (80 > 79 characters)

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

Line 441:1: E302 expected 2 blank lines, found 1
Line 723:5: E303 too many blank lines (2)
Line 766:5: E303 too many blank lines (2)
Line 790:5: E303 too many blank lines (2)
Line 800:5: E303 too many blank lines (2)
Line 854:1: E303 too many blank lines (4)
Line 857:54: W291 trailing whitespace
Line 953:80: E501 line too long (93 > 79 characters)
Line 1079:80: E501 line too long (105 > 79 characters)
Line 1210:44: W291 trailing whitespace

Comment last updated at 2024-10-24 03:42:25 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 25.00000% with 45 lines in your changes missing coverage. Please review.

Project coverage is 86.21%. Comparing base (101008b) to head (50efeb9).

Files with missing lines	Patch %	Lines
package/MDAnalysis/lib/distances.py	0.00%	14 Missing and 9 partials ⚠️
package/MDAnalysis/lib/_distopia.py	40.54%	22 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (101008b) and HEAD (50efeb9). Click for more details.

HEAD has 2 uploads less than BASE

Flag BASE (101008b) HEAD (50efeb9)

7 5

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4734      +/-   ##
===========================================
- Coverage    93.65%   86.21%   -7.44%     
===========================================
  Files          175      187      +12     
  Lines        21564    22676    +1112     
  Branches      3023     3030       +7     
===========================================
- Hits         20195    19550     -645     
- Misses         925     2690    +1765     
+ Partials       444      436       -8

☔ 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

hmacdope · 2024-10-23T12:40:22Z

@orbeckst @IAlibay feature freeze aside this is probably ready for re-review.

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.

hmacdope · 2024-10-24T03:55:57Z

package/MDAnalysis/lib/distances.py

@@ -303,6 +312,14 @@ def distance_array(reference: Union[npt.NDArray, 'AtomGroup'],
    distances = _check_result_array(result, (refnum, confnum))
    if len(distances) == 0:
        return distances
+


@orbeckst @richardjgowers we have an issue which is that the distopia backend doesn't seem to fill the result array on return. ie post call for the other backends, result == distances outside the function while for distopia its passed through. Not sure if I am doing something simple wrong.

Offending example in a test on this PR.

@pytest.mark.parametrize("dtype", (np.float32, np.float64)) @pytest.mark.parametrize("backend", distopia_conditional_backend()) def test_results_inplace_all_backends(self, backend, dtype,): N = 10 c0 = np.ones(3 * N, dtype=dtype).reshape(N, 3) * 2 c1 = np.ones(3 * N, dtype=dtype).reshape(N, 3) * 3 result = np.zeros(N, dtype=np.float64) distances.calc_bonds(c0, c1, result=result, backend=backend) expected = np.ones(N, dtype=dtype) * 3**(1/2) # test the result array is updated in place assert_almost_equal(result, expected, self.prec, err_msg="calc_bonds inplace failed")

We can get around this with a copy, but shouldn't be nessecary right? The distopia core API works inplace. Im fairly sure I am just getting some basic scoping wrong.

This test shows distopia works in place.

https://github.com/MDAnalysis/distopia/pull/179/files#diff-a0adc50f7cfca5f98b7c9829f3709cdfb652f5ed0325fffc37885bb1308b4f18R69

I'd look into any use of @check_coords as it creates copies by default.

(Maybe not relevant as you're concerned with results.)

I think it's actually the astype() — see comment review.

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

package/MDAnalysis/lib/_distopia.py

        HAS_DISTOPIA = False


-from .c_distances import (
-    calc_bond_distance_triclinic as _calc_bond_distance_triclinic_serial,
-)
 import numpy as np


 def calc_bond_distance_ortho(
    coords1, coords2: np.ndarray, box: np.ndarray, results: np.ndarray


Any good reason why coords1 does not have a type?

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.

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

hmacdope added 3 commits October 21, 2024 16:11

add explanatory notes

5733a74

change tests in line with discussion

36c010a

changelog

763b493

hmacdope requested review from orbeckst and IAlibay October 21, 2024 10:09

hmacdope changed the title ~~[WIP] distopia 0.3.0 compatibility changes~~ distopia 0.3.0 compatibility changes Oct 23, 2024

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

orbeckst reviewed Oct 24, 2024

View reviewed changes

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

hmacdope commented Oct 23, 2024

orbeckst left a comment

orbeckst Oct 23, 2024

orbeckst Oct 23, 2024

orbeckst Oct 23, 2024

hmacdope Oct 24, 2024

hmacdope Oct 24, 2024

hmacdope Oct 24, 2024

orbeckst Oct 24, 2024

orbeckst Oct 24, 2024

orbeckst Oct 24, 2024

orbeckst Oct 24, 2024

orbeckst Oct 24, 2024

orbeckst left a comment

orbeckst Oct 24, 2024

orbeckst Oct 24, 2024

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 2024-10-24 03:42:25 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

hmacdope commented Oct 23, 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

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

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

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()`