Skip to content

"Hooks" for ShootFromSnapshotsSimulation (i.e., committor) #755

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dwhswenson merged 23 commits into from
Jun 29, 2021
Merged

"Hooks" for ShootFromSnapshotsSimulation (i.e., committor) #755

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
710eccb
Start to path simulator hooks
dwhswenson Jan 29, 2018
64639b7
Tests for attaching hooks (generic)
dwhswenson Jan 29, 2018
c34bc67
Hooks-based approach seems to work in Committor
dwhswenson Jan 29, 2018
789784b
Add hook state for after_step hook
dwhswenson Jan 30, 2018
c4803d4
Add after_step hook_state to tests
dwhswenson Jan 30, 2018
83cf746
fix problem with hook state for hooks test sim
dwhswenson Feb 1, 2018
3112541
Merge remote-tracking branch 'upstream/master' into committor_hooks
dwhswenson May 5, 2018
fad83f6
Merge remote-tracking branch 'upstream/master' into committor_hooks
dwhswenson Sep 17, 2018
520abf2
Merge remote-tracking branch 'upstream/master' into committor_hooks
dwhswenson Apr 19, 2019
4a69542
move hooks to beta
dwhswenson Apr 19, 2019
c2597bb
Merge remote-tracking branch 'upstream/master' into committor_hooks
dwhswenson May 1, 2019
7b0690b
Merge remote-tracking branch 'upstream/master' into committor_hooks
dwhswenson Mar 21, 2020
6d1e134
tests for StorageHook
dwhswenson Mar 22, 2020
a2611f1
tests for ShootFromSnapshotsOutputHook
dwhswenson Mar 22, 2020
40a7694
attempt to clean up for Py2
dwhswenson Mar 22, 2020
5dcd421
Merge remote-tracking branch 'upstream/master' into committor_hooks
dwhswenson Dec 24, 2020
1d66070
Add docs on hooks
dwhswenson Jan 20, 2021
45a774e
Merge remote-tracking branch 'upstream/master' into committor_hooks
dwhswenson Jan 20, 2021
89473af
Add some other coverage ignore options
dwhswenson Jan 20, 2021
a9df664
Merge remote-tracking branch 'upstream/master' into committor_hooks
dwhswenson Mar 28, 2021
22461c6
Merge remote-tracking branch 'upstream/master' into committor_hooks
dwhswenson Jun 4, 2021
be22dd1
Merge remote-tracking branch 'upstream/master' into committor_hooks
dwhswenson Jun 18, 2021
d59b306
before_sim gets kwargs; after_sim gets hook_state
dwhswenson Jun 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add hook state for after_step hook 
This will enable analysis, even with parallel hooks
  • Loading branch information
@dwhswenson
dwhswenson committed Jan 30, 2018
commit 789784b574da9e54925dceb4e5f894ce7e7f4772
6 changes: 4 additions & 2 deletions openpathsampling/pathsimulators/hooks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ def before_simulation(self, sim):
def before_step(self, sim, step_number, step_info, state):
pass

def after_step(self, sim, step_number, step_info, state, results):
def after_step(self, sim, step_number, step_info, state, results,
hook_state):
pass

def after_simulation(self, sim):
Expand All @@ -47,7 +48,8 @@ def before_simulation(self, sim):
if self.frequency is None:
self.frequency = sim.save_frequency

def after_step(self, sim, step_number, step_info, state, results):
def after_step(self, sim, step_number, step_info, state, results,
hook_state):
if self.storage is not None:
self.storage.save(results)
if step_number % self.frequency == 0:
Expand Down
7 changes: 6 additions & 1 deletion openpathsampling/pathsimulators/path_simulator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,13 @@ def add_hook_method(hook_method, hook_name):

def run_hooks(self, hook_name, **kwargs):
"""Run the hooks for the given ``hook_name``"""
hook_name_state = {}

@sroet sroet Mar 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be good to (have the option) keep a constant state here, so change this into

Suggested change
hook_name_state = {}
hook_name_state = kwargs.get('hook_name_state') or {}

for hook in self.hooks[hook_name]:
hook(**kwargs)
result = hook(**kwargs)
if result is not None:
hook_name_state.update({hook: result})

@sroet sroet Mar 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should always update, None is a decent return as well

Suggested change
if result is not None:
hook_name_state.update({hook: result})
hook_name_state.update({hook: result})

if hook_name_state != {}:
return hook_name_state
Comment on lines +138 to +139

@sroet sroet Mar 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should always return, as each of the hook states should be stored with hook.func as key, so these should never update hook keys that shouldn't. (It also helps if you one hook state needs the result from another hook, such as before_step and after_step for step timings)

Suggested change
if hook_name_state != {}:
return hook_name_state
return hook_name_state


@abc.abstractmethod
def run(self, n_steps):
Expand Down
9 changes: 6 additions & 3 deletions openpathsampling/pathsimulators/shoot_snapshots.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ def run(self, n_per_snapshot, as_chain=False):
self.step = 0
snap_num = 0
n_snapshots = len(self.initial_snapshots)
hook_state = None
self.run_hooks('before_simulation', sim=self)

@sroet sroet Mar 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hook_states should be carried around

Suggested change
self.run_hooks('before_simulation', sim=self)
hook_state = self.run_hooks('before_simulation', sim=self, hook_name_state=hook_state)

for snapshot in self.initial_snapshots:
# before_snapshot
Expand Down Expand Up @@ -163,9 +164,11 @@ def run(self, n_per_snapshot, as_chain=False):
)
# shoot_snapshot_task (end)

self.run_hooks('after_step', sim=self,
step_number=step_number, step_info=step_info,
state=start_snap, results=mcstep)
hook_state = self.run_hooks(
'after_step', sim=self, step_number=step_number,
step_info=step_info, state=start_snap, results=mcstep,
hook_state=hook_state
)
Comment on lines +171 to +172

@sroet sroet Mar 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
hook_state=hook_state
)
hook_state=hook_state,
hook_name_state = hook_state
)


self.step += 1
# after_snapshot
Expand Down