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

Add MetropolisHastings algorithm to example of MCMCKernel #680

Merged
merged 2 commits into from Jul 15, 2020

Conversation

fehiepsi
Copy link
Member

Addresses #674. This also fixes some issues if users' MCMC state does not have z, diverging, or mean_accept_prob fields, or even not a namedtuple.

cc @PHYSICSIDEA

@neerajprad
Copy link
Member

The example looks great. The changes to support arbitrary fields works for now, but I think that instead of hardcoding ('z', 'diverging') as the set of default fields to collect in MCMC, we should instead have a property MCMCKernel.default_collect_fields so that the onus of naming their state parameters is on the users, and we can use that within the MCMC class. The current interface would still work as users will be able to optionally ask for additional fields via extra_fields. WDYT?

@fehiepsi
Copy link
Member Author

I think it is a good idea. I like it. Maybe it is more useful to have two fields: default_sample_field, default_extra_fields? A property for get_diagnostics_str function is also useful for the general kernel I think. Do you want to address it in a separate PR?

@neerajprad
Copy link
Member

+1 on get_diagnostics_str. I'm not yet sure if making a distinction between the sample field and other fields will be important, but it is worth keeping in mind.

Do you want to address it in a separate PR?

Sure, I'll add this in a separate PR.

@neerajprad neerajprad merged commit 0857645 into pyro-ppl:master Jul 15, 2020
@fehiepsi
Copy link
Member Author

making a distinction between the sample field and other fields

I just thought that if get_samples returns only z field, then we need a way to separate it (I assumed that if users define x as a sample field, they will want mcmc.get_samples returns x field). I don't have a strong opinion on this though. Requiring z as the sample field is also good IMO.

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

Successfully merging this pull request may close these issues.

None yet

2 participants