Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates for OpenMM 7.6 #50

Merged
merged 3 commits into from
Sep 3, 2021

Conversation

dwhswenson
Copy link
Member

Most significantly, this fixes a test error on the URL that @sroet suggested I add a test for (thanks @sroet!) by removing the simtk from the URL. I also went ahead and made it so we preferred new 7.6-style imports everywhere else.

@codecov
Copy link

codecov bot commented Sep 2, 2021

Codecov Report

Merging #50 (70bdc3c) into main (5581252) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #50   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           29        29           
  Lines         1202      1204    +2     
=========================================
+ Hits          1202      1204    +2     
Impacted Files Coverage Δ
paths_cli/wizard/openmm.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5581252...70bdc3c. Read the comment docs.

@dwhswenson
Copy link
Member Author

This is ready for review and comment. I will leave it open for at least 24 hours, merging no earlier than Fri 03 Sep 01:00 GMT (Thu 02 Sep 21:00 my local).

Copy link
Member

@sroet sroet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple left over old code and missing reasons for skips. I am not too happy about the code duplication on the test files (around the imports), but this is easier to understand than indirect imports, so no favor one way or another (As long as it exists in just 2 files)

paths_cli/tests/wizard/conftest.py Show resolved Hide resolved
paths_cli/tests/wizard/conftest.py Outdated Show resolved Hide resolved
paths_cli/tests/wizard/test_openmm.py Show resolved Hide resolved
paths_cli/tests/wizard/test_openmm.py Outdated Show resolved Hide resolved
@sroet
Copy link
Member

sroet commented Sep 2, 2021

Most significantly, this fixes a test error on the URL that @sroet suggested I add a test for (thanks @sroet!)

Glad this test already paid off 😄

@dwhswenson
Copy link
Member Author

dwhswenson commented Sep 2, 2021

Made changes, though not quite the ones you suggested. I intentionally left the commented importorskip to make it very easy to switch back to that code when we drop support for OpenMM < 7.6. I've added a comment to make that clear. Also followed your suggestion to add a message to the skip.

Copy link
Member

@sroet sroet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dwhswenson dwhswenson merged commit 8e71c33 into openpathsampling:main Sep 3, 2021
@dwhswenson dwhswenson deleted the fix-openmm-simtk-url branch September 3, 2021 03:21
@dwhswenson dwhswenson mentioned this pull request Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants