Protocol to run plain MD simulations #516

hannahbaumann · 2023-08-07T12:36:09Z

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 · 2023-08-08T08:32:31Z

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (2213b71) 92.82% compared to head (b16d91d) 91.38%.

Files	Patch %	Lines
.../tests/protocols/test_openmm_plain_md_protocols.py	95.08%	9 Missing ⚠️
openfe/protocols/openmm_md/plain_md_methods.py	97.14%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #516      +/-   ##
==========================================
- Coverage   92.82%   91.38%   -1.44%     
==========================================
  Files         128      132       +4     
  Lines        8711     9114     +403     
==========================================
+ Hits         8086     8329     +243     
- Misses        625      785     +160

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

richardjgowers · 2023-08-16T08:29:16Z

@hmacdope MD stuff here

dotsdl · 2023-08-17T04:50:12Z

Is this a Protocol intended for use with a NonTransformation? What is the meaning of having a stateA and stateB ChemicalSystem for this Protocol? Would they just be the same ChemicalSystem?

hannahbaumann · 2023-08-17T06:57:36Z

Yes, this is intended for use with NonTransformation and stateA and stateB are the same ChemicalSystem (which I should probably make more clear).

IAlibay

Super super late review - sorry @hannahbaumann

Finally got to looking at this last night. Overall this looks great thanks!

Just a couple of things, mostly some updates for changes that occured in main.

openfe/protocols/openmm_md/plain_md_methods.py

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

pep8speaks · 2023-11-03T09:23:11Z

Hello @hannahbaumann! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file openfe/protocols/openmm_md/plain_md_methods.py:

Line 157:80: E501 line too long (89 > 79 characters)
Line 392:80: E501 line too long (91 > 79 characters)
Line 396:80: E501 line too long (88 > 79 characters)
Line 493:80: E501 line too long (89 > 79 characters)
Line 550:80: E501 line too long (83 > 79 characters)
Line 595:80: E501 line too long (84 > 79 characters)
Line 596:80: E501 line too long (84 > 79 characters)
Line 597:80: E501 line too long (84 > 79 characters)
Line 598:80: E501 line too long (84 > 79 characters)
Line 599:80: E501 line too long (84 > 79 characters)
Line 600:80: E501 line too long (94 > 79 characters)

In the file openfe/protocols/openmm_utils/omm_settings.py:

Line 386:76: W291 trailing whitespace
Line 400:80: E501 line too long (86 > 79 characters)
Line 402:62: W291 trailing whitespace
Line 405:67: W291 trailing whitespace
Line 408:67: W291 trailing whitespace

In the file openfe/tests/data/openmm_md/__init__.py:

Line 1:1: W391 blank line at end of file

In the file openfe/tests/protocols/conftest.py:

Line 220:80: E501 line too long (81 > 79 characters)

In the file openfe/tests/protocols/test_openmm_plain_md_protocols.py:

Line 51:80: E501 line too long (91 > 79 characters)
Line 108:80: E501 line too long (83 > 79 characters)
Line 317:52: E251 unexpected spaces around keyword / parameter equals
Line 318:9: E128 continuation line under-indented for visual indent
Line 319:63: E251 unexpected spaces around keyword / parameter equals
Line 320:9: E128 continuation line under-indented for visual indent
Line 322:52: E251 unexpected spaces around keyword / parameter equals
Line 323:9: E128 continuation line under-indented for visual indent
Line 323:80: E501 line too long (94 > 79 characters)
Line 379:80: E501 line too long (90 > 79 characters)
Line 380:80: E501 line too long (81 > 79 characters)
Line 397:80: E501 line too long (90 > 79 characters)
Line 398:80: E501 line too long (81 > 79 characters)
Line 427:80: E501 line too long (93 > 79 characters)
Line 437:5: E303 too many blank lines (2)

In the file openfe/utils/system_probe.py:

Line 527:1: W391 blank line at end of file

Comment last updated at 2023-12-28 00:03:29 UTC

…nto plain_md

IAlibay · 2023-11-23T16:12:25Z

@hannahbaumann - I think I know what's going on with that runner, I'm just going to push an extra __init__.py file to debug.

IAlibay

I still need to do a proper review later, but here's a couple of things re: the __init__.py files.

openfe/protocols/openmm_md/__init__.py

openfe/tests/data/openmm_md/__init__.py

IAlibay

Reviewing in chunks to make it easier 😅 - here's the protocol results, just needs better return information.

