Skip to content

PathMoveChange improvements - #251

Merged
dwhswenson merged 110 commits into
openpathsampling:masterfrom
jhprinz:pmc
May 28, 2015
Merged

dwhswenson merged 110 commits into
openpathsampling:masterfrom
jhprinz:pmc

Conversation

@jhprinz

@jhprinz jhprinz commented May 10, 2015

Copy link
Copy Markdown
Contributor

See #246 for detailed discussion and ideas.

  • refactor sample generating movers into Generators
  • allow generators to be used to generate samples without SampleSets
  • remove .accepted from samples
  • .valid to property
  • move treelogic to separate file
  • fix tests to work with changes
  • add new PMC_store for faster loading and saving : Closes Fast loading for MovePathChanges? #237
  • add .trials, .results to PMC
  • add .in_ensembles and out_ensembles computation
  • fix visualization
  • better __contains__ for trees
  • add .canonical property : Closes Add canonical_move function to PathMoveChange #243
  • remove ForceEnsembleChange from MinusMover
  • add MCStep : Closes New MCStep object #244
  • add bias for EnsembleHop : Closes Add support for bias in EnsembleHopMover #142
  • move PathMover.engine to ShootingMover.engine : Closes Remove global PathMover.engine #150
  • fix analysis
  • merge MSTIS
  • little renames in MCStep
  • extreme speed-up for analysis
  • make canonical a property
  • cache_all for orderparameters
  • add StateSwapMover
  • fast loading of MCStep
  • check docstrings

I kept the old names .trials for all changes and .results for the actual final changes.

Will move some minor cleanup to a separate PR:

  • add simpler ensembles=[...] parameter for PathMovers : Means to use a more unified way to express the ensembles attribute for PathMovers.
  • cleanup for ensemble selection. This is related to better EnsembleHop support with bias. This might require little more discussion
  • change tree description to match networkx use of dicts of dicts of dicts (optional). This is not necessary right now and might take longer than I thought
  • add specific unit tests for treelogic, chaindict, etc...

@dwhswenson

Copy link
Copy Markdown
Member

Basically, I don't think there's any need for what you're calling a [...]Mover above.

@jhprinz

jhprinz commented May 22, 2015

Copy link
Copy Markdown
Contributor Author

Detailed Balance

I need some more help for clarification here to make sure that we talk about the same thing.

Detailed balance refers to

  1. making sure that when moving between ensembles we keep detailed balance (RepEx, Hop, Extend, Truncate, etc...)
  2. making sure that when moving inside an ensemble we keep detailed balance (Shooting, Reversal)

Using the input_ensemble and output_ensemble we could (not really easy, but possible) scan the whole mover tree and compute a relative proposal probability for all possible moves and between which ensembles they move. If that matrix is symmetrizable we know that we have a stationary distribution meaning we know what portion of the time the replica is in which ensemble. This requires some more logic in each mover.

