Renaming and moving some of the protocol settings #689

hannahbaumann · 2024-01-17T15:47:52Z

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

pep8speaks · 2024-01-17T15:47:58Z

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

In the file devtools/data/gen-serialized-results.py:

Line 97:80: E501 line too long (106 > 79 characters)
Line 98:80: E501 line too long (105 > 79 characters)

In the file openfe/protocols/openmm_afe/base.py:

Line 640:80: E501 line too long (82 > 79 characters)
Line 687:80: E501 line too long (89 > 79 characters)
Line 690:80: E501 line too long (90 > 79 characters)
Line 801:80: E501 line too long (87 > 79 characters)
Line 814:80: E501 line too long (86 > 79 characters)
Line 815:80: E501 line too long (98 > 79 characters)
Line 950:80: E501 line too long (83 > 79 characters)

In the file openfe/protocols/openmm_afe/equil_afe_settings.py:

Line 54:80: E501 line too long (80 > 79 characters)
Line 121:72: W291 trailing whitespace
Line 122:70: W291 trailing whitespace
Line 123:79: W291 trailing whitespace

In the file openfe/protocols/openmm_afe/equil_solvation_afe_method.py:

Line 400:80: E501 line too long (83 > 79 characters)
Line 401:80: E501 line too long (80 > 79 characters)
Line 621:80: E501 line too long (81 > 79 characters)
Line 623:80: E501 line too long (80 > 79 characters)
Line 626:80: E501 line too long (90 > 79 characters)
Line 627:80: E501 line too long (88 > 79 characters)
Line 753:80: E501 line too long (82 > 79 characters)
Line 837:80: E501 line too long (83 > 79 characters)

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

Line 394:80: E501 line too long (87 > 79 characters)
Line 398:80: E501 line too long (84 > 79 characters)
Line 470:80: E501 line too long (82 > 79 characters)
Line 550:80: E501 line too long (86 > 79 characters)
Line 596:80: E501 line too long (87 > 79 characters)
Line 597:80: E501 line too long (87 > 79 characters)
Line 598:80: E501 line too long (87 > 79 characters)
Line 599:80: E501 line too long (87 > 79 characters)
Line 600:80: E501 line too long (87 > 79 characters)
Line 601:80: E501 line too long (97 > 79 characters)

In the file openfe/protocols/openmm_rfe/_rfe_utils/lambdaprotocol.py:

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

In the file openfe/protocols/openmm_rfe/equil_rfe_methods.py:

Line 632:80: E501 line too long (94 > 79 characters)
Line 804:80: E501 line too long (88 > 79 characters)
Line 831:80: E501 line too long (82 > 79 characters)
Line 877:80: E501 line too long (82 > 79 characters)
Line 886:80: E501 line too long (89 > 79 characters)
Line 890:80: E501 line too long (107 > 79 characters)
Line 951:80: E501 line too long (84 > 79 characters)
Line 985:80: E501 line too long (85 > 79 characters)

In the file openfe/protocols/openmm_rfe/equil_rfe_settings.py:

Line 76:72: W291 trailing whitespace
Line 85:77: W291 trailing whitespace
Line 111:72: W291 trailing whitespace
Line 112:70: W291 trailing whitespace
Line 113:79: W291 trailing whitespace

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

Line 96:80: E501 line too long (82 > 79 characters)
Line 271:80: E501 line too long (80 > 79 characters)
Line 273:80: E501 line too long (94 > 79 characters)
Line 291:1: W293 blank line contains whitespace
Line 293:80: E501 line too long (105 > 79 characters)
Line 297:80: W291 trailing whitespace
Line 298:79: W291 trailing whitespace
Line 299:25: W291 trailing whitespace
Line 304:80: E501 line too long (88 > 79 characters)
Line 308:17: W291 trailing whitespace
Line 309:1: W293 blank line contains whitespace
Line 392:62: W291 trailing whitespace

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

