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

Alternative tutorial on implementing samplers #468

Merged
merged 14 commits into from
Jun 11, 2024

Conversation

torfjelde
Copy link
Member

@torfjelde torfjelde commented May 31, 2024

I did a talk on Turing.jl for a statistical audience earlier today and described how it's possible to implement a samplers in AbstratMCMC.jl in such a way that it's very easy to use the resulting sampler to target both Turing.jl and Stan models (through BridgeStan).

Afterwards I was asked if there was some tutorial or something online outlining how to do this in a detailed way. After having a look at the current docs on how to use the externalsampler, I felt they were a bit overly complicated and didn't quite get all the points across, and so I did a rewrite of it.

I think this version will be very, very useful for inference researchers who want their methods to become available for a larger userbase:)

EDIT: Note that I haven't added example with StanLogDensityProblems.jl, but I think we maybe should, as it would be quite nice.

I'm also thinking we should make the example a bit more complicated later in the tutorial, e.g. adding stuff like "how to test the sampler with MCMCTesting.jl" from @Red-Portal , adding a few more things to the MALAState, e.g. isaccept to indicate whether it got accepted, maybe add some adaptation, etc. But I think the current version should be merged first, and then we can build on it later 👍

Copy link
Contributor

Preview the changes: https://turinglang.org/docs/pr-previews/468, Please avoid using the search feature and navigation bar in PR previews!

@yebai
Copy link
Member

yebai commented May 31, 2024

Let’s add this as a new docs instead of overriding an existing one. I am aware of users relying on the pathfinder and external samplers in current docs.

@torfjelde
Copy link
Member Author

But the current docs are somewhat out of date + it's too similar in what they describe IMO. Maybe it makes more sense to add another section at the end of this tutorial demonstrating some examples, where I'll add back the examples in the original?

@torfjelde
Copy link
Member Author

Maybe it makes more sense to add another section at the end of this tutorial demonstrating some examples, where I'll add back the examples in the original?

Happy with this Hong?

@yebai
Copy link
Member

yebai commented Jun 4, 2024

Let's keep both tutorials. This tutorial can be called something else, but go to the For developers (Inference) section.

The previous tutorial on external samplers provides the basic mechanism for calling AbstractMCMC-compatible external samplers. This PR can build upon that and provide instructions on implementing a new AbstractMCMC sampler.

It's okay to reorganise docs tutorials later, but I felt we needed to do that after making a plan and reviewing all existing docs.

@torfjelde
Copy link
Member Author

Done 👍

@torfjelde
Copy link
Member Author

@yebai notice that I've implemented a version of MALA which uses a single leapfrog step rather than the "standard" formulation of MALA. My original intention was to add a final section on a more "complex" version being autoMALA. Buuut I'm uncertain if this is worth it or not. Should I still pursue this or just leave it for now?

@yebai
Copy link
Member

yebai commented Jun 6, 2024

@mhauru can you take a look at this tutorial?

Copy link
Member

@mhauru mhauru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff, I learned a lot from reading it. I only have some localised comments and typo fixes.

tutorials/docs-17-implementing-samplers/index.qmd Outdated Show resolved Hide resolved
tutorials/docs-17-implementing-samplers/index.qmd Outdated Show resolved Hide resolved
tutorials/docs-17-implementing-samplers/index.qmd Outdated Show resolved Hide resolved
tutorials/docs-17-implementing-samplers/index.qmd Outdated Show resolved Hide resolved
tutorials/docs-17-implementing-samplers/index.qmd Outdated Show resolved Hide resolved
)
model = model_wrapper.logdensity
# Let's just create the initial state by sampling using a Gaussian.
x = randn(rng, LogDensityProblems.dimension(model))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only valid for our particular model, right? In that this may e.g. give negative values, which might not be valid parameters for some models. Don't want to overcomplicate the example, but might it be worth mentioning that in general one should e.g. sample from the prior?

EDIT: Noticed that you address this later. Maybe when you address it mention that the fix would be to change this method of step?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if you use a different sample method here which samples values inside the support of the distribution, you'd still run into the same bug (just further down the callstack) because a gradient-based sampler like MALA will inevitably end up exploring the space outside of the support (unless you have a tiny stepsize). So we can't really implement this sampler for models (in a generic fashion, without a lot of gluecode) in such a way that it also works with constrained distributions.

Heeeence I just avoid the issue entirely, demosntrate a case later and how we can use Turing.jl to "address" this.

using Turing

# Overload the `getparams` method for our "sample" type, which is just a vector.
Turing.Inference.getparams(::Turing.Model, x::AbstractVector{<:Real}) = x
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused by this, which looks like type piracy to me. Should this rather be defined for MALAState?

EDIT: Also just noticed that the docstring of getparams says "Return a named tuple of parameters", which adds to my confusion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this was just a temporary hack.

And regarding the getparams docstring; it's unfortunately quite a mess. getparams is used in different ways depending on what inputs it receives. We decided to just overload this for the AbstractMCMC.jl purpose because we're going to move the particular implementations that relates to AbstractMCMC.jl samplers into AbstractMCMC.jl itself, in which case we do want to call it getparams (and then the current Turing.Inference.getparams should never be touched outside of Turing.jl).

But I agree that the type-piracy should be avoided, so I'll define a MALASample (note that the "sample" returned from step is generally going to be different from the state; the "sample" should contain information "for public consumption" while the state should contain all the needed information for the MCMC sampler).

tutorials/docs-17-implementing-samplers/index.qmd Outdated Show resolved Hide resolved
tutorials/docs-17-implementing-samplers/index.qmd Outdated Show resolved Hide resolved
tutorials/docs-17-implementing-samplers/index.qmd Outdated Show resolved Hide resolved
@torfjelde
Copy link
Member Author

This should be ready now:)

@torfjelde torfjelde merged commit 71e2303 into master Jun 11, 2024
3 checks passed
github-actions bot added a commit that referenced this pull request Jun 11, 2024
@yebai yebai deleted the torfjelde/alternative-sampler-docs branch June 20, 2024 17:24
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.

3 participants