Skip to content
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

Skipping the Hastings computation for symmetric proposals #45

Closed
wants to merge 2 commits into from

Conversation

luiarthur
Copy link
Contributor

This is an attempt to fix #41 so that the Hastings part of the Metropolis-Hastings log acceptance probability is not computed for symmetric proposals.

I separated out the computation from AbstractMCMC.step, and defined different behaviors for the ratio computation for different proposals. Basically, if the proposal is a (vector of) symmetric distribution, the Hastings part is not computed.

I added two tests to check if a sampler is indeed symmetric. I'm happy to add tests as directed. (I found it hard to test that the correct behavior was achieved because this merely skips the computation of something that evaluates to 0...)

Not sure if this is a good way to do this. An alternative solution might have been to add a filed in MetropolisHastings (e.g. issymmetric::Bool) to specify whether the proposal is symmetric. I'm open to suggestions.

@luiarthur luiarthur changed the title Attempt to fix #41. Skipping the Hastings computation for symmetric proposals Dec 8, 2020
@devmotion
Copy link
Member

IMO it is not a good approach to encode this property in a special union type, it would be easier to extend and to implement if one would add a trait is_symmmetric_proposal(::Proposal) that could be checked inside of step. Union types are fixed and can't be changed or extended by users whereas the trait could be implemented for any custom proposal.

@luiarthur
Copy link
Contributor Author

it would be easier to extend and to implement if one would add a trait is_symmmetric_proposal(::Proposal) that could be checked inside of step.

That sounds good to me. I'll take another stab at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skipping the Hastings computation for symmetric proposals
2 participants