Skip to content

Conversation

@IAlibay
Copy link
Member

@IAlibay IAlibay commented Oct 14, 2025

TODO:

  • Fix tests for limiting the compute platforms
  • Add news entry

Checklist

  • Added a news entry

Developers certificate of origin

@mikemhenry
Copy link
Contributor

I know we like to use .lower() to make things easier for users, but do we think that breaking roundtrip serialization is worth it? Looks like we need to update a fixture so that our tests work. While its nice for testing and can be done with a fixture, do we want to expose a get_test_settings() method that just runs a little like these tests or do you think it depends too much on what you are trying to do?

@IAlibay
Copy link
Member Author

IAlibay commented Oct 14, 2025

but do we think that breaking roundtrip serialization is worth it

I might be missing something, we're storing the string as-is, unless you mean something else?

@mikemhenry
Copy link
Contributor

I see what you are saying, we use .lower() on the check but don't enforce any regularization on serialization, so if a user sets the default platform to CuDA that is what will be written in the transformation + result json.

@IAlibay
Copy link
Member Author

IAlibay commented Oct 17, 2025

I see what you are saying, we use .lower() on the check but don't enforce any regularization on serialization, so if a user sets the default platform to CuDA that is what will be written in the transformation + result json.

That's the behaviour we've always had - if you want to change this, I would suggest we do this in a separate PR. I'm only scoping this as "changing the default from None to cuda".


@field_validator('compute_platform')
def supported_sampler(cls, v):
supported = ['cpu', 'opencl', 'cuda']
Copy link
Contributor

Choose a reason for hiding this comment

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

Just adding a link here so if we ever decide we need HIP support, I can find this easily

#951

Copy link
Contributor

@mikemhenry mikemhenry left a comment

Choose a reason for hiding this comment

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

LGTM! Really just wondering if it makes sense to set s.protocol_repeats = 1 in the testing fixtures instead of in most of the tests that use the fixture

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if any tests here actually need more than one repeat

@IAlibay
Copy link
Member Author

IAlibay commented Oct 17, 2025

Really just wondering if it makes sense to set s.protocol_repeats = 1 in the testing fixtures instead of in most of the tests that use the fixture

Thanks, I will manually check and update - apologies in advance, for my own sanity I might not merge your suggestions from the github interface (so I can keep track of what has and hasn't been updated).

@codecov
Copy link

codecov bot commented Oct 17, 2025

Codecov Report

❌ Patch coverage is 90.56604% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.42%. Comparing base (ec229cc) to head (063efe8).

Files with missing lines Patch % Lines
...s/protocols/openmm_rfe/test_hybrid_top_protocol.py 75.00% 14 Missing ⚠️
...ests/protocols/openmm_md/test_plain_md_protocol.py 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1576      +/-   ##
==========================================
- Coverage   95.45%   93.42%   -2.03%     
==========================================
  Files         174      174              
  Lines       14552    14534      -18     
==========================================
- Hits        13891    13579     -312     
- Misses        661      955     +294     
Flag Coverage Δ
fast-tests 93.42% <90.56%> (?)
slow-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mikemhenry
Copy link
Contributor

mikemhenry commented Oct 17, 2025

Really just wondering if it makes sense to set s.protocol_repeats = 1 in the testing fixtures instead of in most of the tests that use the fixture

Thanks, I will manually check and update - apologies in advance, for my own sanity I might not merge your suggestions from the github interface (so I can keep track of what has and hasn't been updated).

That is fine! I haven't been able to get the "add suggestion to batch" to work with the new PR UI anyway so it would have been a separate commit for each one. I mostly did it so I could keep track of how many there were set to one versus how many were set to the default.

@IAlibay
Copy link
Member Author

IAlibay commented Oct 17, 2025

Ok that's all of the protocol repeats dealt with @mikemhenry !

@mikemhenry
Copy link
Contributor

I'm happy with it!

@github-actions
Copy link

No API break detected ✅

@IAlibay
Copy link
Member Author

IAlibay commented Oct 20, 2025

Failure is unrelated to this PR - merging so we can fix other things elsewhere.

@IAlibay IAlibay merged commit 7f93af7 into main Oct 20, 2025
4 of 12 checks passed
@IAlibay IAlibay deleted the default-cuda branch October 20, 2025 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants