Protocols test migration & MD Protocol simulation tests by IAlibay · Pull Request #1294 · OpenFreeEnergy/openfe

IAlibay · 2025-05-23T15:07:33Z

Fixes #1298
Fixes #1295
Fixes #1299

It started as just the AHFE protocol, but I thought it'd be better to just move everything.

TODO:

MD Protocol tokenization tests.
MD Protocol slow tests.

Checklist

Added a news entry - internal API only, no news needed

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/.

codecov · 2025-05-23T16:53:04Z

Codecov Report

❌ Patch coverage is 53.60000% with 58 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.63%. Comparing base (8ade13d) to head (8963fd2).
⚠️ Report is 266 commits behind head on main.

Files with missing lines	Patch %	Lines
...fe/tests/protocols/openmm_md/test_plain_md_slow.py	16.17%	57 Missing ⚠️
openfe/tests/protocols/conftest.py	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1294      +/-   ##
==========================================
- Coverage   94.34%   92.63%   -1.71%     
==========================================
  Files         141      143       +2     
  Lines       10868    10957      +89     
==========================================
- Hits        10253    10150     -103     
- Misses        615      807     +192

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

hannahbaumann

Thanks @IAlibay , this looks good to me, maybe we can add a test for the number of frames once that mystery is solved... =)

IAlibay · 2025-05-27T14:07:26Z

Thanks @IAlibay , this looks good to me, maybe we can add a test for the number of frames once that mystery is solved... =)

Yup, I'm adding this in the follow up PR #1302 :)

atravitz

as far as file organization, I'd prefer to have the tests more closely match the code's file structure. so something like:

thoughts?
(I can make this commit if you agree)

mikemhenry

LGTM! +1 to @atravitz file layout suggestions

mikemhenry · 2025-05-28T17:16:49Z

openfe/tests/protocols/openmm/plain_md/test_simulation_slow.py

+    available_platforms,
+    tmpdir
+):
+    if platform not in available_platforms:


This is nice, eventually we will want to set things up so we can "force" the GPU tests so we can catch issues like the GPU drivers not working BUT for fast tests, I think this is a great idea

Not me unfortunately, there's a good chance this was you from a previous test :P

mikemhenry · 2025-05-28T17:25:57Z

openfe/tests/protocols/openmm/plain_md/test_tokenization.py

+class TestPlainMDProtocol(GufeTokenizableTestsMixin):
+    cls = openmm_md.PlainMDProtocol
+    key = None
+    repr = "PlainMDProtocol-"
+
+    @pytest.fixture()
+    def instance(self, protocol):
+        return protocol
+
+    def test_repr(self, instance):
+        """
+        Overwrites the base `test_repr` call to do a bit more.
+        """
+        assert isinstance(repr(instance), str)
+        assert self.repr in repr(instance)
+
+
+class TestPlainMDProtocolUnit(GufeTokenizableTestsMixin):
+    cls = openmm_md.PlainMDProtocolUnit
+    repr = "PlainMDProtocolUnit("
+    key = None
+
+    @pytest.fixture
+    def instance(self, protocol_unit):
+        return protocol_unit
+
+    def test_repr(self, instance):
+        """
+        Overwrites the base `test_repr` call to do a bit more.
+        """
+        assert isinstance(repr(instance), str)
+        assert self.repr in repr(instance)
+
+
+class TestPlainMDProtocolResult(GufeTokenizableTestsMixin):
+    cls = openmm_md.PlainMDProtocolResult
+    key = None
+    repr = "PlainMDProtocolResult-"
+
+    @pytest.fixture()
+    def instance(self, protocol_result):
+        return protocol_result
+
+    def test_repr(self, instance):
+        """
+        Overwrites the base `test_repr` call to do a bit more.
+        """
+        assert isinstance(repr(instance), str)
+        assert self.repr in repr(instance)


Maybe take all the duplicated bits and toss it into a class?

I'm going to raise an issue for this. It feels like we should just upstream the check to gufe if we're overriding the same method everywhere.

IAlibay · 2025-05-28T17:46:18Z

as far as file organization, I'd prefer to have the tests more closely match the code's file structure. so something like:

thoughts? (I can make this commit if you agree)

So I started with that file structure, but the only "issue" I found was that when you do abfes & ahfes you want to have them as separate folders but code-wise they exist within the one protocols folder. So that structure starts failing really quickly.

IAlibay · 2025-05-28T17:52:24Z

as far as file organization, I'd prefer to have the tests more closely match the code's file structure. so something like:
thoughts? (I can make this commit if you agree)

So I started with that file structure, but the only "issue" I found was that when you do abfes & ahfes you want to have them as separate folders but code-wise they exist within the one protocols folder. So that structure starts failing really quickly.

Just to follow-up here, assuming we don't really mind being loose with that structure (e.g. for abfes), I'm perfectly happy with that structure @atravitz

