Add spring shooting to OPS core by sroet · Pull Request #850 · openpathsampling/openpathsampling

sroet · 2019-08-06T15:55:07Z

This adds the spring shooting algorithm as described by Brotzakis and Bolhuis to the core of OPS. It has been developed previously on an E-CAM 2020 workshop.

dwhswenson

Overall, my main concerns are about things that seem to behave differently for spring shooting (especially around the selector) than for other shooting methods. If overridden functions have significantly different meanings/call signatures, maybe they're best given a different name? (e.g., ._biases). You can raise NotImplementedError if something is impossible to calculate for spring shooting.

From my own interests in playing with spring shooting, my biggest concern is that it looks to me like the .probability() method will return nonsense.

Also, I've been trying to think if there's another way around some of the cyclic dependency issues you'd had (IIRC, the issue was that you needed to share information between fwd and bkwd selectors, but then storage wouldn't see a DAG, and therefore couldn't recreate). Would something like this work?

class SpringShootingSelector(StorableNamedObject):
    def __init__(self, k_spring, delta_max):
        self.forward_selector = ForwardSpringShootingSelector(k_spring, delta_max)
        self.backward_selector = BackwardSpringShootingSelector(k_spring, delta_max)
        self.forward_selector._parent = self
        self.backward_selector._parent = self
        self.initial_trajectory = ...
        ...

class SpringShootingMover(...):
    def __init__(self, selector, ensemble):
        super().__init__(selector, ensemble)
        self.movers = [
            ForwardSpringShootingMover(selector.forward_selector, ensemble),
            BackwardSpringShootingMover(selector.backward_selector, ensemble),
        ]
        ...

(Obviously needing to fill in a lot of blanks.) I think that gets around the storage issue. Think of it as "recreate, then initialize," where the initialization is just telling where the parent is (where the joint storage of last selected snapshot is.) Note that if you do a custom from_dict for SpringShootingSelector, you'd need to do this in there.

