-
Notifications
You must be signed in to change notification settings - Fork 22
Skipping the Hastings computation for symmetric proposals #46
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
Conversation
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.
I think this is a better way to skip computations for symmetric proposals 👍
I added some comments and suggestions.
src/mh-core.jl
Outdated
@@ -191,6 +191,41 @@ function AbstractMCMC.step( | |||
return transition, transition | |||
end | |||
|
|||
""" | |||
is_symmetric_proposal(proposal::P) where P | |||
|
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.
Can you add some explanation here?
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.
Just added some documentation there.
test/runtests.jl
Outdated
|
||
# Set up the proposal. | ||
p1 = (x=RandomWalkProposal(Normal(0,.5)), y=RandomWalkProposal(Normal(0,.5))) | ||
AdvancedMH.is_symmetric_proposal(proposal::typeof(p1)) = true |
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.
This is not a very "nice" example - usually one would want to define the trait for a custom proposal or distribution.
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.
Maybe define a custom distribution, e.g., of normal distributions with zero mean?
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.
Sounds good to me.
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.
Just modified the test.
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
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.
I like it! No comments from me, though could you increment the version number to 0.5.6 before this is merged in?
The tests seem to be passing. But in the logs, I see this at the beginning (before the tests are actually run):
Any thoughts? |
I think that's because of a hangup in MCMCChains (TuringLang/MCMCChains.jl#252), but it doesn't seem to have impacted the tests so I think we can ignore it. |
This is a do-over of #45 for #41 which adds a method
is_symmmetric_proposal
whose behavior can be extended by users.