atravitz · 2025-05-28T20:36:50Z

So I started with that file structure, but the only "issue" I found was that when you do abfes & ahfes you want to have them as separate folders but code-wise they exist within the one protocols folder. So that structure starts failing really quickly.

I'm not sure I understand the issue - I'm fine with us adding tests/protocols/openmm_afe/test_abfe_protocol.py etc. - I still think that it's clearer which tests are supposed to cover which pieces of code. But let me know if I'm misunderstanding!

IAlibay · 2025-05-28T20:59:26Z

So I started with that file structure, but the only "issue" I found was that when you do abfes & ahfes you want to have them as separate folders but code-wise they exist within the one protocols folder. So that structure starts failing really quickly.

I'm not sure I understand the issue - I'm fine with us adding tests/protocols/openmm_afe/test_abfe_protocol.py etc. - I still think that it's clearer which tests are supposed to cover which pieces of code. But let me know if I'm misunderstanding!

To avoid confusion, I would like to have tests/protocols/openmm_abfe and tests/protocols/openmm_ahfe, even though there's only a single protocols/openmm_afe.

atravitz · 2025-05-28T21:23:51Z

ah okay, i'm fine with that! it still keeps things clear imo. i'll make those changes shortly.

IAlibay · 2025-05-28T21:33:42Z

ah okay, i'm fine with that! it still keeps things clear imo. i'll make those changes shortly.

Thanks!

github-actions · 2025-05-28T23:04:17Z

No API break detected ✅

IAlibay · 2025-05-28T23:04:41Z

Thanks @atravitz, I just renamed openmm_afe under tests to openmm_ahfe, that way it'll make it easier for when openmm_abfe gets added in #1045

IAlibay added 4 commits May 23, 2025 15:53

Address typo from previous PR

3243539

move tests around

32196b2

Add tokenization tests for MD protocol

1f3d447

update import for HAS_INTERNET

d85cf97

IAlibay requested a review from atravitz May 23, 2025 18:09

Add gas phase slow test for MD Protocol, and fix PDB writing

613a0e8

IAlibay changed the title ~~[WIP] Protocols test migration~~ Protocols test migration & MD Protocol simulation tests May 24, 2025

IAlibay added 3 commits May 24, 2025 22:17

Add complex sim long test

edf8280

Make file returns optional rather than not including fields

18fc056

revert trajectory file key rename, we'll do this later

7eaa6da

IAlibay requested a review from hannahbaumann May 25, 2025 21:41

IAlibay mentioned this pull request May 26, 2025

Adjust plain MD simulation PDB writing & production step counter #1302

Open

2 tasks

hannahbaumann reviewed May 27, 2025

View reviewed changes

jameseastwood assigned atravitz May 27, 2025

hannahbaumann approved these changes May 27, 2025

View reviewed changes

IAlibay added 2 commits May 27, 2025 18:37

Merge branch 'main' into ahfe-test-migration

db46003

Merge branch 'main' into ahfe-test-migration

7143483

jameseastwood assigned mikemhenry May 28, 2025

atravitz reviewed May 28, 2025

View reviewed changes

mikemhenry approved these changes May 28, 2025

View reviewed changes

atravitz and others added 2 commits May 28, 2025 15:25

reorganizing test directory structure

67e2fc7

AFE to AHFE

8963fd2

IAlibay mentioned this pull request May 28, 2025

Create new test class for serialization tests or upstream to gufe? #1308

Open

IAlibay merged commit 9533f95 into main May 29, 2025
12 of 13 checks passed

IAlibay deleted the ahfe-test-migration branch May 29, 2025 10:39

Conversation

IAlibay commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Developers certificate of origin

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

hannahbaumann left a comment

Choose a reason for hiding this comment

Uh oh!

IAlibay commented May 27, 2025

Uh oh!

atravitz left a comment

Choose a reason for hiding this comment

Uh oh!

mikemhenry left a comment

Choose a reason for hiding this comment

Uh oh!

mikemhenry May 28, 2025

Choose a reason for hiding this comment

Uh oh!

IAlibay May 28, 2025

Choose a reason for hiding this comment

Uh oh!

mikemhenry May 28, 2025

Choose a reason for hiding this comment

Uh oh!

IAlibay May 28, 2025

Choose a reason for hiding this comment

Uh oh!

IAlibay commented May 28, 2025

Uh oh!

IAlibay commented May 28, 2025

Uh oh!

atravitz commented May 28, 2025

Uh oh!

IAlibay commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atravitz commented May 28, 2025

Uh oh!

IAlibay commented May 28, 2025

Uh oh!

github-actions bot commented May 28, 2025

Uh oh!

IAlibay commented May 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

IAlibay commented May 23, 2025 •

edited

Loading

codecov bot commented May 23, 2025 •

edited

Loading

IAlibay commented May 28, 2025 •

edited

Loading