SepTop: Addressing issues raised in review #1532

hannahbaumann · 2025-09-24T11:47:55Z

This PR addresses the following issues:

Checklist

Added a news entry

Developers certificate of origin

I certify that this contribution is covered by the MIT License here and the Developer Certificate of Origin at https://developercertificate.org/.

…is False

IAlibay · 2025-09-24T12:48:47Z

openfe/protocols/openmm_septop/base.py

            # 6. Get integrator
+            # Virtual sites sanity check - ensure we restart velocities when
+            # there are virtual sites in the system
+            has_virtual_sites = False


You'll want to do this early rather than late - in the setup unit, ideally.

Changed this!

codecov · 2025-09-25T09:17:35Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.19%. Comparing base (9987706) to head (54ee90f).
⚠️ Report is 117 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1532      +/-   ##
==========================================
- Coverage   95.23%   93.19%   -2.05%     
==========================================
  Files         172      172              
  Lines       14454    14469      +15     
==========================================
- Hits        13765    13484     -281     
- Misses        689      985     +296

Flag	Coverage Δ
fast-tests	`93.19% <100.00%> (?)`
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.

openfe/protocols/openmm_septop/base.py

openfe/protocols/openmm_septop/equil_septop_method.py

openfe/protocols/openmm_septop/equil_septop_settings.py

openfe/tests/protocols/openmm_septop/test_septop_protocol.py

IAlibay · 2025-09-25T09:35:12Z

openfe/protocols/openmm_septop/equil_septop_settings.py

    """


+class ComplexEquilOutputSettings(SepTopEquilOutputSettings):


Given the default is encouraged to be all for both solvent and complex, why not just force all for both? That will make it less complicated overall.

Otherwise, I would try to align the names - one is called SepTopEquilOutputSetitngs and the other is ComplexEquilOutputSettings, that's somewhat confusing naming wise.

Changed this!

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

openfe/protocols/openmm_septop/equil_septop_method.py

IAlibay

just a typo

…repeats

…nergy/openfe into septop_address_reviews

github-actions · 2025-09-25T13:34:05Z

No API break detected ✅

IAlibay

lgtm thanks!

IAlibay · 2025-09-29T09:37:30Z

openfe/protocols/openmm_septop/equil_septop_settings.py

        return v


 class SepTopEquilOutputSettings(MDOutputSettings):


Thinking about this more - I really like this solution, i.e. each Protocol subclasses their own "equilibration settings". Mainly because they can all quack the same way, but to users these will look like unique settings objects.

SepTop docs improvements lambda schedule and restraint selection

e493d51

This was linked to issues Sep 24, 2025

[SepTop & ABFEs] Update docs to better reflect the restraint selection scheme #1513

Closed

[SepTop] lambda schedule docs are somewhat unclear re: solvent transfer #1514

Closed

Remove set_openmm_threads_1 logic from test_septop_slow

e003449

hannahbaumann linked an issue Sep 24, 2025 that may be closed by this pull request

Remove set_openmm_threads_1 logic from test_septop_slow #1517

Closed

change femto acknowledgements

74e549f

hannahbaumann linked an issue Sep 24, 2025 that may be closed by this pull request

[SepTop] change femto acknowledgements #1521

Closed

update method docstring limitations

0ed6e4a

hannahbaumann linked an issue Sep 24, 2025 that may be closed by this pull request

[SepTop] update method docstring limitations #1524

Closed

Make sure we fail when we have virtual sites and reassign_velocities …

3d57ca7

…is False

hannahbaumann linked an issue Sep 24, 2025 that may be closed by this pull request

[SepTop] Make sure we fail when we have virtual sites and reassign_velocities is False #1519

Closed

clean up usage of openmm imports

40a4206

hannahbaumann linked an issue Sep 24, 2025 that may be closed by this pull request

[SepTop] clean up usage of openmm imports #1528

Closed

IAlibay reviewed Sep 24, 2025

View reviewed changes

hannahbaumann added 2 commits September 24, 2025 15:15

Move virtual site check to setup unit

2176df6

Move virtual site check to base class

7d7caac

hannahbaumann changed the title ~~SepTop: Addressing issues raised in review~~ [ WIP ] SepTop: Addressing issues raised in review Sep 24, 2025

hannahbaumann assigned IAlibay Sep 24, 2025

clean up equil settings output indices so that it enforces all

a2aa81e

hannahbaumann linked an issue Sep 25, 2025 that may be closed by this pull request

[SepTop & ABFEs] clean up equil settings output indices so that it enforces 'all' #1525

Closed

generate smc_off_B on the fly

49f2b35

hannahbaumann linked an issue Sep 25, 2025 that may be closed by this pull request

[SepTop] generate smc_off_B on the fly #1531

Closed

Merge branch 'main' into septop_address_reviews

b3f6d60

IAlibay requested changes Sep 25, 2025

View reviewed changes

hannahbaumann and others added 4 commits September 25, 2025 11:59

Update enforcement of all output indices

bda66b4

Update openfe/protocols/openmm_septop/base.py

d1f5dbe

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

Update openfe/protocols/openmm_septop/base.py

5a5cf62

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

Update openfe/protocols/openmm_septop/equil_septop_method.py

ef605e6

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

IAlibay reviewed Sep 25, 2025

View reviewed changes

openfe/protocols/openmm_septop/equil_septop_method.py Outdated Show resolved Hide resolved

IAlibay reviewed Sep 25, 2025

View reviewed changes

IAlibay and others added 3 commits September 25, 2025 13:04

Update openfe/protocols/openmm_septop/equil_septop_method.py

9946763

Simulation overview unclear on if the same restraint is used between …

95f8a1b

…repeats

Merge branch 'septop_address_reviews' of https://github.com/OpenFreeE…

f8c0233

…nergy/openfe into septop_address_reviews

hannahbaumann linked an issue Sep 25, 2025 that may be closed by this pull request

[SepTop docs] Simulation overview unclear on if the same restraint is used between repeats #1515

Closed

hannahbaumann added 2 commits September 25, 2025 15:25

Update doc string

c76c281

Add test for output indices validation

54ee90f

hannahbaumann changed the title ~~[ WIP ] SepTop: Addressing issues raised in review~~ SepTop: Addressing issues raised in review Sep 25, 2025

IAlibay approved these changes Sep 29, 2025

View reviewed changes

IAlibay merged commit ac06d1c into main Sep 29, 2025
13 checks passed

IAlibay deleted the septop_address_reviews branch September 29, 2025 09:39

		"""


		class ComplexEquilOutputSettings(SepTopEquilOutputSettings):

SepTop: Addressing issues raised in review #1532

SepTop: Addressing issues raised in review #1532

Uh oh!

Conversation

hannahbaumann commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Developers certificate of origin

Uh oh!

IAlibay Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

hannahbaumann Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IAlibay Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

hannahbaumann Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 25, 2025

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

IAlibay Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hannahbaumann commented Sep 24, 2025 •

edited

Loading

codecov bot commented Sep 25, 2025 •

edited

Loading