Merge pull request #1076 from sroet/add_modified_snapshot_selector

Take care of coordinate modifcation in selector probability_ratio

openpathsampling/deprecations.py

@@ -168,6 +168,17 @@ def version_tuple_to_string(version_tuple):
     deprecated_in=(1, 5, 0)
 )
 
+NEW_SNAPSHOT_SELECTOR = Deprecation(
+    problem=("new_snapshot=None; If snapshot has been copied or modified we "
+             "can't find it in trial_trajectory. This call signature will "
+             "update to "
+             "(old_snapshot, old_trajectory, new_snapshot, new_trajectory) "
+             "in {OPS} {version}. "),
+    remedy=("Call with kwargs and use new_snapshot=old_snapshot if "
+            " old_snapshot is not copied or modified in new_traj"),
+    remove_version=(2, 0),
+    deprecated_in=(1, 6, 0)
+)
 
 # has_deprecations and deprecate hacks to change docstrings inspired by:
 # https://stackoverflow.com/a/47441572/4205735

openpathsampling/pathmovers/spring_shooting.py

@@ -3,6 +3,7 @@
 from openpathsampling.high_level.move_strategy import levels
 from openpathsampling.pathmover import SampleNaNError, SampleMaxLengthError
 import openpathsampling.high_level.move_strategy as move_strategy
+from openpathsampling.deprecations import NEW_SNAPSHOT_SELECTOR
 from functools import reduce
 
 
@@ -171,7 +172,7 @@ def sum_spring_bias(self, delta_max, k_spring):
             return sum(self._spring_biases(delta_max, k_spring))
 
     def probability_ratio(self, snapshot, initial_trajectory,
-                          trial_trajectory):
+                          trial_trajectory, new_snapshot=None):
         """
         Returns the acceptance probability of a trial trajectory, this is 1.0
         as long as a snapshot has been selected that is inside of the
@@ -192,6 +193,8 @@ def probability_ratio(self, snapshot, initial_trajectory,
                 1.0 if anacceptable snapshot has been chosen 0.0 otherwise
         """
         # Check if an acceptable snapshot was selected
+        if new_snapshot is None:
+            NEW_SNAPSHOT_SELECTOR.warn(stacklevel=3)
         if self._acceptable_snapshot:
             return 1.0
         else:

openpathsampling/shooting.py

@@ -1,10 +1,9 @@
 import math
 import logging
 
-import numpy as np
-
 from openpathsampling.netcdfplus import StorableNamedObject
 from openpathsampling import default_rng
+from openpathsampling.deprecations import NEW_SNAPSHOT_SELECTOR
 
 logger = logging.getLogger(__name__)
 init_log = logging.getLogger('openpathsampling.initialization')
@@ -34,9 +33,15 @@ def probability(self, snapshot, trajectory):
         else:
             return 0.0
 
-    def probability_ratio(self, snapshot, old_trajectory, new_trajectory):
+    def probability_ratio(self, snapshot, old_trajectory,
+                          new_trajectory, new_snapshot=None):
+        # TODO OPS 2.0: We should probabily alter the order and keyword names
+        # of this call
+        if new_snapshot is None:
+            NEW_SNAPSHOT_SELECTOR.warn(stacklevel=3)
+        new_snapshot = new_snapshot or snapshot
         p_old = self.probability(snapshot, old_trajectory)
-        p_new = self.probability(snapshot, new_trajectory)
+        p_new = self.probability(new_snapshot, new_trajectory)
         return p_new / p_old
 
     def _biases(self, trajectory):
@@ -171,7 +176,7 @@ def sum_bias(self, trajectory):
 
     def pick(self, trajectory):
         idx = self._rng.integers(self.pad_start,
-                                len(trajectory) - self.pad_end)
+                                 len(trajectory) - self.pad_end)
         return idx
 
 
@@ -229,8 +234,13 @@ def pick(self, trajectory):
     def probability(self, snapshot, trajectory):  # pragma: no cover
         return 1.0  # there's only one choice
 
-    def probability_ratio(self, snapshot, old_trajectory, new_trajectory):
+    def probability_ratio(self, snapshot, old_trajectory,
+                          new_trajectory, new_snapshot=None):
+        # TODO OPS 2.0: alter the order + keywords in the call
         # must be matched by a final-frame selector somewhere
+
+        if new_snapshot is None:
+            NEW_SNAPSHOT_SELECTOR.warn(stacklevel=3)
         return 1.0
 
 
@@ -253,6 +263,11 @@ def pick(self, trajectory):
     def probability(self, snapshot, trajectory):  # pragma: no cover
         return 1.0  # there's only one choice
 
-    def probability_ratio(self, snapshot, old_trajectory, new_trajectory):
+    def probability_ratio(self, snapshot, old_trajectory,
+                          new_trajectory, new_snapshot=None):
+        # TODO OPS 2.0: alter the order + keywords in the call
         # must be matched by a first-frame selector somewhere
+
+        if new_snapshot is None:
+            NEW_SNAPSHOT_SELECTOR.warn(stacklevel=3)
         return 1.0

openpathsampling/tests/test_shooting.py

@@ -83,6 +83,7 @@ def test_probability(self, frame):
         expected = pytest.approx(self.f[frame] / norm)
         assert self.sel.probability(traj[frame], traj) == expected
 
+
 class TestBiasedSelector(SelectorTest):
     def setup(self):
         super(TestBiasedSelector, self).setup()
@@ -125,6 +126,7 @@ def test_probability(self, func, frame):
         expected = pytest.approx(f[frame] / norm)
         assert sel.probability(traj[frame], traj) == expected
 
+
 class TestFirstFrameSelector(SelectorTest):
     def test_pick(self):
         sel = FirstFrameSelector()
@@ -214,3 +216,19 @@ def test_f(self):
         for idx1, frame in enumerate(mytraj):
             if (idx1 != expected_idx):
                 assert self.sel.f(frame, mytraj) == 0.0
+
+    @pytest.mark.parametrize("new_coord,expected", [(0.11, 1.0), (-0.09, 0.0)])
+    def test_probability_ratio_modified_coordinates(self, new_coord, expected):
+        # Test if probability ratio still works when coordinates are modified
+        mytraj = make_1d_traj(coordinates=[-0.5, -0.4, -0.3, -0.1,
+                                           0.1, 0.2, 0.3, 0.5])
+        idx = self.sel.pick(mytraj)
+        frame = mytraj[idx]
+        # Alter 0.1 to 0.11 and replace the original snapshot with the modded
+        # one
+        mod_frame = frame.copy_with_replacement(coordinates=[[new_coord]])
+        mod_traj = mytraj[:idx]
+        mod_traj += paths.Trajectory([mod_frame])
+        mod_traj += mytraj[idx+1:]
+        prob = self.sel.probability_ratio(frame, mytraj, mod_traj, mod_frame)
+        assert prob == expected