Skip to content

Update for openfe v1.9 - #190

Merged
IAlibay merged 36 commits into
mainfrom
update-v1.9
Mar 25, 2026
Merged

Update for openfe v1.9#190
IAlibay merged 36 commits into
mainfrom
update-v1.9

Conversation

@IAlibay

@IAlibay IAlibay commented Mar 7, 2026

Copy link
Copy Markdown
Member

Fixes #169 #188 #184

@IAlibay

IAlibay commented Mar 7, 2026

Copy link
Copy Markdown
Member Author

pre-commit.ci autofix

@codecov

codecov Bot commented Mar 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.88390% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.55%. Comparing base (4353fcb) to head (2e00013).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...bus/tests/protocols/relative/test_protocol_slow.py 5.88% 16 Missing ⚠️
.../pontibus/protocols/relative/hybridtop_protocol.py 96.29% 1 Missing ⚠️
src/pontibus/protocols/relative/hybridtop_units.py 97.50% 1 Missing ⚠️
...bus/tests/protocols/solvation/test_tokenization.py 98.36% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #190      +/-   ##
==========================================
- Coverage   96.10%   93.55%   -2.56%     
==========================================
  Files          35       38       +3     
  Lines        3263     3195      -68     
==========================================
- Hits         3136     2989     -147     
- Misses        127      206      +79     

☔ View full report in Codecov by Harness.
📢 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.

@IAlibay

IAlibay commented Mar 8, 2026

Copy link
Copy Markdown
Member Author

pre-commit.ci autofix

@IAlibay

IAlibay commented Mar 8, 2026

Copy link
Copy Markdown
Member Author

pre-commit.ci autofix

@IAlibay

IAlibay commented Mar 8, 2026

Copy link
Copy Markdown
Member Author

The code has now all been updated and works for v1.9.
All that remains is docs, but that is blocked until #189 is complete.

@IAlibay
IAlibay marked this pull request as ready for review March 10, 2026 07:27
@j-wags j-wags self-assigned this Mar 17, 2026

@j-wags j-wags left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks great! Neither of my comments requests changes, just notes. This should be good to merge once #178 is merged in and tests pass!

Comment on lines +36 to 39
this solvent component. Default `False`
ion_concentration : openff.units.Quantity, optional
ionic concentration required, default 0.15 * unit.molar
ionic concentration required, default 0.0 * unit.molar
this must be supplied with units, e.g. "1.5 * unit.molar"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍 Good catch!

Comment thread src/pontibus/tests/protocols/solvation/test_results.py
IAlibay added 2 commits March 17, 2026 18:44
Added docstring to test classes explaining protocol results generation and simulation duration.
Added docstring to clarify protocol result generation and simulation expectations.
@IAlibay

IAlibay commented Mar 17, 2026

Copy link
Copy Markdown
Member Author

pre-commit.ci autofix

@j-wags j-wags left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah, my apologies - In my review and in our meeting earlier this week, I'd said:

This should be good to merge once #178 is merged in and tests pass!

I had misunderstood and thought that this PR couldn't run the slow/GPU tests on AWS without #178, but I now see that AWS tests haven't been run because they need to be manually dispatched (and since they're billed to OpenFE I'm not comfortable dispatching them myself). So I no longer think #178 is a blocker for this PR to be merged.

That said, it WOULD be good to run the AWS GPU and CPU tests on this branch but since I've already misspoken once on this topic I'm not going to call this a blocker. Sorry for the confusion, I'd be happy to discuss this synchronously in our meeting next week to ensure I'm not misunderstanding yet again.

@IAlibay

IAlibay commented Mar 19, 2026

Copy link
Copy Markdown
Member Author

I'm a little bit lost 😅 - can I suggest we don't merge this PR until we discuss it next week?

@j-wags

j-wags commented Mar 19, 2026

Copy link
Copy Markdown
Collaborator

Absolutely, I've added this to our meeting agenda. Apologies again for the confusion.

@@ -0,0 +1,68 @@
name: "CI - main branch of deps"
on:
pull_request:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@atravitz not sure if it was intention for this not to be triggered on PR. From discussions today, it sounds like that behaviour we wanted, so I added it. However I'm happy to remove it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Opened #192 so that we can decouple it from this PR and revisit it later.

@IAlibay

IAlibay commented Mar 24, 2026

Copy link
Copy Markdown
Member Author

GPU tests pass locally.

@IAlibay

IAlibay commented Mar 24, 2026

Copy link
Copy Markdown
Member Author

CPU vslow tests running here: https://github.com/OpenFreeEnergy/pontibus/actions/runs/23513343286
(they pass locally, but for some reason one of the tests had fewer waters.. so testing on aws CI to make sure it matches).

@IAlibay

IAlibay commented Mar 25, 2026

Copy link
Copy Markdown
Member Author

Took a while, but cpuvslow tests also pass on CI: https://github.com/OpenFreeEnergy/pontibus/actions/runs/23536584339/job/68513643017

@IAlibay

IAlibay commented Mar 25, 2026

Copy link
Copy Markdown
Member Author

Merging since we have now achieved all key objectives of this PR. Validation on test systems will come after.

@IAlibay
IAlibay merged commit b1877a0 into main Mar 25, 2026
11 of 13 checks passed
@IAlibay
IAlibay deleted the update-v1.9 branch March 25, 2026 15:38
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.

Update downstream ASFE Protocols for refactored AFE base units

3 participants