Skip to content

Conversation

@phipsgabler
Copy link
Member

@phipsgabler phipsgabler commented May 12, 2020

This is just the same as #1172, but I did some bad things with Git there, so creating a new PR seemed easier


Introduces a sampler GibbsConditional that can be used to manually specify a -- surprise! -- Gibbs conditional for a variable:

sample(model, Gibbs(GibbsConditional(:λ, cond_λ), GibbsConditional(:m, cond_m)), N)

See also this discussion.

where the cond_v are functions from a NamedTuple of the conditioned variables ((λ,) for m and vice versa in the above example) to a Distribution (or probably just a Sampleable is enough). Example code.

Currently, only one variable per sampler object is handled (as opposed to some other samplers). Of course extension is possible, but I believe it is better to leave it this way and let Gibbs handle the sweeps.

Since this seemed too simple, I'm quite sure there's work left. At least:

  • There's a lot of stuff (esp. assume/observe.) simply copied from MH -- can this be simplified or at least abstracted?
  • Documentation!
  • Tests, naturally
    • Basic feasibility (gdemo, mixture of Gaussians)
    • Check whether indexed variables really work
    • Mixing Gibbs conditionals with other samplers
  • Doctests (discuss first)

Currently, the code is working on the tests when only the conditionals or a combination of conditioals and MH is used, but fails in combination with an HMC sampler due to some AD things I don't understand.

@cpfiffer
Copy link
Member

cpfiffer commented Nov 9, 2020

The question is also if we want to merge this PR or #1428 (updates Turing to AbstractMCMC 2 which has a different interface and hence requires different implementations of GibbsConditional) first. I have a deadline tomorrow but will try to finish the other PR (and the corresponding one to DynamicPPL) on Wednesday this week.

I would say this one first since it's "smaller" in that it touches less of the code base.

phipsgabler and others added 2 commits November 10, 2020 18:43
(I'll test the rest manually)

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
@phipsgabler
Copy link
Member Author

Should be fine now; there's still some issue with variable subsumption, but @trappmartin agreed that I should merge already. I'll make a separate issue.

@phipsgabler phipsgabler merged commit f7813b2 into TuringLang:master Nov 13, 2020
@phipsgabler phipsgabler deleted the phg/new_gibbs_conditionals branch November 13, 2020 18:28
@devmotion
Copy link
Member

You forgot to bump the version number - can you fix this in a new PR? 🙂

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.

6 participants