GlobalParameterState and changes for multi-restraint calculations #363

andrrizzi · 2018-07-21T23:45:44Z

Deprecated the signature of IComposableState._on_setattr to fix a bug where the objects were temporarily left in an inconsistent state when an exception was raised and caught.
Deprecated update_alchemical_charges in AlchemicalState in anticipation of the new implementation of the exact PME that will be based on the NonbondedForce offsets rather than updateParametersInContext().
Allow restraint force classes to be controlled by a parameter other than 'lambda_restraints'. This will enable multi-restraints simulations.
Implemented states.GlobalParametersState. This more or less generalize the logic of AlchemicalState and implement the "suffix" mechanism that we'll be able to use in multi-restraint calculations to control parameters such as lambda_restraints_restraint1, lambda_restraints_restraint2 and, in the future, in multiple alchemical region calculations. With this, the new RestraintState class in YANK can be simplified to:

class RestraintState(GlobalParameterState):
   lambda_restraints = GlobalParameterState.GlobalParameter('lambda_restraints', standard_value=1.0)

   @lambda_restraints.validator
   def lambda_restraints(self, instance, new_value):
       if not (0.0 <= new_value <= 1.0):
           raise ValueError('lambda_restraints must be between 0.0 and 1.0')
       return new_value

   # A constructor just to give parameters_name_suffix a more meaningful name.
   def __init__(restraint_name=None, **kwargs):
       super().__init__(parameters_name_suffix=restraint_name, **kwargs)

AlchemicalState will be similarly simplified, but we'll have to wait to do it until the new exact PME implementation will be in.

The default is still lambda_restraints, but this will enable multi-restraint simulations.

…callback. This breaks backwards compatibility so the old signature is deprecated. This change allows composable states to restore the old value if something goes wrong, thus leaving the object in a consistent state if eventual ComposableStateError are raised and catched.

In future versions, exact treatment of PME will be implemented with the NonbondedForce offset mechanism offered starting from OpenMM 7.3 instead of using updateParametersInContext so the argument will be meaningless.

This class will simplify a lot the implementation of AlchemicalState, RestraintState, and all other composable states that require controlling a global parameter. This class also add the logic to control parameters that have a specific suffix appended to their name. This feature will enable multi-restraint and multi-alchemical-region simulations in the future.

andrrizzi · 2018-07-22T02:11:01Z

The two failing tests (see #358) have been already fixed in #362 so we can ignore them here.

Lnaden · 2018-07-24T13:34:40Z

Is this ready for merging? If so, I am going to pull master into here to see if it fixes the tests. I want to make sure this is working as intended. Do you also want to try to fix some of the CodeClimate things its complaining about? I can take a pass through that if you have other things you are working on.

andrrizzi · 2018-07-24T14:47:32Z

Whops! I didn't notice the failing doctests, I can fix it. I've already merged master to fix a conflict with the release history.

…into multi-restraints

andrrizzi · 2018-07-24T21:03:20Z

Tests finally pass! Hopefully, this is the last time I need to write special code for Python 2.

Lnaden

A couple small things I found and have questions on, if you can justify why you think they should not be changed, thats also fine

Lnaden · 2018-07-25T12:03:13Z

openmmtools/forces.py

    """

    def __init__(self, restraint_parameters, restrained_atom_indices1,
-                 restrained_atom_indices2, *args, **kwargs):
+                 restrained_atom_indices2, controlling_parameter_name,


Can we make controlling_parameter_name have a default? E.g. controlling_parameter_name = 'lambda_restraints'? Or would we instead just want controlling_parameter_name = 'restraints' and the lambda_ prefix is assumed prepended?

Lnaden · 2018-07-25T12:05:22Z

openmmtools/forces.py


    """

    def __init__(self, energy_function, restraint_parameters,
-                 restrained_atom_indices1, restrained_atom_indices2):
+                 restrained_atom_indices1, restrained_atom_indices2,
+                 controlling_parameter_name='lambda_restraints'):


Ah, see, here in this subclass, controlling_parameter_name an optional keyword arg, which changes the call from the parent function. We should make it consistent in the base class as well

We should make it consistent in the base class as well

Sounds good! I'll move the default to the base class and use kwargs here. I'll have to keep it buried in kwargs in the base class though because Python 2 doesn't allow having both kwargs and keyword arguments.

Oh yeah... I forgot about that. Might be best to leave it then and it can be removed in 0.17 when we drop Py2 officially

