Add SamplerState interface to OpenMM CustomCVForce #362

Lnaden · 2018-07-19T18:39:05Z

This PR gives the SamplerState a regular way to access the CustomCVForce variables and values which may appear in a Context.

Implement feature / fix bug
Add tests
Update documentation as needed
Update changelog

This PR gives the `SamplerState` a regular way to access the CustomCVForce variables and values which may appear in a `Context`.

Lnaden · 2018-07-19T18:40:10Z

This does not make OpenMMTools require OpenMM 7.3 yet, it should gracefully trap the existence, or lack thereof, of OpenMM 7.3

Lnaden · 2018-07-19T19:51:31Z

Are there any objections to me dropping these two lines from the setup.py file? They are causing build clashes between conda build and setuptools/pip trying to install some ancient version of certifi on top of the one conda downloads.

https://github.com/choderalab/openmmtools/blob/master/setup.py#L174-L175

andrrizzi · 2018-07-19T20:30:15Z

No problems here!

Lnaden · 2018-07-20T12:19:10Z

Implementation wise, how does this look?

andrrizzi

Looks good in general, but I think there are a couple of things that we should change before merging this.

andrrizzi · 2018-07-20T13:38:57Z

openmmtools/states.py

+        self._collective_variables = collective_variables if collective_variables else None
+
+    @property
+    def _test_positions_valid(self):


Maybe rename this as _are_positions_valid, which I believe is the most common name convention for these.

andrrizzi · 2018-07-20T13:41:12Z

openmmtools/states.py

+        self._kinetic_energy = openmm_state.getKineticEnergy()
+        self._set_collective_variables(context_state)
+
+    def _set_collective_variables(self, context_state):


Can you call this maybe _read_collective_variables to be consistent with _read_context_state?

andrrizzi · 2018-07-20T13:53:41Z

openmmtools/states.py

@@ -1843,7 +1867,7 @@ def update_from_context(self, context_state):
        """
        self._read_context_state(context_state, check_consistency=True)

-    def apply_to_context(self, context, ignore_velocities=False):
+    def apply_to_context(self, context, ignore_velocities=False, ignore_collective_variables=False):


I was remembering incorrectly and I thought there was an ignore_velocities in update_from_context() on top of apply_to_context. I wouldn't have an ignore_collective_variables argument here anyway. This function task is to write the context so it shouldn't also read information from it, and collective variables like the energies, are not written.

I'm now thinking we should also add optional ignore_positions, ignore_velocities, ignore_energies, and ignore_collective_variables argument in update_from_context(), similarly to OpenMM's Context.getState() to allow users to optimize reading off the GPU. We'll definitely need the ignore_collective_variables in any case.

andrrizzi · 2018-07-20T13:59:40Z

openmmtools/states.py

+            except AttributeError:
+                pass
+        # Trap no variables found (empty dict), return None
+        self._collective_variables = collective_variables if collective_variables else None


I would convert the defaultdict to a normal dict either here or in the SamplerState.collective_variable property since the user expects that type.

andrrizzi · 2018-07-20T14:03:38Z

openmmtools/states.py

+            The object to read. This only works with Context's for now,
+            but in the future, this may support OpenMM State objects as well.
+        """
+        if isinstance(context_state, openmm.State):


I think we should raise an exception if the user passes an openmm.State object and get_collective_variables=True. Otherwise, this will fail silently and end up with a value for SamplerState.collective_variables indistinguishable from having a System with no collective variables.

I don't really like this idea. This can lead the user to be mandated to use the get_collective_variables=False in every call to this function when the collective variables are less common.

SamplerState.collective_variables indistinguishable from having a System with no collective variables.

Yes, though I thought that was partially the point? If a user never supplies the information about a given state, then we can't use that. I don't like the idea of the users being required to now track collective variables, which are an optional construct in of themselves.

The user wouldn't have to set get_collective_variable=False in every call as long as he passes an actual Context object. Passing the State object is there exclusively for optimization (i.e. if I already have one around I don't have to read the info off the Context again).

In any case, I think avoiding silent failing would be worth forcing the users to add the extra option, and the default behavior should be the safest. With this implementation, for example, all our metropolized MCMCMoves are currently failing silently and not retrieving CV variables that should actually be available after apply has been performed.

andrrizzi · 2018-07-20T14:04:23Z

openmmtools/tests/test_states.py