In this matrix of change probabilities of moving in or between ensembles are still lots of MoverDependent Probabilities, but these are Mover Specific and should in most cases cancel (see your example #2). So that could really work. That could be future. :)

Btw. How do we implement SR when we cannot remove samples from samplesets? Do we still need the ForceEnsembleChangeMover? Or do we add the Sample(trajectory=None) as a delete Sample. There is of course the possibility to let the EnsembleHopMove to keep the replicaID then it will overwrite the current sample. But I think we need to check the current implementation for that.

@jhprinz

jhprinz commented May 22, 2015

Copy link
Copy Markdown
Contributor Author

I think this can be much easier than that. The picture I've had is that there is no default acceptor per replica exchange or per shooting or anything like that.

If you want a default acceptor, that happens at the move_scheme level. There only needs to be as shown by the circles at the top of my pictures in this thread (see example 2, in particular). If you're doing a ConditionalSequential, then each of its submoves needs an acceptor, which is why I thought it made sense to put a default in there, as well.

Interesting, that could work. That solves the problem of default acceptance. Still, having to add an acceptance mover only for Sequential-like movers seems unintuitive to me. In QM thinking. I want a sequence of steps to depend on measurements. Then I would think that I require the results of measurements and not the experiments and do the measurements myself. Hmm, I am still not sure which way is better.

Another way would be to specify the acceptance when calling the mover:

change = root_mover.move(globalstate, acceptor=Metropolis)

the acceptor is given to all sub-movers and used. This would also make sure that you cannot mix different acceptors (would that even make sense?). I know that mixing of different acceptance schemes can introduce a bias, there is something called "Parrondo's paradox".

This way we would also not have to store the acceptor in the PathMover. Only the changes would know about the used acceptor of course.

@dwhswenson

Copy link
Copy Markdown
Member

Detailed balance refers to

  1. making sure that when moving between ensembles we keep detailed balance (RepEx, Hop, Extend, Truncate, etc...)
  2. making sure that when moving inside an ensemble we keep detailed balance (Shooting, Reversal)

Overall, detailed balance refers to making sure that the state of the Markov chain satisfies microscopic reversibility. Our "state" is the set of all active samples.

If a move only involves one ensemble, then we have to make sure that it works at a per-ensemble level (shooting, path reversal).

If it involves more than one ensemble, then we should make sure that it works on a sampleset level. RepEx does this easily; some compound moves like Minus do it by combining asymmetric moves (hop, extend), each of which, independently, does NOT satisfy detailed balance. But combined, they do.

Single replica hops should satisfy detailed balance, but they only will if there are other hops which are part of the reverse process. (Similarly, extend and truncate can make a pair of moves; each of which alone does not satisfy detailed balance, but an appropriate pair does).

Using the input_ensemble and output_ensemble we could (not really easy, but possible) scan the whole mover tree and compute a relative proposal probability for all possible moves and between which ensembles they move.

Not always -- only if the user behaves well when designing the move scheme. Even if you have a forward and backward hop, they might be submoves of another move, in which case they aren't actually the "reverse" of the other move. Look at the single replica example I drew, but pretend that instead of the SHS mover in B, we just had a B->A hop. It would be impossible to make this satisfy detailed balance, because it would be impossible (or at least immeasurably unlikely) have a move starting in B create the same trial that had been made by a move starting in A. (You could only satisfy detailed balance if you accounted for the condition that the SHS rejects every move that gives a new trajectory -- in other words, if the shooting has any net effect.)

Btw. How do we implement SR when we cannot remove samples from samplesets?

We replace in the sampleset based on replica ID, not the ensemble. Replica ID never changes in SR. Only the ensemble does.

(BTW, your StateSwap is not the Single Replica State Swap, which is what we normally mean by State Swap.)

Still, having to add an acceptance mover only for Sequential-like movers seems unintuitive to me.

This is why I was thinking of two groups of SequentialMover. The acceptor is only relevant for bundling-type moves, and is absolutely necessary for bundling-type moves. It's obvious that Conditional and PartialAcceptance are meaningless without an acceptor associated with each submove, right?

Then there's Sequential. It is completely reasonable to use Sequential as a bundle (as we do now), or to use it as a multiple step trial generator (as we would like to be able to do). But you have to figure out what the correct acceptance probability is. For a bundle, it is 1 if any submover is accepted. For a multiple step trial generator, it is the product of the probabilities from its constituent parts.

I think it is possible to hack some math and maybe add some flags to keep this all in one class. But why? Isn't it more straightforward to distinguish between bundled moves and multistep trials? Doesn't making that distinction explicitly save our users from exactly the kind of confusion we had?

In the QM analogy, I think this makes complete sense. Prepare your system in some superposition state (start your move). Let it evolve for some time (generate part 1 of the trial). Let it evolve for longer time (generate part 2 of the trial). Measure now. Compare that to what would happen if you measured twice in that period. Different results -- like I said, it's quantum Zeno.

This would also make sure that you cannot mix different acceptors (would that even make sense?). I know that mixing of different acceptance schemes can introduce a bias, there is something called "Parrondo's paradox".

I don't think there's a problem with allowing different acceptors -- the user has to really want to do that in order to make it happen, so we shouldn't be getting in their way if they think they've got a clever idea. The default is just Metropolis for everything. I think Acceptor as a wrapper is the best idea so far.

@jhprinz

jhprinz commented May 22, 2015

Copy link
Copy Markdown
Contributor Author

Our "state" is the set of all active samples.

That was what I wanted to know :)