Ah, I just rememberd why I did this. It's because I need to use controlling_parameter_name in the subclass so I need the default to be specified there. I wouldn't write the default in two places as it's harder to keep track of (i.e., if a dev changes only one of the two the code may become inconsistent and unintuitive).

Also a good point

Lnaden · 2018-07-25T12:15:50Z

openmmtools/forces.py

-        energy_function = 'lambda_restraints * ' + energy_function
-        super(RadiallySymmetricBondRestraintForce, self).__init__(restraint_parameters,
-                 [restrained_atom_index1], [restrained_atom_index2], energy_function)
+        energy_function = controlling_parameter_name + ' * ' + energy_function


So I just noticed a potential bug here. If the user passes in an energy_function which has additive terms and not wrapped in ( ), this could lead to unintended behavior, which would NOT throw an error. Off the top of my head, the only practical thing I can think of is some kind of external field or shifted function, but this is also an easy enough fix it may be worth fixing now.

e.g.

>>> energy_function = '(K/2)*r^2 + 5*r' >>> controlling_parameter_name = 'X` >>> controlling_parameter_name + ' * ' + energy_function X * (K/2)*r^2 + 5*r

the 5*r term won't be scaled.

Easy fix is just controlling_parameter_name + ' * (' + energy_function + ')'

Good catch! Will change it.

… parameter

Lnaden · 2018-07-25T14:49:03Z

Awesome, I think this is ready. Do we want to cut a release after this?

andrrizzi · 2018-07-25T15:49:53Z

I'd wait to release until we're sure these changes will work for the multi-restriants feature in YANK. I don't think we have an openmmtools-dev version uploaded, but we could run the tests locally after we've implemented the new features in YANK before releasing this.

Lnaden · 2018-07-26T12:15:53Z

Okay we'll hold off on the release. I'll use this branch for developing the new restraints in YANK. I'm on the fence about whether we should merge this or not. If we plan on making changes to this, then I would be hesitant to do so.

andrrizzi · 2018-07-26T12:55:10Z

That's fine I think! Let's remember to merge it before making other changes to openmmtools though.

These tests pass locally.

Fix Python 2.7 ABC reference by just removing it. Add a check to ensure the distance variable is in the energy_string

Expanded test to ensure deserialization correctly restores Python accessed properties Changed name of the CV Distance from `Distance` to `symmetric_restraint_distance` Removed the `restraint_parameters` property as it was not called by any test or implementation here or in YANK. It also would have been a pain to restore correctly. Added helper function for the regex search of energy strings for implementations to follow. Installed NetCDF4 from pip on windows due to odd bug in Conda's NetCDF4 on windows having DLL errors for... reasons?

Try again to fix windows netcdf. Pinning libnetcdf to the last good version the netcdf4 lib built against on CondaForge

Convert RadiallySymmetricRestraint Forces to CV Forces

…t in init.

Lnaden · 2018-08-08T14:36:22Z

HA! HAHAHAHAHAHA! Finally got the windows tests to pass!

andrrizzi · 2018-09-17T01:48:57Z

I've reverted the commits implementing the CV restraint conversion, which are blocking this PR. Once I merge this, I'll open a separate PR reverting the reverted commits that we can merge once we've figure out a way to solve the performance issues.

andrrizzi added 8 commits July 21, 2018 19:03

Fix typo

41a3145

Allow restraint forces to be controlled by an arbitrary parameter.

e68add2

The default is still lambda_restraints, but this will enable multi-restraint simulations.

Fix signature AlchemicalState._on_setattr.

a440ea2

Deprecate update_alchemical_charges argument of AlchemicalState.

2377556

In future versions, exact treatment of PME will be implemented with the NonbondedForce offset mechanism offered starting from OpenMM 7.3 instead of using updateParametersInContext so the argument will be meaningless.

Bump minor version: 0.16.0

84b3d85

Update docs and release history

e7c825c

Merge branch 'master' into multi-restraints

1c575a0

andrrizzi added 5 commits July 24, 2018 11:58

Do not raise deprecation warning in from_system if not necessary.

004bbb9

Fix doctests

694f66d

Merge branch 'multi-restraints' of github.com:choderalab/openmmtools …

32ed63a

…into multi-restraints

Fix tests

b0f219e

Python 2 compatibility fixes

1046305

Lnaden requested changes Jul 25, 2018

View reviewed changes

Lnaden approved these changes Jul 25, 2018

View reviewed changes

Wrap energy function in parenthesis before multiplying by controlling…

