ThermodynamicState support for more barostats #437

Olllom · 2019-09-24T16:00:31Z

Description

Thermodynamic state supports

MonteCarloAnistropicBarostat with same pressure along all axes
MonteCarloMembraneBarostat with zero surface tension

see #436

Todos

Implement feature / fix bug
Add tests
Update documentation as needed
Update changelogNotable points that this PR has either accomplished or will accomplish.

Status

Ready to go

- supports MonteCarloAnistropicBarostat with same pressure along all axes - supports MonteCarloMembraneBarostat with zero surface tension

andrrizzi

Thanks so much for the fixes and for taking the time to add comprehensive tests. I've added only a couple of minor comments where I think the code has a possible bug or made a little more robust.

Fixing the docstrings and the changelog to make clear that the new barostats are supported would also be very helpful.

andrrizzi · 2019-09-24T20:15:20Z

openmmtools/states.py

        else:
            self._pressure = pressure  # Pressure here can also be None.
+        self._barostat_type = openmm.MonteCarloBarostat


What do you think about making _barostat_type a private property that simply returns type(self.barostat)? Because this attribute is accessed very rarely, I'd prefer to make it slightly slower but avoid redundancies.

I would rather like to keep it as it is. The reason is that _initialize calls _is_barostat_consistent indirectly (through _unsafe_set_system and _check_system_consistency_). At this point, self.barostat is not available, since self._standard_system is not initialized, yet.

I see what you mean. It may be safe after all though because, if I understand correctly the code I wrote, _is_barostat_consistent shouldn't be called when self.barostat is unavailable (I'm looking at these lines: https://github.com/Olllom/openmmtools/blob/other_barostats/openmmtools/states.py#L1341-L1342).

andrrizzi · 2019-09-24T20:19:09Z

openmmtools/states.py

+                scaled = [barostat.getScaleX(), barostat.getScaleY(), barostat.getScaleY()]
+                if sum(scaled) == 0:
+                    raise ThermodynamicsError(ThermodynamicsError.UNSUPPORTED_ANISOTROPIC_BAROSTAT)
+                active_pressures = [pressure for pressure, active in zip(pressures, scaled)]


active here is unused.

You probably wanted to add an if active at the end of the list comprehension.

Good catch! Thank you.

andrrizzi · 2019-09-24T22:30:14Z