In fact, that might even make it dangerously ease to (incorrectly) use spring shooting in combination with other path movers (by making the code more compatible, even if it wouldn't be scientifically compatible).

Another thing to consider would be to check the sanity of the input trajectory (to prevent foolishness by the user). The previous shooting snapshot must be a part of the input trajectory (and must have the right index; which side you count from depends on which direction the last shot was), right?

dwhswenson · 2019-08-11T07:51:45Z

+from functools import reduce
+
+
+class SpringShootingSelector(paths.ShootingPointSelector):


General concern on SpringShootingSelector: it looks to me like the inherited methods f and probability would give nonsense (raise an error for probability?) See:

openpathsampling/openpathsampling/shooting.py

Lines 12 to 29 in d2dab42

class ShootingPointSelector(StorableNamedObject):

def f(self, snapshot, trajectory):

"""

Returns the unnormalized proposal probability of a snapshot

Notes

-----

In principle this is an collectivevariable so we could easily add

caching if useful

"""

return 1.0

def probability(self, snapshot, trajectory):

sum_bias = self.sum_bias(trajectory)

if sum_bias > 0.0:

return self.f(snapshot, trajectory) / sum_bias

else:

return 0.0

Giving a correct answer for probability, in particular, would be very useful.

dwhswenson · 2019-08-11T07:51:53Z

+    spring shooting simulation. It uses a biased potential in the shape of
+    min(1, e^(-k*i)) for a forward shooting move and min(1, e^(k*i)) for a
+    backwards shooting move, where i is a frame number in the range
+    [-delta_max, delta_max] where 0 is the last accepted shooting frame index.


This could do with a slightly clearer explanation. Maybe something like `where i is in the range [-delta_max, delta_max] and represents a shift (in frames) relative to the last shooting frame index.

dwhswenson · 2019-08-11T07:52:07Z

+        else:
+            self.k_spring = k_spring
+
+        # Initiate the class variable


instance variables, not class variables!

dwhswenson · 2019-08-11T07:52:13Z

+            raise RuntimeError("Sum of the biases changed")
+
+    @staticmethod
+    def _biases(delta_max, k_spring):


this is a weird override of super's _biases method, see

openpathsampling/openpathsampling/shooting.py

Lines 36 to 41 in d2dab42

def _biases(self, trajectory):

"""

Returns a list of unnormalized proposal probabilities for all

snapshots in trajectory

"""

return [self.f(s, trajectory) for s in trajectory]

dwhswenson · 2019-08-12T11:41:56Z

@@ -0,0 +1,346 @@
+from nose.tools import (assert_equal, assert_false, raises, assert_is,
+                        assert_is_instance)


General comment on tests: we're moving toward pure pytest, so it might be good to rewrite this for pytest now (rather than later). That should be pretty straightfoward: replace assert_* with plain Python assert statements, and use with pytest.raises(Exception): blocks for the exception testing.

We support nose-based tests simply because we haven't gotten around to rewriting all our old tests, but that's on the to-do list (likely as a "good first issue" for someone just starting to get familiar with OPS).

dwhswenson · 2019-08-12T11:46:33Z

+from openpathsampling.pathmovers.spring_shooting import (SpringShootingSelector, SpringMover,
+                             ForwardSpringMover, BackwardSpringMover,
+                             SpringShootingMover, SpringShootingStrategy,
+                             SpringShootingMoveScheme)


clean up style on this import

…or' variable from SpringShootingStrategy

sroet · 2020-12-24T20:14:39Z

@dwhswenson This one should be ready for another round of comments

dwhswenson · 2021-01-20T08:59:41Z

Started on another round of review on this; won't finish it today. But one question: Something weird happens at step 124 in the spring shooting path tree in the example:

Any idea what's up with that? I think it only happens once in the whole tree, but it is strange.

sroet · 2021-01-20T16:37:22Z

Any idea what's up with that? I think it only happens once in the whole tree, but it is strange.

So I looked into it and it seems that happens when we select the "last" index as a shooting point (in my new test it was 38 of a trajectory of length 39), while trying to run forward. This results in a 0 md-step acceptance, which the tree does not like.

Now, I am not too sure what to do with this one (or index 0); we can reject it like other "illegal" indices (negative or ones that are outside of the trajectory) or keep it accepted. It shouldn't matter for the MC acceptance, however it might alter the efficiency of the "spring" (as it does update the last_accepted_index).

I will keep it as the only reference for this edge case mentioned in the paper is:

Reject the entire move if the index is outside of the current path.

And in the paper the path it is not to well defined, as it claims length L but indices [0, L], so I assume it is an off-by-one error and the paper actually means length L+1, start index 1, or final index L-1 and should include both end-points.

Do you have another view on what is the "correct" implementation? (as you are in the acknowledgement for `critically reading' that paper)

… step indices if we select an illegal frame

sroet · 2021-01-25T17:17:00Z

After a high bandwidth discussion between me and @dwhswenson, the algorithm is slightly altered .

It now does not allow for frames in the state to be selected anymore, and now uses these indices to ensure 0-md step shots for illegal shooting points (so it selects index 0 for a backwards shot and len(traj)-1 for a forward shot).

I also updated the tests to reflect this behaviour

codecov · 2021-01-25T17:17:22Z

Codecov Report

Merging #850 (bda94c2) into master (5ea30ac) will increase coverage by 0.33%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #850      +/-   ##
==========================================
+ Coverage   80.25%   80.58%   +0.33%     
==========================================
  Files         136      138       +2     
  Lines       14449    14671     +222     
==========================================
+ Hits        11596    11823     +227     
+ Misses       2853     2848       -5

Impacted Files	Coverage Δ
openpathsampling/pathmovers/__init__.py	`100.00% <100.00%> (ø)`
openpathsampling/pathmovers/move_schemes.py	`100.00% <100.00%> (ø)`
openpathsampling/pathmovers/spring_shooting.py	`100.00% <100.00%> (ø)`
openpathsampling/high_level/network.py	`85.50% <0.00%> (+0.07%)`	⬆️
openpathsampling/ensemble.py	`84.74% <0.00%> (+0.23%)`	⬆️
openpathsampling/numerics/histogram.py	`83.82% <0.00%> (+0.33%)`	⬆️
openpathsampling/high_level/transition.py	`41.19% <0.00%> (+0.66%)`	⬆️
openpathsampling/engines/dynamics_engine.py	`79.68% <0.00%> (+1.56%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ea30ac...bda94c2. Read the comment docs.

sroet · 2021-01-29T14:37:06Z

Reading through the papers that cite OPS following #967 , I noticed that there is one that uses spring shooting with OPS. @dwhswenson do you know if they used this code/E-CAM code or if they implemented their own? (Was just wondering, thought it was cool to see this/similar algorithm being used)

dwhswenson · 2021-01-29T15:03:29Z

@sroet I don't know, but I would assume it was your E-CAM code, since Christoph may have known about that. That work was part of a thesis that came out in 2019, so I doubt it used this PR.

EDIT: Thesis contains the line "In all cases, the TPS simulation was done using Spring Shooting module of OPS," so that's almost definitely your E-CAM module. https://is.muni.cz/th/ndj99/PhDThesis_janos.pdf

dwhswenson

Looks good. A few very minor changes.

I'm a little uncomfortable with the use of SpringShootingSelector.acceptable_snapshot as a way to transfer information between methods. However, I'm not sure how easy it would be to change that, so no need to (unless you see another approach).

Co-authored-by: David W.H. Swenson <dwhs@hyperblazer.net>

sroet · 2021-03-03T15:49:05Z

I'm a little uncomfortable with the use of SpringShootingSelector.acceptable_snapshot as a way to transfer information between methods.

Yeah, it is not ideal. Keep in mind that this is just a backstop to correctly count the rejections, if this is not done correctly this just turns into an identity move (as the trial-trajectory should always be identical to the input trajectory if an illegal frame is selected).

The only issue this might lead to (other than a misrepresentation of the number of rejected paths) is less efficient sampling due to that (illegal) point being picked as the anchor point for the next try. None of these outcomes should actually break detailed balance.

I can make it underscored, or name mangled (double underscored) to indicate that users should not mess with it, if that would help to make it more comfortable?

dwhswenson · 2021-03-03T15:57:34Z

I can make it underscored, or name mangled (double underscored) to indicate that users should not mess with it, if that would help to make it more comfortable?

Yeah, probably adding an underscore would help. How reusable is that class likely to be in other use cases? It might be possible to add methods that aren't part of the standard selector API that give a different return value. So spring_pick might return index and a bool as to whether the snapshot was acceptable.

In the regular shooting mover, the selector is a separate class because different selectors are interchangeable. That isn't true with spring shooting -- here there's a bit of separation of concerns, but it's doing more to just mimic the standard selectors for the purpose of familiarity.

Anyway, that's a bigger change that doesn't need to be done now. Just documenting the thought. For now, just add the underscore, please.

sroet · 2021-03-03T16:21:05Z

@dwhswenson thanks for the review, all your comments should be handled now :)

dwhswenson

LGTM. In the future, we might change the example to only show the first 50 or so moves in the path trees, but will merge this now!

add spring shooting to OPS core

e1cbe39

dwhswenson added the feature label Aug 6, 2019

dwhswenson requested changes Aug 12, 2019

View reviewed changes

update tests style and remove nosetest dependency

b980ff8

dwhswenson mentioned this pull request Oct 31, 2019

Accessing an old storage made by unknown OPS version #869

Closed

dwhswenson added this to the 1.5 milestone Dec 23, 2020

sroet added 5 commits December 24, 2020 16:44

Merge remote-tracking branch 'upstream/master' into spring_shooting

c726f7b

Test for identity of the recreated selector and remove unused 'select…

52bc8c7

…or' variable from SpringShootingStrategy

handle comments and update test for 100% coverage of added code

c3eb636

updated function doc strings

626ea48

rerun example in python3

1797157

sroet added 2 commits January 25, 2021 17:36

Merge remote-tracking branch 'upstream/master' into spring_shooting

5261487

exclude frames inside the state from being selected, also ensure 0 md…

d048df6

… step indices if we select an illegal frame

sroet requested a review from dwhswenson January 25, 2021 17:17

dwhswenson requested changes Mar 3, 2021

View reviewed changes

Comment thread openpathsampling/pathmovers/spring_shooting.py Outdated

Comment thread openpathsampling/pathmovers/spring_shooting.py Outdated

Comment thread openpathsampling/pathmovers/spring_shooting.py Outdated

Comment thread openpathsampling/tests/test_spring_shooting.py Outdated

Update openpathsampling/pathmovers/spring_shooting.py

fa1b474

Co-authored-by: David W.H. Swenson <dwhs@hyperblazer.net>

handle feedback from review of @dwhswenson

bda94c2

sroet requested a review from dwhswenson March 3, 2021 16:20

dwhswenson approved these changes Mar 3, 2021

View reviewed changes

dwhswenson merged commit a5f9ca0 into openpathsampling:master Mar 3, 2021

sroet deleted the spring_shooting branch March 3, 2021 17:16

dwhswenson mentioned this pull request Jul 5, 2021

OpenPathSampling 1.5 #1031

Merged

		from functools import reduce


		class SpringShootingSelector(paths.ShootingPointSelector):

	class ShootingPointSelector(StorableNamedObject):
	def f(self, snapshot, trajectory):
	"""
	Returns the unnormalized proposal probability of a snapshot

	Notes
	-----
	In principle this is an collectivevariable so we could easily add
	caching if useful
	"""
	return 1.0

	def probability(self, snapshot, trajectory):
	sum_bias = self.sum_bias(trajectory)
	if sum_bias > 0.0:
	return self.f(snapshot, trajectory) / sum_bias
	else:
	return 0.0

	def _biases(self, trajectory):
	"""
	Returns a list of unnormalized proposal probabilities for all
	snapshots in trajectory
	"""
	return [self.f(s, trajectory) for s in trajectory]

		@@ -0,0 +1,346 @@
		from nose.tools import (assert_equal, assert_false, raises, assert_is,
		assert_is_instance)

Uh oh!

Conversation

sroet commented Aug 6, 2019

Uh oh!

dwhswenson left a comment

Choose a reason for hiding this comment

Uh oh!

dwhswenson Aug 11, 2019

Choose a reason for hiding this comment

Uh oh!

dwhswenson Aug 11, 2019

Choose a reason for hiding this comment

Uh oh!

dwhswenson Aug 11, 2019

Choose a reason for hiding this comment

Uh oh!

dwhswenson Aug 11, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dwhswenson Aug 12, 2019

Choose a reason for hiding this comment

Uh oh!

dwhswenson Aug 12, 2019

Choose a reason for hiding this comment

Uh oh!

sroet commented Dec 24, 2020

Uh oh!

dwhswenson commented Jan 20, 2021

Uh oh!

sroet commented Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sroet commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sroet commented Jan 29, 2021

Uh oh!

dwhswenson commented Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwhswenson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sroet commented Mar 3, 2021

Uh oh!

dwhswenson commented Mar 3, 2021

Uh oh!

sroet commented Mar 3, 2021

Uh oh!

dwhswenson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sroet commented Jan 20, 2021 •

edited

Loading

sroet commented Jan 25, 2021 •

edited

Loading

codecov Bot commented Jan 25, 2021 •

edited

Loading

dwhswenson commented Jan 29, 2021 •

edited

Loading