Great to see this! Sorry that I'm not going to be very responsive in the next week or two. Somehow I'm significantly busier on lockdown than at normal times.

The current test failure is a known issue with the most recent conda-forge release of svgwrite. It should be fixed very soon -- in principle it is fixed; needs me to reply to someone though (one of the many things on my list).

Quick comments on your questions/todos:

1. [RE: output_stream, save_frequency] I would expect to be able to set these attributes on the PathSimulator to influence the behaviour of the currently attached hooks.

This is a good point. I'm being quick here, so I'm not even checking the code, but what should probably happen is to treat the PathSimulator attributes as a default fallback: (1) By default, hooks don't set this; instead they get it from the PathSimulator (every hook knows its simulator, right?). This means if the user specifically changes it in simulator and never set it in hooks, it behaves as expected. (2) If a user sets it in the hook and in the simulator, I think the hook takes precedence.

What should not happen is, hook is added, implicitly takes the simulator's value. Then user changes simulator's value, and it has no effect. This would be a bug.

2. I moved the LiveVisualization to after step.

This seems fine; but is there also a way to ensure that the 0th step is visualized? (Maybe a before_step hook and also has some internal state so it only gets run once?) I'm not sure the current behavior, but it would make sense to show the initial conditions if you're running a simulation that takes a while to do the first step.

The caveat on refreshing output is completely fine. There's no way around that, so as long as the docstrings are clear on the topic, all good.

3. The step_info passed to the after_step hooks from the PathSampling is specifically tailored for the PathSamplingOutputHook, i.e. it only takes the variables the OutputHook needs (current step, total steps, total elapsed time)

Good question. As I recall, the idea was for step_info to carry all metadata that might possibly be of interest to a hook. For committors, I believe there were 4 things: snapshot number, total number of snapshots, step number (inner loop), and total number of steps per inner loop.

Is the time info actually used by anything other than the output hook? You can probably move the time info into the hook -- get initial time at before_simulation, then update at each stage.

No problem.

Concerning the points:

1. Atm it is exactly as you said, if it is set in the hook the hook takes precedence otherwise the hook takes the defaults from the PathSimulator. The only situation in which I see potential trouble is the following:
   First user adds implicit default hooks (without value). Then user runs a round of sampling (i.e. before_simulation is executed and the hooks get their implicit defaults from the PathSimulator). If the user now changes the attributes on the PathSimulator and runs another round of sampling the hooks will not update, because they already got their (default) values when before_simulation was ran the first time.
   In addition to making these attributes properties or removing them from the PathSimulator (and moving them to the hooks altogether), I think another solution would be to add a state-flag to the hook: If hook is initialized with values it should always keep them. If it was initalized without it should always take the values from the associated PathSimulator when before_simulation is run. What do you think?

2. Previously the first step was also not visualized. Currently the StepVisualizer2D needs/uses the MC change to visualize. It was just that the visualization for step n was ran at the start of the next iteration, i.e. directly before step (n+1). But I will look into how/if it is possible to visualize the first step too.

3. No, at the moment the timing info is not used anywhere else. I could move the timing to the hook, but the PathSamplingOutputHook still needs the total number of snapshots and the current number to be able to calculate the progress string, this would then be similar to the committors, where we also have total iterations and loop counter.
   Something else where the time info could be useful is something I have been thinking about: Maybe it would be useful to write a "gracious kill" hook to make ops work nicer with the time limits on queuing systems. This would be hook that knows the maximum walltime and simply checks every step if we expect to finish the next step given the time the previous steps took on average. If not it would sync the storage and exit the simulation loop.

First off, the error you have now should be fixed as soon as you merge in master. It's a problem with the miniconda install (they moves the web page we use for checksums).

Point-by-point:

Atm it is exactly as you said, if it is set in the hook the hook takes precedence otherwise the hook takes the defaults from the PathSimulator.

Let me sketch out in code what I meant, which I think will address the issue. Let's pretend that the simulator has a property called frequency. The hook should do something like:

class MyHook(PathSimulatorHook):
    def __init__(self, frequency=None):
        self._simulation = None
        self._frequency = frequency

    def before_simulation(self, sim):
        self._simulation = sim

    @property
    def frequency(self):
        if self._freq_val is not None:
            return self._frequency
        elif self._simulation is not None:
            return self._simulation.frequency
        else:
            raise RuntimeError("`frequency` has not been set")  # maybe return None?

That isn't how I initially implemented it, but I think it (mostly) makes more sense. It solves the problem you're thinking of, right? If you ever set it in the hook, then the hook's value is used (even if you set it in the simulator later). If you haven't set it in the hook, the simulation's value is used.