Not always -- only if the user behaves well when designing the move scheme. Even if you have a forward and backward hop, they might be submoves of another move, in which case they aren't actually the "reverse" of the other move. Look at the single replica example I drew, but pretend that instead of the SHS mover in B, we just had a B->A hop. It would be impossible to make this satisfy detailed balance, because it would be impossible (or at least immeasurably unlikely) have a move starting in B create the same trial that had been made by a move starting in A. (You could only satisfy detailed balance if you accounted for the condition that the SHS rejects every move that gives a new trajectory -- in other words, if the shooting has any net effect.)

That was not my intent. At least, the goal was not to fix a potential non-balanced simulation, but to test whether it obeys detailed balance. That would be very nice. But I agree that this involves some more logic and coding in PathMovers which might be too cumbersome at the time.

I think it is possible to hack some math and maybe add some flags to keep this all in one class. But why? Isn't it more straightforward to distinguish between bundled moves and multistep trials? Doesn't making that distinction explicitly save our users from exactly the kind of confusion we had?

Well, not really. It is just another way of placing the AcceptanceMovers somewhere in the mover tree. You want to put them with different kinds of SequentielMovers using an additional parameter, like

ConditionalSequentialMover([
ForwardShooter, Backwardshooter
], acceptor = [Metropolis]*2)

while I would prefer to put the acceptor with the mover that it accepts

ConditionalSequentialMover([
Metropolis(ForwardShooter), 
Metropolis(Backwardshooter)
])

You differentiate between bundled and multi-trial moves, while I differentiate between Movers that have an accept at the end (called real or just movers) and ones that don't (called trials).

Both ways are more or less equivalent in the tree structure of movers they generate. The Acceptance will be at the same places. So I would say, that we agree on that part. It seems to have reached the philosophical state, which is a good sign :)

Also, the [...]Mover classes are just for convenience. They are of course not needed and were meant to keep everything as close to was it was and make a beginner not think about different type of movers: If you only use "movers" (with acceptors) there are no problems other than limited functionality. To be then consistent with this limited only mover approach I added also Trials and Acceptor as simpler building blocks to make even more complex moves. No additional need for other SequentialMovers. All of these are Movers because they have acceptance build in, which is for Contitional and Partial independent of adding an Acceptance because it is already an eigenstate (I really like that way to talk about MC) nice...

To be honest this is not totally true, the SequentialMover (in my terms) is not a Mover but should have been called SequentialTrial, since it will not accept if you put in trials. If, however, you only put in "movers" then the SequentialTrial is also a SequentialMover. And so the only mover approach is selfconsistent.
This is a little tricky, but the underlying idea is pretty clear and easy to understand and we agree upon. This is already VERY powerful and we only need to get the little details done.

  • a "change" is a change in sampleset space
  • there are changes that are deterministic and changes that are uncertain until decided.
  • uncertain changes need to be projected to a real change by an acceptor, i.e. if a change is not certain we need to decide at some point if we actually do it or not.

A few questions that I think we need to sort out:

  • Do we want to have an only 'accepting mover' level as the simplest possible and if so, what do we want to call these accepting movers, e.g. just ForwardShooter, etc... or add Accepting[...] ?
  • Do we want to associate the acceptance with (a) the sequences or the (b) movers ? I prefer (b) because it seems more intuitive for me whereas (a) might be shorter to write and makes checking in Conditional and Partial obsolete. Reusability is probably better for (b) although only slightly:
    example: create 2 oneway-shooters, one with metropolis and one with a second acceptor. This could be prepared and reused at various places. If we add this to the Sequence you would have to change all sequences where this mover is used. Sure, this is a very unlikely case and should maybe not used to decide what we do, but it explains a little why my intuition leans toward (b).
  • attempt (b) could use some checking if the used submovers are acceptable, while attempt (a) will make them all acceptable. This is nice but might hide errors. A user might add an Acceptor to a Trial and not to others for some reason but the Sequential will just accept all.
  • Would renaming help?