Line 92:80: E501 line too long (80 > 79 characters)
Line 97:80: E501 line too long (90 > 79 characters)
Line 98:80: E501 line too long (87 > 79 characters)
Line 130:80: E501 line too long (80 > 79 characters)
Line 135:80: E501 line too long (89 > 79 characters)
Line 139:80: E501 line too long (108 > 79 characters)
Line 140:80: E501 line too long (110 > 79 characters)
Line 143:80: E501 line too long (97 > 79 characters)
Line 146:80: E501 line too long (116 > 79 characters)
Line 147:80: E501 line too long (110 > 79 characters)

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

Line 69:80: E501 line too long (86 > 79 characters)

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

Line 29:80: E501 line too long (84 > 79 characters)

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

Line 427:80: E501 line too long (89 > 79 characters)
Line 469:80: E501 line too long (89 > 79 characters)

In the file openfecli/tests/commands/test_gather.py:

Line 213:80: E501 line too long (100 > 79 characters)

Comment last updated at 2024-02-06 11:50:51 UTC

hannahbaumann · 2024-01-18T08:38:18Z

openfe/protocols/openmm_utils/omm_settings.py

+    steps_per_iteration = 250 * 4 fs
+    --> The free energy would be analyzed every 250 ps (250 * 250 * 4 fs)
+    """
+    early_termination_target_error: FloatQuantity = 0.0 * unit.boltzmann_constant * unit.kelvin


Is this unit correct or does it need to be changed? Should this be 0.0 * boltzmann_constant * temperature * unit.boltzmann_constant * unit.kelvin? Not sure...

Change this to unit kcal/mol

openfe/protocols/openmm_rfe/equil_rfe_settings.py

richardjgowers · 2024-01-19T10:29:53Z

openfe/protocols/openmm_rfe/equil_rfe_settings.py

-    Simulation control settings, including simulation lengths and record-keeping.
+    Simulation control settings, including simulation lengths.
+    """
+    output_settings: OutputSettings


can probably wait, but this OutputSettings idea could percolate back to gufe where iirc there's a proto-Settings object which tries to define the pattern all should follow

richardjgowers · 2024-01-19T10:31:19Z

openfe/protocols/openmm_utils/omm_settings.py

+    Number of integration timesteps between each time the MCMC move
+    is applied. Default 250 * unit.timestep.
+    """
+    real_time_analysis_interval: Optional[int] = 250


This isn't unit.timestep as it's a multiple of steps_per_iteration right? So the default is 250 * 250?

I think this is answered in the docstring below >.<

richardjgowers · 2024-01-19T10:33:27Z

openfe/protocols/openmm_utils/omm_settings.py

+    steps_per_iteration = 250 * 4 fs
+    --> The free energy would be analyzed every 250 ps (250 * 250 * 4 fs)
+    """
+    early_termination_target_error: FloatQuantity = 0.0 * unit.boltzmann_constant * unit.kelvin


maybe can wait, but possibly making this Optional is a better way to toggle the behaviour on/off, rather than using 0.0

openfe/protocols/openmm_utils/omm_settings.py

hannahbaumann · 2024-01-19T12:31:49Z

openfe/protocols/openmm_utils/omm_settings.py

@@ -134,62 +134,72 @@ class Config:
    or `independent` (independently sampled lambda windows).
    Default `repex`.
    """
-    online_analysis_interval: Optional[int] = 250
+    steps_per_iteration = 250 * unit.timestep  # todo: IntQuantity


For this, would it technically also be 250 * timestep * unit.timestep? Or just make it an IntQuantity that has to be used together with timestep to be a useful quantity?

hannahbaumann · 2024-01-19T12:53:53Z

openfe/protocols/openmm_utils/omm_settings.py

-    """
-    Number of integration timesteps between each time the MCMC move
-    is applied. Default 250 * unit.timestep.
-    """
    reassign_velocities = False


Potentially move to AlchemicalSamplerSettings or change doc string to explain what move means here.

is this for reassign_velocities? seems like an integrator thing

reassign velocities is specific to the behaviour that exists when doing multi state moves, not the timestep integration itself, that makes it a property of the sampler not the integrator

…step

openfe/protocols/openmm_rfe/equil_rfe_methods.py

IAlibay

Second half of review

IAlibay · 2024-02-05T09:24:12Z

openfe/protocols/openmm_afe/base.py

@@ -234,10 +235,11 @@ def _handle_settings(self):
          * solvation_settings : SolvationSettings
          * alchemical_settings : AlchemicalSettings
          * lambda_settings : LambdaSettings
-          * sampler_settings : AlchemicalSamplerSettings
+          * sampler_settings : MultiStateSimulationSettings


Please remove both sampler_settings and system_settings

IAlibay · 2024-02-05T09:25:48Z

openfe/protocols/openmm_afe/base.py


        Returns
        -------
        integrator : openmmtools.mcmc.LangevinDynamicsMove
          A configured integrator object.
        """
