distopia 0.4.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-06 18:00:18 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 97.05882% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.64%. Comparing base (975486e) to head (19f0efc).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
package/MDAnalysis/lib/_distopia.py	94.28%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4734      +/-   ##
===========================================
- Coverage    93.65%   93.64%   -0.02%     
===========================================
  Files          177      189      +12     
  Lines        21795    22916    +1121     
  Branches      3067     3079      +12     
===========================================
+ Hits         20413    21459    +1046     
- Misses         931     1006      +75     
  Partials       451      451

☔ 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 <[email protected]>

…ompat

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

@orbeckst this is probably ready for another look.

testsuite/MDAnalysisTests/lib/test_distances.py

orbeckst

Quick first look (sorry for brevity), see minor comments and below:

Could you please also make PEP8 happy?
Coverage?

testsuite/MDAnalysisTests/lib/test_distances.py

orbeckst · 2025-01-06T23:44:31Z

Something is not right with the macos-13 tests — they timed out at 55 mins and they are again taking much longer than all the other runners.

IAlibay · 2025-01-07T01:27:02Z

Something is not right with the macos-13 tests — they timed out at 55 mins and they are again taking much longer than all the other runners.

This is unrelated to this PR as far as I can tell (we probably should re-open #4781?). It's been a while but iirc the macos13 images had a sudden performance drop at some point in the last year-ish and it never was resolved. We could drop the tests to be "non optional deps only" that might help keep things a bit sane for now.

IAlibay

Couple things, apparent missing test coverage is the biggest thing, otherwise (assuming @orbeckst's comments are addressed), lgtm.

IAlibay · 2025-01-07T01:29:34Z

package/MDAnalysis/lib/_distopia.py

-    has_distopia_020 = all([hasattr(distopia, func) for func in needed_funcs])
-    if not has_distopia_020:
+    # check for compatibility: currently needs to be >=0.3.1,
+    # some versions of `distopia` don't have a version attribute


Are these versions known? I.e. if we know that this only affects a certain set of releases and they are all <0.3.1, then can we skip the warning altogther and just assume that this only happens in old versions we don't care about?

(intent: I'm trying to completly bypass the follow-up "there are no tests for line 50" with a blunt solution)

We can (now) just say that if we are not able to determine the version then it's too old.

package/MDAnalysis/lib/_distopia.py

package/MDAnalysis/lib/distances.py

orbeckst · 2025-02-04T16:52:07Z

@hmacdope any bandwidth to get this one done soon-ish? It would be really good to have it in to finish one of the performance track items.

IAlibay · 2025-02-04T16:57:45Z

@hmacdope any bandwidth to get this one done soon-ish? It would be really good to have it in to finish one of the performance track items.

If I can jump in here - it would be great to get this into a hypothetical v2.9.0 release of MDAnalysis (i.e. one that deals with #4899 and allows us to do py3.13).

hmacdope · 2025-02-04T20:42:37Z

@orbeckst @IAlibay yes can bump up the list, IIRC just need to add some coverage.

hmacdope · 2025-02-09T10:20:43Z

@orbeckst this is probably ready for another round of review. I cut a new release of distopia to change some of the function names, should read much nicer now.

orbeckst

Sorry, just quick: there are distopia-related tests that are failing. Can you please check and fix or if necessary skip tests as needed?

orbeckst

Please see small fixes in comments.

package/MDAnalysis/lib/_distopia.py

orbeckst · 2025-02-10T00:03:54Z

package/MDAnalysis/lib/_distopia.py

-    has_distopia_020 = all([hasattr(distopia, func) for func in needed_funcs])
-    if not has_distopia_020:
+    # check for compatibility: currently needs to be >=0.3.1,
+    # some versions of `distopia` don't have a version attribute


We can (now) just say that if we are not able to determine the version then it's too old.

orbeckst · 2025-02-10T00:05:33Z

package/MDAnalysis/lib/_distopia.py

+    try:
+        distopia_version = Version(distopia.__version__)
+    except AttributeError:
+        warnings.warn("distopia version cannot be determined, assuming 0.0.0")


Although we don't need to issue the warning, we need to be able to set the fake distopia version.

Is it worthwhile testing this code path with a proper mock? – @IAlibay ?

Test is simple enough through monkeypatch. Adding it would be great, but I'm not sure it needs to be a blocker for this PR. At the end of the day we'll pin the conda-forge recipe to distopia>=0.4 and probably should do the same for pypi (is there a pypi release of distopia?).

TLDR; I'd be ok to see this is as a follow-up PR, but I agree we probably should do it at some point.

Can we push to seperate issue? Will raise when this is closed.

(is there a pypi release of distopia?).

No there isn't, very hard to ship required libs.

package/MDAnalysis/lib/distances.py

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 Outdated 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 <[email protected]>

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 requested a review from orbeckst January 2, 2025 07:43

Merge branch 'develop' into distopia_0.3.0_compat

a865a46

orbeckst reviewed Jan 6, 2025

View reviewed changes

testsuite/MDAnalysisTests/lib/test_distances.py Show resolved Hide resolved

orbeckst requested changes Jan 6, 2025

View reviewed changes

testsuite/MDAnalysisTests/lib/test_distances.py Show resolved Hide resolved

testsuite/MDAnalysisTests/lib/test_distances.py Show resolved Hide resolved

IAlibay approved these changes Jan 7, 2025

View reviewed changes

Merge branch 'develop' into distopia_0.3.0_compat

10e0ec1

hmacdope added 11 commits February 8, 2025 12:46

add explanatory comment on use of serial backend comparison

195dabe

add testing

3f9c2d3

black

3a7833b

fix typo

a227494

fix test

69c6143

fix dih

4d2b54a

black

914f9d3

fix dihedrals

5711abd

fix reference values

8860d86

change function names for 0.4.0

b5b788e

black

26d2665

hmacdope requested a review from orbeckst February 9, 2025 10:19

orbeckst requested changes Feb 9, 2025

View reviewed changes

fix typo

ba049f9

hmacdope changed the title ~~distopia 0.3.0 compatibility changes~~ distopia 0.4.0 compatibility changes Feb 9, 2025

orbeckst requested changes Feb 10, 2025

View reviewed changes

fix wrong versions

19f0efc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

distopia 0.4.0 compatibility changes #4734

distopia 0.4.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 Jan 2, 2025

orbeckst left a comment

orbeckst commented Jan 6, 2025

IAlibay commented Jan 7, 2025

IAlibay left a comment

IAlibay Jan 7, 2025

orbeckst Feb 10, 2025

orbeckst commented Feb 4, 2025

IAlibay commented Feb 4, 2025

hmacdope commented Feb 4, 2025

hmacdope commented Feb 9, 2025

orbeckst left a comment

orbeckst left a comment

orbeckst Feb 10, 2025

orbeckst Feb 10, 2025

IAlibay Feb 10, 2025

hmacdope Feb 10, 2025

distopia 0.4.0 compatibility changes #4734

Are you sure you want to change the base?

distopia 0.4.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-06 18:00:18 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 Jan 2, 2025

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst commented Jan 6, 2025

IAlibay commented Jan 7, 2025

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay Jan 7, 2025

Choose a reason for hiding this comment

orbeckst Feb 10, 2025

Choose a reason for hiding this comment

orbeckst commented Feb 4, 2025

IAlibay commented Feb 4, 2025

hmacdope commented Feb 4, 2025

hmacdope commented Feb 9, 2025

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst Feb 10, 2025

Choose a reason for hiding this comment

orbeckst Feb 10, 2025

Choose a reason for hiding this comment

IAlibay Feb 10, 2025

Choose a reason for hiding this comment

hmacdope Feb 10, 2025

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