-            #     sampler_state.positions[5:8] = [[2.0, 2.0, 2.0], [2.0, 2.0, 2.0], [2.0, 2.0, 2.0]] * pos_unit
-            #     assert sampler_state.positions.has_changed
-            #     assert np.all(old_unitless_positions[5:8] != sampler_state._unitless_positions[5:8])
+            if isinstance(sampler_state._positions._value, np.ndarray):


Thanks for re-activating these tests! I forgot about this.

Lnaden · 2018-07-20T19:30:03Z

Okay! Changes made, let me know what you think.

andrrizzi · 2018-07-20T19:49:05Z

Looks great, thanks for the changes!

With the current implementation now, the integrator MCMCMove tests should fail because of this line: https://github.com/choderalab/openmmtools/blob/master/openmmtools/mcmc.py#L718 .

We'll have to change the implementation of BaseIntegratorMove.apply() to pass a Context to make it work, or better, if we want to maintain the current performance, we'll have to add ignore_X=False arguments to SamplerState.update_from_context(), and substitute that line with

sampler_state.update_from_context(context_state, ignore_collective_variables=True)
sampler_state.update_from_context(context, ignore_positions=True, ignore_velocities=True, ignore_energy=True)

With repex, I think this would save us n_states positions+velocities read ops from the GPU w.r.t. simply passing the context.

andrrizzi · 2018-07-20T19:53:14Z

To put that into context, with the BaseIntegratorMoves we already have an OpenMM State around because we need to check that the potential energy is not NaN for the nan recovery feature.

Lnaden · 2018-07-20T20:20:16Z

I'm not sure I follow what you would prefer I do here, nor where I need to make the change

andrrizzi · 2018-07-20T20:37:26Z

Sorry, that was unclear. I'd prefer adding the rest of the ignore_X arguments to SamplerState.update_from_context() and use

sampler_state.update_from_context(context_state, ignore_collective_variables=True)
sampler_state.update_from_context(context, ignore_positions=True, ignore_velocities=True, ignore_energy=True)

to avoid degrading the performance of the integrator moves.

Lnaden · 2018-07-20T20:53:23Z

Okay, but would you want all those to default to True?

andrrizzi · 2018-07-20T21:02:30Z

I think the safer default is probably False, similarly to ignore_collective_variables, so that users will be forced to think about what they are not keeping up-to-date with the latest changes in the Context.

Lnaden · 2018-07-23T12:04:44Z

Having a thought. Would it be more clear to use inclusive keywords instead of exclusive? e.g. get_collective_variables=True instead of ignore_collective_variables=False

jchodera · 2018-07-23T12:39:02Z

That sounds clearer to me!

andrrizzi · 2018-07-23T12:42:46Z

Me and Levi just finished talking about this. I thought of the get_X=True variables at the beginning, but I changed my comments to suggests ignore_X=False because 1) it's consistent with apply_to_context, which uses ignore_velocities instead of set_velocities 2) it sounds clearer to me that the old values of the ignored attributes will be left untouched instead of being set to None.

andrrizzi · 2018-07-23T16:53:08Z

openmmtools/states.py

+        if ignore_collective_variables:
+            if isinstance(context_state, openmm.State):
+                # Invalidate if State is passed in
+                self._collective_variables = None


I'm not sure we want to do this. I'm worried about the fact that these two snippets:

sampler_state.update_from_context(context_state, ignore_collective_variables=True) sampler_state.update_from_context(context, ignore_positions=True, ignore_velocities=True)

and

sampler_state.update_from_context(context, ignore_positions=True, ignore_velocities=True) sampler_state.update_from_context(context_state, ignore_collective_variables=True)

will result in two different states. I think this mistake would be hard to find and it's easy to miss this behavior in the docs. I think it's safer to have always ignore_collective_variables leave the CV as it is because if the user is using the ignore_X attributes it means that he's updating everything by steps.

Hmm. I understand now that you were probably worried about the fact that the user won't be able to invalidate a particular attribute during the manual update. If this is the case, I'd rather have the properties setters allow to set (exclusively) the None value.

Because we support both State and Context objects, which now cause a loss of information, we have to do something to handle it. I don't want the user to by default be able to put the SamplerState into something inconsistent.

I understand now that you were probably worried about the fact that the user won't be able to invalidate a particular attribute during the manual update.

When would this be?

I'd rather have the properties setters allow to set (exclusively) the None value.

I can change that, but lets figure out all the use cases first.

I don't want the user to by default be able to put the SamplerState into something inconsistent.

This is already possible if the user does

sampler_state.update_from_context(context, ignore_collective_variables)