8ed7277

… parameter

Remove check which ignored test because openmm7.2 is actually out

e803f50

andrrizzi mentioned this pull request Jul 30, 2018

Extend default compatibility of integrators #364

Merged

Lnaden added 3 commits July 31, 2018 12:52

Convert HarmonicRestraint Forces to CV Forces

05f1bdc

These tests pass locally.

Fix typo in meta.yaml

fdc41d5

Make it way more clear about backwards compatibility breaking.

8726101

Fix Python 2.7 ABC reference by just removing it. Add a check to ensure the distance variable is in the energy_string

Lnaden added 11 commits August 1, 2018 08:22

Start trying to make serialization more stable.

e36b593

Fix unneeded import

3c7ba04

Try again to fix windows netcdf. Pinning libnetcdf to the last good version the netcdf4 lib built against on CondaForge

Stupid. netcdf. stupid

7686d1f

AHG!

77c6ac8

Merge pull request #365 from choderalab/cv-restraints

24841a0

Convert RadiallySymmetricRestraint Forces to CV Forces

Pin to the last build which worked for us

aa3d00f

Overkill build pin of netcdf4

f8d6ed8

Running out of things to pin for windows here...

b96e28f

I'm giving up after this

5c6a0aa

I'm giving up after this, maybe

c548fbf

Lnaden mentioned this pull request Aug 2, 2018

Convert Restraints to CV-Based choderalab/yank#1059

Merged

andrrizzi and others added 2 commits August 2, 2018 12:10

Pass parameter_name_suffix as positional argument to allow renaming i…

b64c7c5

…t in init.

Testing a different workaround for the NetCDF issues

e0afad7

andrrizzi mentioned this pull request Aug 25, 2018

Mulitiple Alchemical Regions #366

Closed

andrrizzi mentioned this pull request Sep 9, 2018

Tutorials #367

Merged

andrrizzi added 2 commits September 16, 2018 21:28

Revert changes from PR #365. These will be re-added in a separate PR.

bf6e799

Re-add some of the fixes that are necessary to merge this PR

8e14077

Merge branch 'master' into multi-restraints

f28b16a

andrrizzi merged commit 9e0841b into master Sep 17, 2018

andrrizzi added a commit that referenced this pull request Sep 17, 2018

Re-revert features implemented in PR #365 that were reverted in #363.

27b4fcb

andrrizzi mentioned this pull request Sep 17, 2018

[WIP] Convert restraint forces to inherit from CVCustomForce #369

Open

jchodera deleted the multi-restraints branch January 26, 2019 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GlobalParameterState and changes for multi-restraint calculations #363

GlobalParameterState and changes for multi-restraint calculations #363

andrrizzi commented Jul 21, 2018 •

edited

Loading

andrrizzi commented Jul 22, 2018

Lnaden commented Jul 24, 2018 •

edited

Loading

andrrizzi commented Jul 24, 2018

andrrizzi commented Jul 24, 2018 •

edited

Loading

Lnaden left a comment

Lnaden Jul 25, 2018

Lnaden Jul 25, 2018

andrrizzi Jul 25, 2018

Lnaden Jul 25, 2018

andrrizzi Jul 25, 2018

Lnaden Jul 25, 2018

Lnaden Jul 25, 2018

andrrizzi Jul 25, 2018

Lnaden commented Jul 25, 2018

andrrizzi commented Jul 25, 2018

Lnaden commented Jul 26, 2018

andrrizzi commented Jul 26, 2018

Lnaden commented Aug 8, 2018

andrrizzi commented Sep 17, 2018

GlobalParameterState and changes for multi-restraint calculations #363

GlobalParameterState and changes for multi-restraint calculations #363

Conversation

andrrizzi commented Jul 21, 2018 • edited Loading

andrrizzi commented Jul 22, 2018

Lnaden commented Jul 24, 2018 • edited Loading

andrrizzi commented Jul 24, 2018

andrrizzi commented Jul 24, 2018 • edited Loading

Lnaden 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lnaden commented Jul 25, 2018

andrrizzi commented Jul 25, 2018

Lnaden commented Jul 26, 2018

andrrizzi commented Jul 26, 2018

Lnaden commented Aug 8, 2018

andrrizzi commented Sep 17, 2018

andrrizzi commented Jul 21, 2018 •

edited

Loading

Lnaden commented Jul 24, 2018 •

edited

Loading

andrrizzi commented Jul 24, 2018 •

edited

Loading