You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
@jhprinz and I discussed the fact that analysis is a bit confusing because some things are studied using the SampleSet and some are studied using the PathMoveChange. Furthermore, it sounds like some of his changes to speed up loading of PathMoveChanges may make it harder to loop over them in a one-per-MC-step way.
We realized that we can solve all of this by saving an MCStep object which has attributes for both the newly-determined SampleSet and the most recent PathMoveChange (from the top of the tree). This should make analysis a good bit easier from the user's point of view.
Related to this: if we have such an object, it might be possible to create something like slices for storage based on subsets of this. A couple examples where I would want something like that:
while developing analysis methods, it can be nice to work on a small subset of the data for speed purposes
block averaging can be easily implemented by using slices
If each step contains both the SampleSet and PathMoveChange, slicing the storage would just be slicing the steps list.
Alright. I have this implemented in #251. The current MCStep has the following attributes
class MCStep(OPSObject):
"""
A monte-carlo step in the main PathSimulation loop
It references all objects created and used in a MC step:
The used mover, and simulator as well as the initial and final sampleset,
the step number and the generated pathmovechange.
Attributes
----------
simulation : PathSimulation
the running pathsimulation responsible for generating the step
step : int
the step number counting from the root sampleset
pre : SampleSet
the initial (pre) sampleset
post : SampleSet
the final (post) sampleset
change : PathMoveChange
the pathmovechange describing the transition from pre to post
root : SampleSet
the first sampleset at step 0.
"""
Questions:
Is the naming good?
Do we need both .pre and .post? I thought this makes it very clear that this is a step which goes from a pre-state to a post-state and not a state itself.
Do we need the root? This is somehow redundant and in most cases the same as steps[0].pre
Do we want an additional reference to the used mover? This is accessible by step.change.mover. We could add this as a property. Actually I think I just do that.
@dwhswenson suggested to allow to splice a store. Do you mean just internally and do something like:
which would just generate a fake storage that only contains every 10th step. Or really create a new storage as a reduced copy that only contains what is needed.
Third option is to just add a slicing in the analysis to not use all data.
step to step_number (or even just number) -- otherwise, common usage might look weird:
forstepinsteps:
step_number=step.step
(or worse yet, a user might be tempted to make that line step = step.step, and then spend a while wondering why the analysis crashes so hard)
I would also suggest a different naming on pre and post. You've gone with a very symmetric treatment, but I don't think people will think of them symmetrically. Usually, you just want the new one (post). Keeping the previous around mainly serves as a sanity check, right? And a way to store the "-1" (root) step?
I'd change post to current or active (possibly followed by _state or _samples), and pre to previous (with the same suffix, if you add one).
change, simulation, and root all make sense to me (although if you suffix the above, root should also take that suffix).
Do we need both .pre and .post?
There are two reasonable ways to think about what the record of a simulation (which the list of MCSteps is) fundamentally represents:
it is a series of $N$ states, with metadata about how you got to state $n$ from state $n-1$
it is a series of $N-1$ changes, with convenience methods to obtain the state at either end
They are, of course, equivalent; the question is which should our code reflect. You seem to be going toward the latter. I think that is the less intuitive approach and will seem unfamiliar to novices, so we should make things look more like the former.
Keeping pre and post, but with less symmetric names, is, I think, a decent compromise of the two.
Do we need the root?
I don't know. The answer to that question should be the same as the answer to this one: Is there ever a reason within the analysis of a specific step that we would need to know what the root is?
I can't think of a case for that, but that doesn't mean it doesn't exist.
Note also that if you're doing block analysis, you basically split your steps into "independent" simulations. In that analysis context, the root for block 5 should have changed from what it was in the global analysis. So this makes me lean again keeping the root property.
Do we want an additional reference to the used mover?
I like the property idea. I assume you mean here that you'll return the canonical_mover from #243?
Splicing storage
I'm not entirely sure what implementation I'd like to see here, but let me explain the features. Also, some of the implementation changes might (probably will) need to happen in the analysis functions.
This is all about making analysis functions reusable. I can immediately think of 3 contexts where it might be nice to use the same analysis function on different data:
global analysis on all of the data in storage
subset analysis on some group of steps, e.g., as would be used to obtain error estimates by block averaging or resampling
per-replica analysis, as with Gelman-Rubin diagnostics
Originally, I wanted to have the analysis functions take storage as their argument, because that way I could include storage.*.cache_all() in the analysis function and therefore not require the user to call it manually. Then, a sufficiently nice slicing behavior that makes the sliced storage look like a normal storage, but providing a different list of steps, would be a simple way to solve subset analysis.
But I still don't see a simple solution to per-replica analysis. This requires some sort of filtering of the SampleSets in the step.post.
Perhaps the best approach is to change the analysis functions to take a list/generator of MCSteps, and to apply some sort of extra step to filter the selected replica ID for the per-replica analysis. This also has the downside that storage.*.cache_all() needs to go into functions that the user must call. But we can combine all of them in storage.run_this_first_for_faster_analysis() (well, maybe not that name...)
Actually, regarding storage.*.cache_all(): is there any reason not to make that a part of the loading of the storage file? At the end of storage.__init__ just do
Maybe add another flag in initialization like cache_on_load=True and check that in the if statement as well? You could also create a different mode, like "r-nocache" which you catch before sending mode "r" to netCDF. But I think caching by default when opening read-only will be the most user-friendly behavior.
You might also, at each cache step, output something (at least to log, possibly to screen) saying "Loading N changes", "Loading N samples", etc, just so the user knows that the code is running correctly and why it is taking a couple seconds.
In that case, the analysis functions should work on a list of MCSteps instead of Storage objects, and this removes the need for slicing storage: subset analysis works by explicitly creating a subset of storage.mcsteps, and per-replica analysis requires creating a filter that takes a list of MCSteps and returns a list of MCSteps with a different (filtered) SampleSet; that new list is then passed into the analysis routines.
I think this solves all my concerns. So, to summarize:
don't worry about slicing storage
cache_all() stuff should be done by default as part of loading the file for reading (with an option to not do that if the user desires)
analysis routines should all be based on lists of MCSteps instead of storage. This may break a few things around automatically checking whether we're using the same storage as previously; I think I want to readjust that paradigm though (if, for some reason, you're analyzing the same transition object using a different storage, after analyzing it with a previous storage, then you're doing something advanced enough that you should know how to use the force=True parameter)
it is a series of $N$ states, with metadata about how you got to state $n$ from state $n-1$
it is a series of $N-1$ changes, with convenience methods to obtain the state at either end
Yes, that was my question. The name step seemed to hint to the latter, that is why I used it and had a function to get the previous state and final state. If you say active or current is the post, then this solves another question on which we should use. It also means that if we want to use all states we have to add the initial one extra or we add a step that goes from nothing to the first state.
We should add that to the pathSimulation as the initial step.
The root would be useful as a reference . Since a simulation does the same mover several times we would have
Not sure if that is necessary, but it could be nice to have this additional information.
The additional initial mcstep makes this a little redundant since we can just go back step_number steps to get the initial one. Maybe I remove this for now. We can add it again, if we really have a use for it.
The .mover property would just return mcstep.change.mover and would this return the root_mover of the simulation. Canonical needs to be applied in addition mcstep.change.mover.canonical
Auto-Caching
Good idea. I had that in the code at some point and removed it because of memory concerns. I think it should be default to cache these with an option to turn it off.
Activity
dwhswenson commentedon May 13, 2015
Related to this: if we have such an object, it might be possible to create something like
slices forstoragebased on subsets of this. A couple examples where I would want something like that:If each
stepcontains both theSampleSetandPathMoveChange, slicing thestoragewould just be slicing thestepslist.jhprinz commentedon May 15, 2015
Will be part of #251.
jhprinz commentedon May 17, 2015
Alright. I have this implemented in #251. The current MCStep has the following attributes
Questions:
.preand.post? I thought this makes it very clear that this is a step which goes from a pre-state to a post-state and not a state itself.steps[0].prestep.change.mover. We could add this as a property. Actually I think I just do that.@dwhswenson suggested to allow to splice a store. Do you mean just internally and do something like:
which would just generate a fake storage that only contains every 10th step. Or really create a new storage as a reduced copy that only contains what is needed.
Third option is to just add a slicing in the analysis to not use all data.
dwhswenson commentedon May 17, 2015
We might make things a bit more explicit.
steptostep_number(or even justnumber) -- otherwise, common usage might look weird:(or worse yet, a user might be tempted to make that line
step = step.step, and then spend a while wondering why the analysis crashes so hard)I would also suggest a different naming on
preandpost. You've gone with a very symmetric treatment, but I don't think people will think of them symmetrically. Usually, you just want the new one (post). Keeping the previous around mainly serves as a sanity check, right? And a way to store the "-1" (root) step?I'd change
posttocurrentoractive(possibly followed by_stateor_samples), andpretoprevious(with the same suffix, if you add one).change,simulation, androotall make sense to me (although if you suffix the above,rootshould also take that suffix).There are two reasonable ways to think about what the record of a simulation (which the list of
MCSteps is) fundamentally represents:They are, of course, equivalent; the question is which should our code reflect. You seem to be going toward the latter. I think that is the less intuitive approach and will seem unfamiliar to novices, so we should make things look more like the former.
Keeping
preandpost, but with less symmetric names, is, I think, a decent compromise of the two.I don't know. The answer to that question should be the same as the answer to this one: Is there ever a reason within the analysis of a specific step that we would need to know what the root is?
I can't think of a case for that, but that doesn't mean it doesn't exist.
Note also that if you're doing block analysis, you basically split your steps into "independent" simulations. In that analysis context, the root for block 5 should have changed from what it was in the global analysis. So this makes me lean again keeping the root property.
I like the property idea. I assume you mean here that you'll return the
canonical_moverfrom #243?Splicing storage
I'm not entirely sure what implementation I'd like to see here, but let me explain the features. Also, some of the implementation changes might (probably will) need to happen in the analysis functions.
This is all about making analysis functions reusable. I can immediately think of 3 contexts where it might be nice to use the same analysis function on different data:
storageOriginally, I wanted to have the analysis functions take
storageas their argument, because that way I could includestorage.*.cache_all()in the analysis function and therefore not require the user to call it manually. Then, a sufficiently nice slicing behavior that makes the sliced storage look like a normal storage, but providing a different list of steps, would be a simple way to solve subset analysis.But I still don't see a simple solution to per-replica analysis. This requires some sort of filtering of the
SampleSets in thestep.post.Perhaps the best approach is to change the analysis functions to take a list/generator of
MCSteps, and to apply some sort of extra step to filter the selected replica ID for the per-replica analysis. This also has the downside thatstorage.*.cache_all()needs to go into functions that the user must call. But we can combine all of them instorage.run_this_first_for_faster_analysis()(well, maybe not that name...)dwhswenson commentedon May 17, 2015
Actually, regarding
storage.*.cache_all(): is there any reason not to make that a part of the loading of the storage file? At the end ofstorage.__init__just doMaybe add another flag in initialization like
cache_on_load=Trueand check that in the if statement as well? You could also create a different mode, like "r-nocache" which you catch before sending mode "r" to netCDF. But I think caching by default when opening read-only will be the most user-friendly behavior.You might also, at each cache step, output something (at least to log, possibly to screen) saying "Loading N changes", "Loading N samples", etc, just so the user knows that the code is running correctly and why it is taking a couple seconds.
In that case, the analysis functions should work on a list of
MCSteps instead ofStorageobjects, and this removes the need for slicing storage: subset analysis works by explicitly creating a subset ofstorage.mcsteps, and per-replica analysis requires creating a filter that takes a list ofMCSteps and returns a list ofMCSteps with a different (filtered)SampleSet; that new list is then passed into the analysis routines.I think this solves all my concerns. So, to summarize:
cache_all()stuff should be done by default as part of loading the file for reading (with an option to not do that if the user desires)MCSteps instead ofstorage. This may break a few things around automatically checking whether we're using the same storage as previously; I think I want to readjust that paradigm though (if, for some reason, you're analyzing the same transition object using a different storage, after analyzing it with a previous storage, then you're doing something advanced enough that you should know how to use theforce=Trueparameter)jhprinz commentedon May 17, 2015
Few comments:
Yes, that was my question. The name
stepseemed to hint to the latter, that is why I used it and had a function to get the previous state and final state. If you say active or current is the post, then this solves another question on which we should use. It also means that if we want to use all states we have to add the initial one extra or we add a step that goes from nothing to the first state.We should add that to the pathSimulation as the initial step.
The root would be useful as a reference . Since a simulation does the same mover several times we would have
Not sure if that is necessary, but it could be nice to have this additional information.
The additional initial mcstep makes this a little redundant since we can just go back step_number steps to get the initial one. Maybe I remove this for now. We can add it again, if we really have a use for it.
The
.moverproperty would just returnmcstep.change.moverand would this return the root_mover of the simulation. Canonical needs to be applied in additionmcstep.change.mover.canonicalAuto-Caching
Good idea. I had that in the code at some point and removed it because of memory concerns. I think it should be default to cache these with an option to turn it off.
I will also add fast load/save/cache for mcstep.