Anyway, I need to work this out a little more and see little code examples to make a decision what is clearer.

The wrapper is a good idea. I like writing

Metropolis(OnewayShooting(...))

@dwhswenson dwhswenson mentioned this pull request May 22, 2015
12 tasks done
@dwhswenson

Copy link
Copy Markdown
Member

At least, the goal was not to fix a potential non-balanced simulation, but to test whether it obeys detailed balance.

What I'm saying, though, is that I don't think it is even possible to determine that in all cases, unless you either require that all moves be made in pairs to find the reverse version (kind of like you do with snapshots, although some movers are their own reverse) or have the move scheme builder manually identify them. Note that it also is a concept that only applies at the canonical.mover level. Outside of that, finding the detailed-balance reverse is nonsense. I think the only way to implement this is to manually set it when creating the move scheme. And I think we're going to need that for single replica.

You differentiate between bundled and multi-trial moves, while I differentiate between Movers that have an accept at the end (called real or just movers) and ones that don't (called trials).

I think I didn't explain the distinction I'm trying to make. Let me try again.

All movers are just trials. Nothing is actually accepted until the final acceptor accepts it. What I call "bundling" is not necessarily correctly called "real", because even accepted moves (during bundling) might not be "real", i.e., might not be accepted.

For example, consider the following move scheme:

image

Just because A1 was accepted part of the bundle in SeqEns doesn't mean that it will be accepted overall, because the ConditionSequential might not accept one of its later moves.

Again, there are two ways to make a move that we're discussing. One is just to create a trial move, the old-fashioned way, and accept based on the appropriate probability. Those are trials, sometimes made of subtrials.

I think it is possible to hack some math and maybe add some flags to keep this all in one class. But why? Isn't it more straightforward to distinguish between bundled moves and multistep trials? Doesn't making that distinction explicitly save our users from exactly the kind of confusion we had?

Well, not really. It is just another way of placing the AcceptanceMovers somewhere in the mover tree.

Can you explain how you plan to handle the difference in the mathematics? For a sequential trial, the correct value to return for p_metropolis is the product of the results for the steps. For a bundled move, the correct value is the maximum of the results of the steps (assuming acceptance movers return 1 or 0 for p_metropolis, which makes them trivially idempotent.)

It seems like getting this behavior out of the same class will involve adding if-statements to use completely different logic for the two cases. I would call that a sign that we really have two classes.

I remark again that ConditionalSequential and PartialAcceptanceSequential are nonsense without acceptors. Given that we have moves like that, it seems very weird to me to say "these two always must have an acceptor wrapper around any move you give them, but no other mover should ever need one, except maybe this one other thing called Sequential. If you want to use it one way, you should wrap your moves with acceptors. If you want to use it the other way, don't wrap them." Again, that sure sounds to me like the Sequential should be two different classes.

Given that it took us a while to make this distinction, I don't think we should assume that users will get it right away. This is why I think we should split Sequential into two classes: BundledSequential, which requires acceptors, and regular Sequential, which does not. Understanding the difference between BundledSequential and Sequential approaches is easier if they have separate names and thus separate documentation.

This also allows usage to be easier. Let's say a user wanted to make the repex-shoot-repex example I always give. If I understand correctly, in your implementation, this looks like:

SequentialTrial([
    MetropolisAcceptor(repex12), MetropolisAcceptor(repex23),
    MetropolisAcceptor(shoot1), MetropolisAcceptor(shoot2), MetropolisAcceptor(shoot3),
    MetropolisAcceptor(repex23), MetropolisAcceptor(repex12)
])

In mine:

BundledSequential([repex12, repex23, shoot1, shoot2, shoot3, repex12, repex23])

Both should give the same behavior. Why not use the shorter syntax?

  • there are changes that are deterministic and changes that are uncertain until decided.
  • uncertain changes need to be projected to a real change by an acceptor, i.e. if a change is not certain we need to decide at some point if we actually do it or not.

Again, in a technical sense, I might agree, but I'm pretty sure that I disgree with the general implications. There is only one place a change becomes truly "decided" or "real". Until then, it just has an acceptance probability. That probability may appear, at some point in the process, to be 100% (based on the information so far). However, it might still get rejected later, because 100% times 0% gives us 0%.