+        # TODO: Check this is correct
+        tpi_fs = simulation_settings.time_per_iteration.to(unit.femtosecond).m


Shouldn't this be using whatever got added to openmm_utils?

IAlibay · 2024-02-05T09:30:21Z

openfe/protocols/openmm_afe/equil_afe_settings.py

+from typing import (
+    Literal,
+    Optional,
+)


Not seeing either of these being used in the diff?

openfe/protocols/openmm_afe/equil_solvation_afe_method.py

IAlibay · 2024-02-05T09:36:50Z

openfe/protocols/openmm_utils/settings_validation.py

@@ -71,3 +71,95 @@ def get_simsteps(sim_length: unit.Quantity,
        raise ValueError(errmsg)

    return sim_steps
+
+
+def convert_steps_per_iteration(


Going over the MD protocol - I'm probably missing something really obvious but isn't this method exactly the same as using get_simsteps but with an MC value of 1 (like we do there)?

IAlibay · 2024-02-05T17:08:37Z

devtools/data/gen-serialized-results.py

    settings.vacuum_simulation_settings.equilibration_length = 10 * unit.picosecond
-    settings.vacuum_simulation_settings.production_length = 1000 * unit.picosecond
+    settings.vacuum_simulation_settings.production_length = 10 * unit.picosecond


I've tried this several times below 1 ns and it fails on the regular. It ends up wasting a lot more time re-running everything than just running the full nanosecond.

codecov · 2024-02-06T09:16:48Z

Codecov Report

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

Comparison is base (443fcf2) 91.36% compared to head (2a948cc) 91.43%.
Report is 36 commits behind head on main.

Files	Patch %	Lines
openfe/protocols/openmm_utils/omm_settings.py	82.27%	14 Missing ⚠️
openfe/tests/protocols/test_openmm_afe_slow.py	0.00%	8 Missing ⚠️
...tests/protocols/test_openmm_equil_rfe_protocols.py	88.00%	3 Missing ⚠️
openfe/protocols/openmm_afe/base.py	90.90%	2 Missing ⚠️
openfe/protocols/openmm_afe/equil_afe_settings.py	94.73%	2 Missing ⚠️
openfe/protocols/openmm_md/plain_md_settings.py	87.50%	2 Missing ⚠️
openfe/protocols/openmm_rfe/equil_rfe_methods.py	90.00%	2 Missing ⚠️
openfe/protocols/openmm_rfe/equil_rfe_settings.py	91.30%	2 Missing ⚠️
...ts/protocols/test_openmm_afe_solvation_protocol.py	97.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #689      +/-   ##
==========================================
+ Coverage   91.36%   91.43%   +0.07%     
==========================================
  Files         132      132              
  Lines        9174     9333     +159     
==========================================
+ Hits         8382     8534     +152     
- Misses        792      799       +7

Flag	Coverage Δ
fast-tests	`91.43% <91.59%> (+0.07%)`	⬆️

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.

IAlibay

Looks like the docs build is failing because of missing references to settings?

IAlibay

Couple of extra comments, none blocking.

Would be nice to think about moving non-unit conversions somewhere eventually - it's going to be necessary going forward to have a good place to do kt->kj->kcal->kt etc...

IAlibay · 2024-02-06T09:35:41Z

openfe/protocols/openmm_afe/equil_afe_settings.py

+    def must_be_positive(cls, v):
+        if v <= 0:
+            errmsg = f"protocol_repeats must be a positive value, got {v}."
+            raise ValueError(errmsg)


Would it be worth a test to check that the validators kick in properly at this level?

IAlibay · 2024-02-06T09:37:23Z

openfe/protocols/openmm_rfe/equil_rfe_methods.py

        integrator_settings: IntegratorSettings = protocol_settings.integrator_settings

        # is the timestep good for the mass?
        settings_validation.validate_timestep(
            forcefield_settings.hydrogen_mass,
            integrator_settings.timestep
        )
+        # TODO: Also validate various conversions?


If possible, please raise an issue if there's a todo.

IAlibay · 2024-02-06T09:41:12Z

openfe/protocols/openmm_utils/omm_settings.py

    def must_be_positive_or_zero(cls, v):
        if v < 0:
-            errmsg = ("collision_rate, and n_restart_attempts must be "
-                      "zero or positive values")
+            errmsg = ("langevin_collision_rate, and n_restart_attempts must be"


I won't point to all the codecov stuff - but there's a lot of stuff that's not covered, worth raising an issue to make sure we test it all?

IAlibay · 2024-02-06T09:59:48Z

openfe/protocols/openmm_utils/settings_validation.py

+    return rta_its, rta_min_its
+
+
+def convert_target_error_from_kcal_per_mole_to_kT(


I'm not going to block here, but would it make sense rename this to be "convert_kcal_per_mole_to_kT" instead? Long term is might be good if we had a series of conversions somewhere else that we can use.

IAlibay

lgtm!

Initial commit to rename protocol settings

189815e

hannahbaumann linked an issue Jan 17, 2024 that may be closed by this pull request

Review settings names #665

Closed

hannahbaumann added 2 commits January 18, 2024 09:18

more settings changes

7a4266e

Transfer changes in AlchemicalSamplerSettings to different protocols

1ed1ea9

hannahbaumann commented Jan 18, 2024

View reviewed changes

openfe/protocols/openmm_rfe/equil_rfe_settings.py Show resolved Hide resolved

hannahbaumann added 4 commits January 18, 2024 09:53

Change n_steps in protocols

a16a91e

Propagate changes in OutputSettings to protocols and tests

190eec9

change validator

eb238e3

fixes new settings

d499356

richardjgowers reviewed Jan 19, 2024

View reviewed changes

openfe/protocols/openmm_rfe/equil_rfe_settings.py Outdated Show resolved Hide resolved

richardjgowers reviewed Jan 19, 2024

View reviewed changes

openfe/protocols/openmm_rfe/equil_rfe_settings.py Outdated Show resolved Hide resolved

richardjgowers reviewed Jan 19, 2024

View reviewed changes

openfe/protocols/openmm_utils/omm_settings.py Show resolved Hide resolved

richardjgowers added this to the 1.0.0 milestone Jan 19, 2024

hannahbaumann added 2 commits January 19, 2024 13:23

small changes

768b4b3

changes protocol_repeats

d7145d8

hannahbaumann commented Jan 19, 2024

View reviewed changes

hannahbaumann added 3 commits January 19, 2024 13:40

add values to validators

ff84000

small fix

2566438

change doc string minimum iterations

b22c51a

hannahbaumann commented Jan 19, 2024

View reviewed changes

hannahbaumann added 2 commits January 19, 2024 14:02

more renaming online_analysis to real_time and pep8 fixes

2dd0faa

for MD protocol use mcmc_steps=1 since only used for testing the time…

b39e6e3

…step

hannahbaumann commented Jan 23, 2024

View reviewed changes

openfe/protocols/openmm_rfe/equil_rfe_methods.py Outdated Show resolved Hide resolved

hannahbaumann added 2 commits January 24, 2024 12:58

changes softcore_LJ

e55ce2c

Fixes protocol_repeats

4d08133

IAlibay requested changes Feb 5, 2024

View reviewed changes

richardjgowers added 6 commits February 5, 2024 10:20

document dev scripts for generating jsons

55981bb

fixup usage of 'sampler_settings' in AFE base

60a42bc

clean up some imports

c8acd48

use convert_steps_per_iteration in afe base

3aa4581

works on validators and unit conversions

238bedf

fixup on unit conversion

940424f

richardjgowers mentioned this pull request Feb 5, 2024

AFE protocol requires validator to check FF settings #702

Open

richardjgowers added 6 commits February 5, 2024 11:31

remove commented out SamplerSettings block

e0fabfe

added tests for settings_validation conversions

6db0c53

got kT conversion upside down

8bc76b1

small fixups for new settings

0b858df

update result jsons for tests

0fa3995

shorten AHFE time in dev scripts

03c4a17

IAlibay reviewed Feb 5, 2024

View reviewed changes

richardjgowers changed the title ~~[ WIP] Renaming and moving some of the protocol settings~~ Renaming and moving some of the protocol settings Feb 5, 2024

richardjgowers added 4 commits February 5, 2024 17:12

Merge branch 'main' into rename_settings

8457b78

shorten AHFE time in dev scripts

5f25fd4

update openfecli test files for new settings

fcf5735

revert changes to gen-serialized-results.py

cc3e22e

richardjgowers requested a review from IAlibay February 6, 2024 00:10

update away from nose test style

ae5374e

IAlibay requested changes Feb 6, 2024

View reviewed changes

IAlibay reviewed Feb 6, 2024

View reviewed changes

richardjgowers added 2 commits February 6, 2024 11:50

fixup LambdaSettings docstrings

8c84c7b

remove doc stubs for removed settings

2a948cc

IAlibay approved these changes Feb 6, 2024

View reviewed changes

richardjgowers merged commit 2ab9b43 into main Feb 6, 2024
9 checks passed

richardjgowers deleted the rename_settings branch February 6, 2024 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renaming and moving some of the protocol settings #689

Renaming and moving some of the protocol settings #689

hannahbaumann commented Jan 17, 2024

pep8speaks commented Jan 17, 2024 •

edited

Loading

hannahbaumann Jan 18, 2024 •

edited

Loading

hannahbaumann Jan 30, 2024

richardjgowers Jan 19, 2024

richardjgowers Jan 19, 2024

richardjgowers Jan 19, 2024

richardjgowers Jan 19, 2024

hannahbaumann Jan 19, 2024 •

edited

Loading

hannahbaumann Jan 19, 2024

richardjgowers Feb 2, 2024

IAlibay Feb 2, 2024

IAlibay left a comment

IAlibay Feb 5, 2024

IAlibay Feb 5, 2024

IAlibay Feb 5, 2024

IAlibay Feb 5, 2024

IAlibay Feb 5, 2024

codecov bot commented Feb 6, 2024 •

edited

Loading

IAlibay left a comment

IAlibay left a comment

IAlibay Feb 6, 2024

IAlibay Feb 6, 2024

IAlibay Feb 6, 2024

IAlibay Feb 6, 2024 •

edited

Loading

IAlibay left a comment

		return rta_its, rta_min_its


		def convert_target_error_from_kcal_per_mole_to_kT(

Renaming and moving some of the protocol settings #689

Renaming and moving some of the protocol settings #689

Conversation

hannahbaumann commented Jan 17, 2024

Developers certificate of origin

pep8speaks commented Jan 17, 2024 • edited Loading

Comment last updated at 2024-02-06 11:50:51 UTC

hannahbaumann Jan 18, 2024 • edited Loading

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

hannahbaumann Jan 19, 2024 • edited Loading

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

codecov bot commented Feb 6, 2024 • edited Loading

Codecov Report

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 Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

pep8speaks commented Jan 17, 2024 •

edited

Loading

hannahbaumann Jan 18, 2024 •

edited

Loading

hannahbaumann Jan 19, 2024 •

edited

Loading

codecov bot commented Feb 6, 2024 •

edited

Loading

IAlibay Feb 6, 2024 •

edited

Loading