The only question is what to return when frequency isn't given on initialization and if the simulation is still not known (hasn't started yet). I'm leaning toward an error (probably best to actually subclass RuntimeError with a specialized error case), but returning None is also a reasonable option.

I will look into how/if it is possible to visualize the first step too.

If the first step can be visualized, great, but no big deal if it can't easily be done.

I could move the timing to the hook, but the PathSamplingOutputHook still needs the total number of snapshots and the current number to be able to calculate the progress string, this would then be similar to the committors, where we also have total iterations and loop counter.

I'd say move timing into the hook. For any other hook that needs it, I'm not worried about the overhead of calling time.time() once per MC step.

Maybe it would be useful to write a "gracious kill" hook to make ops work nicer with the time limits on queuing systems.

I think this sounds like a very good idea. In the long run, I want to have mid-trajectory checkpointing, because there are cases where a single trajectory might be as long as your allowed wall time. However, this would be a very useful improvement in the meantime.

If you implement this, please do a separate PR for it. It deserves a separate mention in the release notes.

Yes, that solves the problem I had in mind. I went ahead and implemented it for all hooks.

I moved the timing to the PathSamplingOutputHook, which has the added benefit that the test now also test the actual timing (and not only that we are able to print the correct string given the numbers).

Codecov Report

Merging #911 (6664456) into master (b6da5dd) will increase coverage by 0.07%.
The diff coverage is 85.16%.

@@            Coverage Diff             @@
##           master     #911      +/-   ##
==========================================
+ Coverage   81.40%   81.47%   +0.07%     
==========================================
  Files         140      140              
  Lines       15237    15362     +125     
==========================================
+ Hits        12403    12516     +113     
- Misses       2834     2846      +12     

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 b6da5dd...6664456. Read the comment docs.

@dwhswenson I would say this is ready for review. I cleaned up a bit and added the run_until_n_accepted method you suggested.

@dwhswenson Not morning anymore, but the branch is rebased on the current master and updated. But it looks like I will have to write some more test tomorrow... :)

@dwhswenson All done (I think).

• I moved the sample_set.sanity_check() to its own hook (which is a default hook for PathSampling) + wrote tests for that hook
• LiveVisualizerHook is not default anymore, instead it is attached only upon setting the PathSampling.live_visualizer property to something different then None. I now also throw an error from the hook if the live_visualizer is not set, which should be ok since the hook is not attached by default anymore. + wrote tests for that hook
• I fixed/improved the tests for the PathSamplingOutputHook according to your suggestions. Thanks for pointing me to the patch function, that solved the knot I had in my brain :)
• After sleeping a night on the StorageHook.after_simulation() method I think what we want is this:

if self.storage is not None:
    self.storage.sync_all()

This should work in all cases, namely if the storage set only in the hook and also if the storage is taken from the simulation (because then it will be at hook.storage if the hook does not have a storage).

So I just realized that I did not address the save try/except clause. I would/will do it together with the StorageHook.after_step() if what I did there is not what we want @dwhswenson : Let me know what you think is best there.

Looks good, as soon as the try/except clause for SimStore vs netcdfplus saving is added!

• After sleeping a night on the StorageHook.after_simulation() method I think what we want is this:

if self.storage is not None:
    self.storage.sync_all()

I agree. I'm not sure why there was a thing to avoid doing a second sync_all. I'm not entirely sure what all happens in netcdfplus, but in SimStore, that just tries to save all objects in an empty list (so there's no real computational cost, and no side effect.) I would think that a second sync_all would also be idempotent in netcdf/netcdfplus.

Looks good, as soon as the try/except clause for SimStore vs netcdfplus saving is added!

Done.

I agree. I'm not sure why there was a thing to avoid doing a second sync_all. I'm not entirely sure what all happens in netcdfplus, but in SimStore, that just tries to save all objects in an empty list (so there's no real computational cost, and no side effect.) I would think that a second sync_all would also be idempotent in netcdf/netcdfplus.

That was me beeing confused from what we previously had

if self.storage is not None:
    sim.storage.sync_all()

which did not make sense to me (checking self.storage but using sim.storage)...and the tests call assert_called_once which I did not want to touch :)

which did not make sense to me (checking self.storage but using sim.storage)...and the tests call assert_called_once which I did not want to touch :)

Ah, I understand. The self vs sim was probably just a slip of the fingers (or a missed change in a refactor).assert_called_once was probably because there didn't seem to be a way for it to be called more than once, but the important thing was that it should have been called at least once!

Anyway, all looks good now! Merging -- thanks again!