{"action":"created","issue":{"url":"https://api.github.com/repos/choderalab/openpathsampling/issues/295","labels_url":"https://api.github.com/repos/choderalab/openpathsampling/issues/295/labels{/name}","comments_url":"https://api.github.com/repos/choderalab/openpathsampling/issues/295/comments","events_url":"https://api.github.com/repos/choderalab/openpathsampling/issues/295/events","html_url":"https://github.com/choderalab/openpathsampling/pull/295","id":92136373,"number":295,"title":"Sets of Trees","user":{"login":"jhprinz","id":5091934,"avatar_url":"https://avatars.githubusercontent.com/u/5091934?v=3","gravatar_id":"","url":"https://api.github.com/users/jhprinz","html_url":"https://github.com/jhprinz","followers_url":"https://api.github.com/users/jhprinz/followers","following_url":"https://api.github.com/users/jhprinz/following{/other_user}","gists_url":"https://api.github.com/users/jhprinz/gists{/gist_id}","starred_url":"https://api.github.com/users/jhprinz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jhprinz/subscriptions","organizations_url":"https://api.github.com/users/jhprinz/orgs","repos_url":"https://api.github.com/users/jhprinz/repos","events_url":"https://api.github.com/users/jhprinz/events{/privacy}","received_events_url":"https://api.github.com/users/jhprinz/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":16,"created_at":"2015-06-30T16:23:52Z","updated_at":"2015-07-05T21:08:51Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/choderalab/openpathsampling/pulls/295","html_url":"https://github.com/choderalab/openpathsampling/pull/295","diff_url":"https://github.com/choderalab/openpathsampling/pull/295.diff","patch_url":"https://github.com/choderalab/openpathsampling/pull/295.patch"},"body":"This tries to solve some issues and cleanup PathMovers and PathMoveChanges (once and for all ;)\r\n\r\nAddressing issue #284, #288.\r\n\r\nLet me give a brief overview about PathMover and PathMoveChanges and their tree structure\r\n\r\n#### PathMovers and the PathMoveTree\r\n\r\nA PathMover is organized in a tree-like fashion. The idea is that each node is a Mover and gets a ´SampleSet´ (usually called the ´globalstate´) and will output a corresponding ´PathMoveChange´.\r\nA PathMover itself can access the results of underlying PathMovers and do something with them.\r\nThe underlying PathMovers are children of a node and here also the order matters, e.g. in a SequentialMover, simply other movers are run in sequence.\r\nThis gets complicated when we allow to chose between different possibilities of movers. This means effectively that our PathMoveTree is not a fixed tree, but a 'RandomProcess' that outputs a tree. This gets even more complicated when we allow the decisions of which mover to chose to be based on the actual global state that is passed to the mover.\r\n\r\nSo, the PathMover itself is providing a set of movers to be run and also the logic on which of these movers to pick.\r\n\r\nNow the problem that makes it hard to deal with these trees right now: We have two kinds of leaves / submovers of a PathMover node.\r\n\r\n1. The leaves can mean choices of movers to pick, so they are part of the 'tree generating process'\r\n2. The leaves can mean a sequence of movers\r\n3. There is even a mixed one `ConditionalPathMover`\r\n\r\nIf we clearly separate these two concepts then the problem of keys can be solved and also searching and subtrees make sense.\r\n\r\nThe example that @dwhswenson gave in #288 hinted me to this. The keys he provided seemed unique and logical, but I could not make out what would happen if we have a sequence of movers. As long as we have choices in a mover like RandomSelectionPathMover there will be only one mover picked from a set of possibilities and hence it is enough in the key to just specify the mover that was picked. For a sequence of movers like \r\n```\r\nmover = Sequential([shootA, reversal, shootA])\r\n```\r\nwe want to be able to say which one we mean, the first or last one. So, in RandomChoices the same mover does not make sense (or could be combined into one), but in a sequence it does. So keys for sequential would look like this:\r\n```\r\n[Sequential, [shootA]]\r\n[Sequential, [shootA], [reversal]]\r\n[Sequential, [shootA], [reversal], [shootA]]\r\n```\r\nor a little simpler\r\n```\r\n[Sequential, [shootA]]\r\n[Sequential, None, [reversal]]\r\n[Sequential, None, None, [shootA]]\r\n```\r\nThis key will point exactly to each possible mover and you can also ask\r\n```\r\n# check if a specific mover was called in a particular PathMoverChange\r\nchange = st.pathmovechanges[0]\r\n[Sequential, None, None, [shootA]] in change\r\n```\r\nto check if this particular mover was called. So, this idea would solve the problem that we have with too long keys. I have implemented this and it works.\r\n\r\nImportant is here that these keys make only sense (so far) to be called with `PathMoveChanges` and not with the original PathMover. Simple example why this fails:\r\n\r\n```\r\nmover = RandomChoice([\r\n    SequentialA([shootA, reversal, shootA]),\r\n    SequentialB([shootB, reversal, shootB]) # this Sequential is different from the first!\r\n])\r\n# give keys for all movers in mover\r\nprint mover.keylist()\r\n>>> [RandomChoice]\r\n>>> [RandomChoice, [SequentialA]]\r\n>>> [RandomChoice, [SequentialA, [shootA]]]\r\n>>> [RandomChoice, [SequentialA, [shootA], [reversal]]]\r\n>>> [RandomChoice, [SequentialA, [shootA], [reversal], [shootA]]]\r\n>>> [RandomChoice, [SequentialB]]\r\n>>> [RandomChoice, [SequentialB, [shootB]]]\r\n>>> [RandomChoice, [SequentialB, [shootB], [reversal]]]\r\n>>> [RandomChoice, [SequentialB, [shootB], [reversal], [shootB]]]\r\n```\r\nThis seems reasonable so far. Now check if the last mover is in the mover\r\n```\r\nprint [key in mover for key in mover.keylist()]\r\n>>> [True, True, True, True, True, False, False, False, False]\r\n```\r\nThe problem is that `in` check for being a subtree starting with the same root and this works for the first few, but when SequentialB is used this is the second node and hence it is not a subtree the way we think about it now. This would be work, but is not, what we want:\r\n```\r\n>>> [RandomChoice, [SequentialA], [SequentialB]]\r\n>>> [RandomChoice, [SequentialA], [SequentialB, [shootB]]]\r\n>>> [RandomChoice, [SequentialA], [SequentialB, [shootB], [reversal]]]\r\n>>> [RandomChoice, [SequentialA], [SequentialB, [shootB], [reversal], [shootB]]]\r\n```\r\nIt looks like SequentialA and SequentialB are run in Sequence which is not true. So what is now missing is a subtree check, that nows, that some leaves are sequences and some are choices.\r\nAlso this keys will never work in a PathMoveChange!\r\n\r\nI have added a hidden function `_leaves` that will give detailed information about the meaning of the submovers. What is missing is to make the `in` function for PathMovers aware of the difference of choice and sequence.\r\n\r\nI think it is important that most user do not have to deal with it and it should just work. It is also mostly to have unique keys that work for PathMovers and PathMoveChanges alike. So, ultimately all of this should work:\r\n```\r\nmover = RandomChoice([\r\n    SequentialA([shootA, reversal, shootA]),\r\n    SequentialB([shootB, reversal, shootB]) # this Sequential is different from the first!\r\n])\r\n# change is a concrete run\r\nchange = RandomChoiceChange([\r\n    SequentialChange([shootBChange, reversalChange, shootBChange])\r\n]) \r\nmover_keys = mover.keylist() # see above for list\r\nprint mover_keys\r\n>>> [RandomChoice]\r\n>>> [RandomChoice, [SequentialA]]\r\n>>> [RandomChoice, [SequentialA, [shootA]]]\r\n>>> [RandomChoice, [SequentialA, [shootA], [reversal]]]\r\n>>> [RandomChoice, [SequentialA, [shootA], [reversal], [shootA]]]\r\n>>> [RandomChoice, [SequentialB]]\r\n>>> [RandomChoice, [SequentialB, [shootB]]]\r\n>>> [RandomChoice, [SequentialB, [shootB], [reversal]]]\r\n>>> [RandomChoice, [SequentialB, [shootB], [reversal], [shootB]]]\r\n\r\nchange_keys = change.keylist()\r\nprint change_keys\r\n>>> [RandomChoice]\r\n>>> [RandomChoice, [SequentialB]]\r\n>>> [RandomChoice, [SequentialB, [shootB]]]\r\n>>> [RandomChoice, [SequentialB, [shootB], [reversal]]]\r\n>>> [RandomChoice, [SequentialB, [shootB], [reversal], [shootB]]]\r\n\r\n# all keys should be valid subtrees of the whole mover\r\n[key in mover for key in mover_keys] \r\n>>> [True, True, True, True, True, True, True, True, True]\r\n\r\n# all keys in the PMC should be in the PMC\r\n[key in change for key in change_keys]\r\n>>> [True, True, True, True, True]\r\n\r\n# only the keys of the mover that were used should be true\r\n[key in change for key in mover_keys]\r\n>>> [True, False, False, False, False, True, True, True, True]\r\n\r\n# but of course all keys in PMC should be part of the PathMover (since they were generated by it)\r\n[key in change for key in mover_keys]\r\n>>> [True, True, True, True, True]\r\n```\r\n\r\nFinally it could make sense to treat `__iter__` as running in order over all 'keys' and not over the nodes. Much like a dictionary runs over all keys and so our special tree will run over all keys and you can use the key to access a specific node.\r\n```\r\nfor key in mover:\r\n    node = mover[key]\r\n    assert(key in mover) # will always work\r\nfor key in change:\r\n    node = mover[key]\r\n    assert(key in change) # will always work\r\n```\r\n\r\n#### Summary\r\nThis PR changes the treelogic to be aware of different meaning of child nodes : sequence, choice\r\n\r\nIt adds\r\n- [x] add `._leaves` to get information about meaning of leaves\r\n- [x] keys are valid subtrees\r\n- [x] keys should work for PMC and PM alike\r\n- [ ] change `in` for PathMover to use `_leaves`\r\n- [ ] change `__iter__` to interate over keys\r\n- [ ] testing\r\n- [ ] example ipynb"},"comment":{"url":"https://api.github.com/repos/choderalab/openpathsampling/issues/comments/118667752","html_url":"https://github.com/choderalab/openpathsampling/pull/295#issuecomment-118667752","issue_url":"https://api.github.com/repos/choderalab/openpathsampling/issues/295","id":118667752,"user":{"login":"jhprinz","id":5091934,"avatar_url":"https://avatars.githubusercontent.com/u/5091934?v=3","gravatar_id":"","url":"https://api.github.com/users/jhprinz","html_url":"https://github.com/jhprinz","followers_url":"https://api.github.com/users/jhprinz/followers","following_url":"https://api.github.com/users/jhprinz/following{/other_user}","gists_url":"https://api.github.com/users/jhprinz/gists{/gist_id}","starred_url":"https://api.github.com/users/jhprinz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jhprinz/subscriptions","organizations_url":"https://api.github.com/users/jhprinz/orgs","repos_url":"https://api.github.com/users/jhprinz/repos","events_url":"https://api.github.com/users/jhprinz/events{/privacy}","received_events_url":"https://api.github.com/users/jhprinz/received_events","type":"User","site_admin":false},"created_at":"2015-07-05T21:08:51Z","updated_at":"2015-07-05T21:08:51Z","body":"Hmmm, just deleted a huge post... Because I clicked to early on PRs... I am really annoyed...\r\n\r\nBasically,\r\n\r\n- yes, I realized the same problem\r\n- I would like to keep the trees we have right now\r\n- keys are short and point to a specific node in the tree\r\n- keys work for PM and PMC\r\n- we add a `.next` and `.prev` function to PMCs like\r\n```\r\npmc = ...\r\n# get the next key\r\nnext_key = pac.next(key)\r\nprev_key = pmc.prev(key)\r\n```\r\n- iterate changes in a PMC exactly like iterating a (ordered) dictionary in python\r\n```\r\nfor key, change in pmc.iteritems():\r\n    pass\r\n```\r\nor\r\n```\r\nfor key in pmc:\r\n    change = pmc[key]\r\n```\r\nor even\r\n```\r\nkey = None\r\nwhile key = pmc.next(key):\r\n    change = pmc[key]\r\n```\r\n- the `__getitem__(key)` for PM or PMC picks a specific node in the tree indicated by the key\r\n- the key itself is a branch that is unique enough to identify a node\r\n- your _key_ would be accessed using a `.past` function, e.g.\r\n```\r\npast = pmc.past(key)\r\n>>> [shootA, shootB, reversal]\r\n```\r\n- `__contains__` just tests if a branch is in the PM or PMC\r\n- `__contains__` and `__getitem__` can have wildcards in which case the first element found it returned\r\n\r\nSo, we keep the PMC, PM almost it is now and add DAG capabilities. So you can traverse using next or you can walk the tree (and both is the same!)\r\n\r\nSorry for the brevity, but I did not wanna repeat the long post now."}}