and context is an actual Context object. We must assume that users know what they're doing when they are requesting only partial information to optimize the communication with the GPU, but I'm worried an inconsistent behavior of the input arguments would trick also more advanced users (me included).

Lnaden · 2018-07-23T17:45:57Z

The collective variables should now be consistent with the other ignore_X flags. State objects are still the exception.

andrrizzi · 2018-07-23T17:50:34Z

Thanks! Feel free to merge when tests pass.

Lnaden · 2018-07-23T17:51:31Z

We keep having SegFaults during the _find_thermostat tests. Did we ever figure out why that happens on Travis only?

andrrizzi · 2018-07-23T17:54:21Z

Nope T.T I gave up after a couple of hours of trying different things. Last time that happened the errors magically disappeared with a new OpenMM version. I'll try to figure out what's going on separately anyway. Locally, all those tests pass here.

Lnaden · 2018-07-23T18:01:20Z

All the tests pass locally for me as well

Add SamplerState interface to OpenMM CustomCVForce

ccc4a2e

This PR gives the `SamplerState` a regular way to access the CustomCVForce variables and values which may appear in a `Context`.

Lnaden mentioned this pull request Jul 19, 2018

Planning and ID'ing the CVs we want to give Restraints choderalab/yank#1036

Open

Stop the setup.py from getting deps.

46ec139

andrrizzi requested changes Jul 20, 2018

View reviewed changes

Make requested changes

3ed678d

andrrizzi approved these changes Jul 20, 2018

View reviewed changes

This was referenced Jul 22, 2018

Fix failing travis tests on master #358

Closed

GlobalParameterState and changes for multi-restraint calculations #363

Merged

Add in new optimization hooks and features

574ba24

andrrizzi reviewed Jul 23, 2018

View reviewed changes

Make ignore_collective_variables consistent.

8e3d46b

Lnaden merged commit fde2b69 into master Jul 23, 2018

Lnaden deleted the SamplerStateCV branch July 23, 2018 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SamplerState interface to OpenMM CustomCVForce #362

Add SamplerState interface to OpenMM CustomCVForce #362

Lnaden commented Jul 19, 2018

Lnaden commented Jul 19, 2018

Lnaden commented Jul 19, 2018

andrrizzi commented Jul 19, 2018

Lnaden commented Jul 20, 2018

andrrizzi left a comment

andrrizzi Jul 20, 2018

andrrizzi Jul 20, 2018

andrrizzi Jul 20, 2018

andrrizzi Jul 20, 2018

andrrizzi Jul 20, 2018

Lnaden Jul 20, 2018

andrrizzi Jul 20, 2018 •

edited

Loading

andrrizzi Jul 20, 2018

Lnaden commented Jul 20, 2018

andrrizzi commented Jul 20, 2018

andrrizzi commented Jul 20, 2018

Lnaden commented Jul 20, 2018

andrrizzi commented Jul 20, 2018

Lnaden commented Jul 20, 2018

andrrizzi commented Jul 20, 2018

Lnaden commented Jul 23, 2018

jchodera commented Jul 23, 2018

andrrizzi commented Jul 23, 2018

andrrizzi Jul 23, 2018

Lnaden Jul 23, 2018

andrrizzi Jul 23, 2018

Lnaden commented Jul 23, 2018

andrrizzi commented Jul 23, 2018

Lnaden commented Jul 23, 2018

andrrizzi commented Jul 23, 2018

Lnaden commented Jul 23, 2018

Add SamplerState interface to OpenMM CustomCVForce #362

Add SamplerState interface to OpenMM CustomCVForce #362

Conversation

Lnaden commented Jul 19, 2018

Lnaden commented Jul 19, 2018

Lnaden commented Jul 19, 2018

andrrizzi commented Jul 19, 2018

Lnaden commented Jul 20, 2018

andrrizzi 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

andrrizzi Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lnaden commented Jul 20, 2018

andrrizzi commented Jul 20, 2018

andrrizzi commented Jul 20, 2018

Lnaden commented Jul 20, 2018

andrrizzi commented Jul 20, 2018

Lnaden commented Jul 20, 2018

andrrizzi commented Jul 20, 2018

Lnaden commented Jul 23, 2018

jchodera commented Jul 23, 2018

andrrizzi commented Jul 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lnaden commented Jul 23, 2018

andrrizzi commented Jul 23, 2018

Lnaden commented Jul 23, 2018

andrrizzi commented Jul 23, 2018

Lnaden commented Jul 23, 2018

andrrizzi Jul 20, 2018 •

edited

Loading