-
Notifications
You must be signed in to change notification settings - Fork 23
Support proposals of varying dimensions in vector-of-proposals (#120) #121
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.
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
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
5c1545d
Support proposals of varying dimensions in vector-of-proposals (#120)
sunxd3 151c8fe
Add proposal_dim method for function-valued proposals
sunxd3 0a52274
Bump version to 0.8.10
sunxd3 5cf0323
Add dimension mismatch check in _split_params and fix single-element …
sunxd3 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code in the PR all looks good, and I know I didn't say this in the original issue, but there are still quite a few more blockers to using this upstream in Turing. For example in this line which handles conditional proposals, are we implicitly assuming that the proposal is necessarily conditional on a single element?
Consider e.g.
where
uniis some univariate distribution, and we want to propose new values ofxbased on the value at the previous step. Then we would need something likeOr even something like a matrix distribution, which we would flatten inside an LDF. I don't immediately know how we can represent this in AdvancedMH where everything has to be flattened. I suppose that we would have to use something like a
StaticProposal(vec(matrix_dist)).Again, I don't think this has to be handled in this PR (if anything I actively think it shouldn't be). To be completely honest, I also don't know how to handle this, short of adding another field to StaticProposal that specifies the length. Just wanted to check that I'm understanding correctly
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically the problem is, if I have a high-level construct like
how do we translate that into AdvancedMH. From an LDF we can identify the range of indices that each varname belongs to, so as long as we can translate the individual
prop's into AdvancedMH proposals, this PR will let us then stack them together, which is great.But we still need to be able to translate the
props and that can depend quite a bit on whatpropis, whatx's distribution in the model is, etc. For example ifx ~ LKJCholesky()and we want to say 'sample x from the prior', that is quite easy to do in Turing because you have access to the distribution. But in AdvancedMH we would need to translate this into 'sample a vectorised form of x from a vectorised form of LKJCholesky'. Then it gets more complex if you have a conditional distribution etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are tricky interface questions, we'll deal with them when they emerge
on the first comment, my cop out thought is that, if the user is serious about using this type of proposal, they can overload
proposal_dim. this is not quite nice, but I am going to leave it there. that said, I added an error messageThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw I'm happy to merge and release this!