Do we want to have an only 'accepting mover' level as the simplest possible

No. There is no need for this. Looking at the various diagrams I've drawn in this thread, consider where we need an "acceptor" (an oval around the move): either at the very top of the tree, or around movers in a bundling-type sequential move.

If the user is using Metropolis, the user should never need to explicitly make an acceptor.

Do we want to associate the acceptance with (a) the sequences or the (b) movers ?

I don't understand the question. The only place acceptance actual happens is at the last step of the move. Everything else calculates a probability of acceptance.

Would renaming help?

I'm completely in support of renaming nearly everything to "Trial" ... this also gets rid of the "Generating" making some of this longer. ReplicaExchangeTrial, ForwardShootingTrial -- we can maybe find another name for the flow control parts, like Sequential. The only reason not to do that is that we're used to calling things "movers" (which isn't a very good reason).

This was referenced May 23, 2015
@dwhswenson

Copy link
Copy Markdown
Member

I wrote up some stuff that includes more detailed math around the Monte Carlo discussion we’ve been having here (including the mathematical derivation of how the "bundling" moves work). It’s all in a gist. Direct link to the PDF.

In order to be complete, it rehashes some pretty basic stuff. On the other hand, some of those basics are all you really need for the bundled move derivation!

@dwhswenson

Copy link
Copy Markdown
Member

Quick status on this. Here are the issues I think are still unresolved:

  • Extension and Shooting should be special cases of a single superclass, to simplify code maintenance
  • in_ensembles should be changed to input_ensembles (for clarity vs. InXEnsemble)
  • We should have a better general treatment of MC (and better naming for properties)

I believe that we agreed on the first two points, but the changes aren't included in the PR yet. We're still working out details for a consensus approach to the third point.

Do we want to split these off as new issues so the rest of this can get merged? I currently have 5 PRs waiting on aspects of this one, and I'm running out of code to write that doesn't require this PR to be merged.

@jhprinz

jhprinz commented May 28, 2015

Copy link
Copy Markdown
Contributor Author

Alright. I spend a lot of time thinking about the best way implementing the possibility to seprarate acceptance steps and I think I have found a way to incorporate both approaches, but I will do that in the next PR.

I agree with David in the points above and will fix this quickly so that David can continue.

The renaming, I think, will mostly involve turning all movers into trials and add a few things to add acceptors, mostly a BundledSequentiel, etc. as a shortcut for almost all sequential cases and some more little building block I think are useful. With these smaller "Acceptors" we can write the BundledSequential, etc in 2 lines of code and thus keep everyhing easy to use and maintable at the same time. More infos in the PR later today...

in and out to input and output is easy and the extension and shooting in one class is a good idea...

@dwhswenson

Copy link
Copy Markdown
Member

Quick request for clarification: the update of the MC system is definitely for the next PR, but on the other points, should I merge as-is or should I wait for new commits?

@jhprinz

jhprinz commented May 28, 2015

Copy link
Copy Markdown
Contributor Author

Let me change the in and out. The refactoring of shooting and extension is independent of the rest, so you can merge after that.

@jhprinz

jhprinz commented May 28, 2015

Copy link
Copy Markdown
Contributor Author

nosetests passed

@jhprinz

jhprinz commented May 28, 2015

Copy link
Copy Markdown
Contributor Author

ipython nb passed. So I think this is ready and does not include the shooting/extention refactoring!

@dwhswenson

Copy link
Copy Markdown
Member

passed Travis tests -- merging!

dwhswenson added a commit that referenced this pull request May 28, 2015
PathMoveChange improvements
@dwhswenson
dwhswenson merged commit 090683c into openpathsampling:master May 28, 2015
@jhprinz jhprinz mentioned this pull request May 28, 2015
@jhprinz

jhprinz commented May 31, 2015

Copy link
Copy Markdown
Contributor Author

Will continue the discussion about the final MoveScheme layout in a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ref docs issues/PRs with discussion that might be added to docs

Projects

None yet

2 participants