As an extension (but feel free to postpone this to a future PR if you don't need it) we could allow self.pressure to be a vector to support different values of pressures for the axes of the anisotropic MC barostat.

Olllom

allow self.pressure to be a vector to support different values of pressures for the axes of the anisotropic MC barostat.

I am not sure what the pV term should look like in the reduced potential for anisotropic pressure. From what I see in textbooks, it should probably be the arithmetic mean of the pressures in all scaled directions. However, the implementation of the MonteCarloAnisotropicBarostat confuses me (if the pressure in one direction is zero, there is no energetic penalty of scaling the overall volume along that axis, which would point to using a geometric rather than an arithmetic mean).
I am happy to add full support for anisotropic pressure once I know that answer.

Olllom · 2019-09-26T14:42:40Z

An easier and more relevant addition for us would be nonzero surface tension. I will add some code for this to the pull request.

andrrizzi · 2019-09-26T14:50:35Z

Good point about the pV work. Feel very free to implement only the bits you need. This is already very helpful.

Olllom · 2019-09-27T15:06:01Z

I added

support for non-zero surface tension
support for setting the platform properties in the context cache and in the thermodynamic state

Point 2. should enable multistate simulations on CUDA in mixed and double precision,
by binding the MCMC moves to caches created with the 'precision'

It also makes it unnecessary to reduce CUDA_VISIBLE_DEVICES to a single device in multinode-gpu simulations. Instead the device ID can be set as a platform property.

Olllom · 2019-09-27T17:50:44Z

This PR is ready to go on my end.

andrrizzi

Love the new changes, thank you so much! The failing test is unrelated so you can ignore it.

The only possibly major things are these:

ThermodynamicState.apply_to_context() modifies the surface tension as well, but with the current code, ContextCache won't be able to recognize that it can effectively transform a context to use a different surface tension. To do this you'll have to add a _STANDARD_SURFACE_TENSION (like here) and modify ThermodynamicState._standardize_system(). In this way, the hash of a thermodynamic state won't change if you alter only the surface tension.
I still think _barostat_type should be better implemented as a property. The fact that we can have redundant info and inconsistencies in the internal state of the object makes me a little uncomfortable. A couple of examples:
- If we call _unsafe_set_system(system, fix_state=True) with a system with a different barostat, self._barostat_type will remain outdated. This might actually be a bug when this is called through set_system(..., fix_state=True) (if I understand correctly the code).
- We can have a thermodynamic state in NVT with thermo_state._barostat_type == MonteCarloBarostat instead of the safer None. I don't think this causes problems right now, but you never know what can happen in the future.

docs/releasehistory.rst

openmmtools/cache.py

andrrizzi · 2019-09-28T12:44:48Z

openmmtools/states.py

@@ -2032,6 +2105,11 @@ def volume(self):
        """The volume of the box (read-only)"""
        return _box_vectors_volume(self.box_vectors)

+    @property
+    def area(self):


What do you think about calling this xy_area or area_xy? I'd skip reading the docstring with this name.

update release history refactor area->area_xy # Conflicts: # docs/releasehistory.rst

Olllom · 2019-10-23T20:32:23Z

Hi Andrea,

thanks for your suggestions and sorry for the delay. I have incorporated all of your requests.

In order to allow compatibility between states with different surface tensions, I basically mimicked the pressure implementation. This should be the most consistent way to standardize the system, since the surface tension needs to be defined by the ThermodynamicState rather than the system barostat.

Instead of having a field for the barostat type, I added a private method _is_barostat_type_consistent. This should avoid the inconsistencies you mentioned above.

jchodera · 2019-10-23T21:21:34Z

Since this PR has added significant complexity, is there anything we could implement at the OpenMM API for these barostats that would significantly simplify this code? We're looking for features to add for the next OpenMM release, so if there is an easy way to extend the OpenMM API to facilitate this, please let us know!

andrrizzi · 2019-10-24T09:53:26Z

Thanks @Olllom ! I'll take a look at the changes before the end of the week. Now that we merged the multiple alchemical region features, I should be able to take a look at your other PR modifying the alchemical factory too next week.

andrrizzi

Looks great, thank you! I've only found a couple of minor typos or things that may have been forgotten in the code, but I think this is essentially ready to be merged. Nice job!

andrrizzi · 2019-10-26T15:06:50Z

openmmtools/states.py

-        if barostat is not None:
-            self._barostat_type = type(barostat)
+
+        # If surface tension is None, we try to infer the pressure from the barostat.


"infer the pressure" -> "infer the surface tension"

andrrizzi · 2019-10-26T15:11:29Z

openmmtools/states.py

+
+
+    def _set_barostat_surface_tension(self, barostat, gamma):
+        # working around a bug in the unit conversion https://github.com/openmm/openmm/issues/2406


Nice catch!

openmmtools/states.py

openmmtools/tests/test_states.py

Olllom · 2019-10-26T16:03:03Z

Great!
All fixed.

andrrizzi · 2019-10-28T10:36:04Z

Almost! It looks there's one of the new tests failing: https://travis-ci.org/choderalab/openmmtools/jobs/603266322#L3314

andrrizzi · 2019-10-28T10:40:34Z

There are also these other two errors in the "allowed failures" that I haven't seen before, but I'm not sure this is related to your changes so I'd address in a separate PR unless you know why that is happening: https://travis-ci.org/choderalab/openmmtools/jobs/603266325#L3185

andrrizzi · 2019-10-28T10:53:41Z

I confirm that the latter tests are unrelated as they appear also in a PR without these changes so there's just the surface tension test to fix.

jchodera · 2019-10-28T21:03:28Z

Is there anything we could implement at the OpenMM API for these barostats that would significantly simplify this code?

andrrizzi · 2019-10-29T09:48:51Z

Is there anything we could implement at the OpenMM API for these barostats that would significantly simplify this code?

Nothing particularly useful comes to mind. I think most of the complexity here comes from the fact that we're providing a unified API to a bunch of objects that have different parameters and (consequently) different APIs. We could create a general Barostat Python object in OpenMM that does these checks for us, but I think the total complexity (i.e., OpenMM + OpenMMTools) will be identical in the end.

Olllom · 2019-10-29T10:42:58Z

@jchodera Something like this old issue of yours would be useful to simplify the API: openmm/openmm#1440

In this way, OpenMM could provide a simplified API to set things like temperature, pressure, surface tension, but also step size, etc. But this would only simplify the present code to a certain degree. IMHO a lot of the complexity in the ThermodynamicState is due to the _standardized_system, from which the data stored explicitly in the ThermodynamicState (pressure, temperature, surface tension) is excluded. However, I do not see a good way around this without hurting performance.

On a side note, one rather boring feature that would be very useful for us is native OpenMM support for the van-der-Waals force switch (see #431). We have to use custom forces for the vdW in most of our simulations to get the correct lipid areas, which is quite a significant performance hit.

Olllom · 2019-10-29T11:19:51Z

@andrrizzi should be fixed now.

andrrizzi · 2019-10-30T14:53:25Z

Fantastic. Thanks again for the really nice work on tests too. Merging.

ThermodynamicState support for more barostats

b11fc6a

- supports MonteCarloAnistropicBarostat with same pressure along all axes - supports MonteCarloMembraneBarostat with zero surface tension

andrrizzi approved these changes Sep 24, 2019

View reviewed changes

Olllom commented Sep 26, 2019

View reviewed changes

Olllom added 4 commits September 26, 2019 16:00

added ThermodynamicState support for non-zero surface tension

37f4cf6

removed old comment

e75f212

enable platform properties in ThermodynamicState.create_context

a6f1f38

enable platform properties in ContextCache

b6a60d6

added platform properties to ContextCache serialization

03158ef

andrrizzi reviewed Sep 28, 2019

View reviewed changes

Olllom added 5 commits October 21, 2019 17:14

Merge branch 'master' into other_barostats

171d5b1

update release history refactor area->area_xy # Conflicts: # docs/releasehistory.rst

Merge branch 'master' into other_barostats

ba91110

update release history refactor area->area_xy # Conflicts: # docs/releasehistory.rst

ContextCache refactoring

e4b6b06

ThermodynamicState refactoring for surface tension

fb94572

ThermodynamicState._is_barostat_type_consistent

101915a

andrrizzi approved these changes Oct 26, 2019

View reviewed changes

final cleanup of other barostats in ThermodynamicState

d1c1812

attempted fix of surface tension test

46308b0

andrrizzi merged commit 6e07232 into choderalab:master Oct 30, 2019

This was referenced Oct 30, 2019

Barostat support in ThermodynamicState #436

Closed

Replica Exchange for Membrane Systems: Semi-Isotropic Pressure Coupling #324

Closed

Support for MonteCarloAnisotropicBarostat? #349

Open

Olllom deleted the other_barostats branch October 30, 2019 18:05

andrrizzi mentioned this pull request Dec 2, 2019

Make serialization of surface tension backwards compatible #455

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ThermodynamicState support for more barostats #437

ThermodynamicState support for more barostats #437

Olllom commented Sep 24, 2019 •

edited

Loading

andrrizzi left a comment •

edited

Loading

andrrizzi Sep 24, 2019

Olllom Sep 26, 2019

andrrizzi Sep 26, 2019

andrrizzi Sep 24, 2019 •

edited

Loading

Olllom Sep 26, 2019

andrrizzi commented Sep 24, 2019

Olllom left a comment

Olllom commented Sep 26, 2019

andrrizzi commented Sep 26, 2019

Olllom commented Sep 27, 2019

Olllom commented Sep 27, 2019

andrrizzi left a comment

andrrizzi Sep 28, 2019

Olllom Oct 22, 2019

Olllom commented Oct 23, 2019

jchodera commented Oct 23, 2019

andrrizzi commented Oct 24, 2019

andrrizzi left a comment

andrrizzi Oct 26, 2019

andrrizzi Oct 26, 2019

Olllom commented Oct 26, 2019

andrrizzi commented Oct 28, 2019

andrrizzi commented Oct 28, 2019

andrrizzi commented Oct 28, 2019

jchodera commented Oct 28, 2019

andrrizzi commented Oct 29, 2019

Olllom commented Oct 29, 2019 •

edited

Loading

Olllom commented Oct 29, 2019

andrrizzi commented Oct 30, 2019



		def _set_barostat_surface_tension(self, barostat, gamma):
		# working around a bug in the unit conversion https://github.com/openmm/openmm/issues/2406

ThermodynamicState support for more barostats #437

ThermodynamicState support for more barostats #437

Conversation

Olllom commented Sep 24, 2019 • edited Loading

Description

Todos

Status

andrrizzi left a comment • 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

andrrizzi Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrrizzi commented Sep 24, 2019

Olllom left a comment

Choose a reason for hiding this comment

Olllom commented Sep 26, 2019

andrrizzi commented Sep 26, 2019

Olllom commented Sep 27, 2019

Olllom commented Sep 27, 2019

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

Olllom commented Oct 23, 2019

jchodera commented Oct 23, 2019

andrrizzi commented Oct 24, 2019

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

Olllom commented Oct 26, 2019

andrrizzi commented Oct 28, 2019

andrrizzi commented Oct 28, 2019

andrrizzi commented Oct 28, 2019

jchodera commented Oct 28, 2019

andrrizzi commented Oct 29, 2019

Olllom commented Oct 29, 2019 • edited Loading

Olllom commented Oct 29, 2019

andrrizzi commented Oct 30, 2019

Olllom commented Sep 24, 2019 •

edited

Loading

andrrizzi left a comment •

edited

Loading

andrrizzi Sep 24, 2019 •

edited

Loading

Olllom commented Oct 29, 2019 •

edited

Loading