IAlibay · 2023-12-07T09:38:03Z

openfe/protocols/openmm_md/plain_md_methods.py

+        """String of trajectory file name"""
+        traj = [pus[0].outputs['nc'] for pus in self.data.values()]
+
+        return traj


Definitely not for this PR, but something we should consider next is getting access to the various other files (e.g. equilibration, etc...). Then we should expand to do things like getting energy values etc...

openfe/protocols/openmm_md/plain_md_methods.py

IAlibay · 2023-12-07T09:39:12Z

openfe/protocols/openmm_md/plain_md_methods.py

+
+        return None
+
+    def get_traj_filename(self):


If possible, could you put a return annotation on this and/or in the docstring so that folks can easily check what they are getting back?

Added this, does that look ok?

openfe/protocols/openmm_md/plain_md_methods.py

IAlibay

second set of comments - protocol looks great, mostly a few comments about names in settings, but nothing major / that I think should significantly change

openfe/protocols/openmm_md/plain_md_methods.py

IAlibay · 2023-12-07T09:53:55Z

openfe/protocols/openmm_md/plain_md_methods.py

+
+        # Set barostat frequency to zero for NVT
+        for x in simulation.context.getSystem().getForces():
+            if x.getName() == 'MonteCarloBarostat':


Future TODO (not in this PR): we should expand this to cover all the barostats (I assume it won't work with flexible/anisotropic/membrane, etc.. variants).

IAlibay · 2023-12-07T11:30:50Z

openfe/protocols/openmm_md/plain_md_methods.py

+        # Save last frame NPT equilibration
+        positions = to_openmm(
+            from_openmm(simulation.context.getState(
+                getPositions=True, enforcePeriodicBox=False


Curiosity question - I've not played around with enforcePeriodicBox much, does it bring along significant advantages to disable it?

Hm, I'm not sure, this might depend on what the user wants to do with the trajectory/structure?
False is the default here and means "the position of each particle will be whatever position is stored in the Context, regardless of periodic boundary conditions".
So if the users want to e.g. visualize the structure afterwards, they might want to first remove pbc/make things whole.
Do you have a preference here?

We don't really expose this file currently, and it's likely only going to really get used for debugging purposes. I think it's probably fine to leave it as it is.

IAlibay · 2023-12-07T11:32:26Z

openfe/protocols/openmm_md/plain_md_methods.py

+        simulation.reporters.append(openmm.app.CheckpointReporter(
+            file=str(shared_basepath / simulation_settings.checkpoint_storage),
+            reportInterval=simulation_settings.checkpoint_interval.m))
+        simulation.reporters.append(openmm.app.StateDataReporter(


TODO: as discussed in a call a few weeks back, in a next PR we should hook up the results object to point to these and maybe have methods for reading data out for commonly needed things (reduced potential etc..)

openfe/protocols/openmm_utils/omm_settings.py

IAlibay

Just the one question on tests, otherwise it looks good to me :)

openfe/tests/protocols/test_openmm_plain_md_protocols.py

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

… settings

IAlibay

Just the one thing, but otherwise this lgtm!

openfe/protocols/openmm_md/plain_md_methods.py

IAlibay · 2023-12-11T15:05:06Z

openfe/protocols/openmm_md/plain_md_methods.py

+        # Save last frame NPT equilibration
+        positions = to_openmm(
+            from_openmm(simulation.context.getState(
+                getPositions=True, enforcePeriodicBox=False


We don't really expose this file currently, and it's likely only going to really get used for debugging purposes. I think it's probably fine to leave it as it is.

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

IAlibay · 2023-12-28T16:21:16Z

Going ahead with a merge here - waiting on this longer will hold up, amongst other things, #614

IAlibay · 2023-12-28T16:21:30Z

Thanks so much for getting this done @hannahbaumann !

hannahbaumann added 17 commits July 31, 2023 13:16

start plain MD scripts

1610300

Add tests, fixing some errors in methods

e64fb75

Add gathertest

86a60c9

Add StateDataReporter

b9c8413

Cleanup functions

f33e21d

fix integrator units

72ee364

fix setvelocity unit

1516300

test reporter

b9f9b6d

change reporter to xtc

033a9ac

change reporter to dcd

3901dfa

clean up reporters

dbcacf3

Add checkpoint reporter

7b4c04d

change reporter to xtc

95b9c16

Fix reporters

96b69f0

Fix StateReporter

fca9c23

Add output indices traj and other fixes

48b626c

merge changes

ed79587

hannahbaumann requested a review from IAlibay August 7, 2023 12:37

hannahbaumann and others added 2 commits August 8, 2023 10:19

Merge branch 'main' into plain_md

ac15045

changes test

0b61cbf

Merge branch 'main' into plain_md

559d18d

richardjgowers self-assigned this Oct 2, 2023

IAlibay requested changes Nov 2, 2023

View reviewed changes

IAlibay mentioned this pull request Nov 2, 2023

[WIP] Do an MD pre-equilibration for AFE simulations #614

Closed

4 tasks

style_fix

e77cad6

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

hannahbaumann and others added 7 commits November 22, 2023 16:48

small fix2

5b9b572

Merge branch 'main' into plain_md

ed0c5b5

Add test for result json file

3996e37

Merge branch 'plain_md' of https://github.com/OpenFreeEnergy/openfe i…

eca1b01

…nto plain_md

Add results json files for MD run

27b9b42

Add tests for logging output

2d6e38e

Add init files

45890d4

Merge branch 'main' into plain_md

552e31b

IAlibay reviewed Nov 23, 2023

View reviewed changes

openfe/protocols/openmm_md/__init__.py Outdated Show resolved Hide resolved

openfe/tests/data/openmm_md/__init__.py Outdated Show resolved Hide resolved

Changes __init__.py

f5e6d99

IAlibay requested changes Dec 7, 2023

View reviewed changes

IAlibay reviewed Dec 7, 2023

View reviewed changes

openfe/tests/protocols/test_openmm_plain_md_protocols.py Outdated Show resolved Hide resolved

hannahbaumann and others added 7 commits December 7, 2023 16:06

add return annotation for results

146b8d5

Update openfe/protocols/openmm_md/plain_md_methods.py

71f09d1

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

make settings names more specific

085919d

change results.json file with new settings names

f21a7b1

change run method to take temp and barostat_frequency instead of full…

7f46b7f

… settings

add slow test

f8ec471

fix test

39803e5

IAlibay approved these changes Dec 11, 2023

View reviewed changes

hannahbaumann and others added 4 commits December 12, 2023 11:37

Update openfe/protocols/openmm_md/plain_md_methods.py

b9c924b

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

Change purepath to path

4704336

Change PurePath to Path

64109d8

Merge branch 'main' into plain_md

b16d91d

IAlibay merged commit 5a2c339 into main Dec 28, 2023
8 of 9 checks passed

IAlibay deleted the plain_md branch December 28, 2023 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protocol to run plain MD simulations #516

Protocol to run plain MD simulations #516

hannahbaumann commented Aug 7, 2023 •

edited

Loading

codecov bot commented Aug 8, 2023 •

edited

Loading

richardjgowers commented Aug 16, 2023

dotsdl commented Aug 17, 2023

hannahbaumann commented Aug 17, 2023

IAlibay left a comment

pep8speaks commented Nov 3, 2023 •

edited

Loading

IAlibay commented Nov 23, 2023

IAlibay left a comment

IAlibay left a comment

IAlibay Dec 7, 2023

IAlibay Dec 7, 2023

hannahbaumann Dec 7, 2023

IAlibay left a comment

IAlibay Dec 7, 2023

IAlibay Dec 7, 2023

hannahbaumann Dec 7, 2023

IAlibay Dec 11, 2023

IAlibay Dec 7, 2023

IAlibay left a comment

IAlibay left a comment

IAlibay Dec 11, 2023

IAlibay commented Dec 28, 2023

IAlibay commented Dec 28, 2023

Protocol to run plain MD simulations #516

Protocol to run plain MD simulations #516

Conversation

hannahbaumann commented Aug 7, 2023 • edited Loading

Developers certificate of origin

codecov bot commented Aug 8, 2023 • edited Loading

Codecov Report

richardjgowers commented Aug 16, 2023

dotsdl commented Aug 17, 2023

hannahbaumann commented Aug 17, 2023

IAlibay left a comment

Choose a reason for hiding this comment

pep8speaks commented Nov 3, 2023 • edited Loading

Comment last updated at 2023-12-28 00:03:29 UTC

IAlibay commented Nov 23, 2023

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay commented Dec 28, 2023

IAlibay commented Dec 28, 2023

hannahbaumann commented Aug 7, 2023 •

edited

Loading

codecov bot commented Aug 8, 2023 •

edited

Loading

pep8speaks commented Nov 3, 2023 •

edited

Loading