Inference algorithm for sampling from the prior

[torfjelde]

Allows you to do stuff like

chain = sample(model, PriorSampler(), 1000)

returning a chain with samples from the prior.

[devmotion]

TuringLang/DynamicPPL.jl#77 would provide the same functionality, it seems?

[torfjelde]

Ah, hadn't seen that! Though we'd still need to wrap it in a InferenceAlgorithm in Turing so that we get back a chain rather than a vector of varinfos, right?

[devmotion]

No, that's not needed. For convenience we allow users to specify InferenceAlgorithms, but the only thing we do is to wrap it in a Sampler object and forward it to the implementation of sample in Turing that specifies the default keyword arguments (which are responsible for getting a chain).

[torfjelde]

Hmm, yeah so I actually did that initially, i.e. only implemented step! but ran into some error and I couldn't quite figure out the source of the bug.

Can you point me to where we convert the vector of varinfos to a chain? Looking at bundle_samples with Type{Chains} it's not quite clear to me whether or not this supports a vector of varinfos as input (doesn't seem to because of the use of _params_to_array).

[codecov]

Codecov Report

Merging #1225 into master will decrease coverage by 0.56%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1225      +/-   ##
==========================================
- Coverage   66.14%   65.58%   -0.57%     
==========================================
  Files          25       26       +1     
  Lines        1288     1299      +11     
==========================================
  Hits          852      852              
- Misses        436      447      +11     

Impacted Files	Coverage Δ
src/inference/Inference.jl	78.78% <ø> (ø)
src/inference/prior.jl	0.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe79450...b58ee5e. Read the comment docs.

[devmotion]

Yes, I expected that but that's not related to InferenceAlgorithm. I guess only the implementations of bundle_samples for these samplers and the desired output types defined in Turing are needed. Maybe one can separate the logic into a separate function that can be called both from an implementation of bundle_samples for VarInfo vectors and vectors of transitions?

[torfjelde]

Yeah sorry, I didn't mean that I disagreed with you; with your PR there's no need to make this a InferenceAlgorithm, but in hindsight I think the error I ran into when implementing step! was because the bundle_samples ended up being the identity → weird stuff happened.

And yeah, it shouldn't be too much work, but maybe we should have that impl of bundle_samples in https://github.com/TuringLang/AbstractMCMC.jl/blob/d5ebd3b579abf805b738d8fb5a8b6d5198670341/src/interface.jl#L52 since AbstractMCMC.jl depends on DynamicPPL.jl? It seems we just need to define one for ts::AbstractVector{<:VarInfo} and Type{Chains}.

[cpfiffer]

I think actually this might be better served in Turing and not in DynamicPPL -- DynamicPPL doesn't really use AbstractMCMC that much so it surprises me to see it there.

[devmotion]

And yeah, it shouldn't be too much work, but maybe we should have that impl of bundle_samples in TuringLang/AbstractMCMC.jl:src/interface.jl@d5ebd3b#L52 since AbstractMCMC.jl depends on DynamicPPL.jl?

The implementation of bundle samples for Vector{NamedTuple} could probably go to DynamicPPL (actually, it has to since otherwise it would be type piracy), but it is not clear what to do for Chains - DynamicPPL does not depend on MCMCChains currently, so one would have to change that (Chains is only defined in MCMCChains). The implementation should definitely not go to AbstractMCMC since that package just contains the abstract inference and doesn't define Sampler, Model, or Chains.

I think actually this might be better served in Turing and not in DynamicPPL -- DynamicPPL doesn't really use AbstractMCMC that much so it surprises me to see it there.

IMO actually parts of the AbstractMCMC interface for Sampler (and SampleFromUniform and SampleFromPrior) that do not contain any types defined in Turing (such as, e.g.,

Turing.jl/src/inference/Inference.jl

Lines 213 to 236 in fe79450

	function AbstractMCMC.sample_init!(
	::AbstractRNG,
	model::Model,
	spl::Sampler,
	N::Integer;
	kwargs...
	)
	# Resume the sampler.
	set_resume!(spl; kwargs...)
	# Set the parameters to a starting value.
	initialize_parameters!(spl; kwargs...)
	end
	function AbstractMCMC.sample_end!(
	::AbstractRNG,
	::Model,
	::Sampler,
	::Integer,
	::Vector;
	kwargs...
	)
	# Silence the default API function.
	end

(BTW sample_end! should just be removed) or

Turing.jl/src/inference/Inference.jl

Lines 402 to 429 in fe79450

	function AbstractMCMC.bundle_samples(
	rng::AbstractRNG,
	model::Model,
	spl::Sampler,
	N::Integer,
	ts::Vector,
	chain_type::Type{Vector{NamedTuple}};
	discard_adapt::Bool=true,
	save_state=false,
	kwargs...
	)
	nts = Vector{NamedTuple}(undef, N)
	for (i,t) in enumerate(ts)
	k = collect(keys(t.θ))
	vs = []
	for v in values(t.θ)
	push!(vs, v[1])
	end
	push!(k, :lp)
	nts[i] = NamedTuple{tuple(k...)}(tuple(vs..., t.lp))
	end
	return map(identity, nts)
	end

) should be moved to DynamicPPL or be specialized for Transition. Otherwise this is just type piracy. Since DynamicPPL already depends on AbstractMCMC, the correct place to define default implementations of the interface for inputs of type Model, Sampler (and SampleFromUniform and SampleFromPrior) and some other base types is DynamicPPL.

[mohamed82008]

I also prefer this over having it in DPPL mainly because TypedVarInfo is used here.

[devmotion]

I also prefer this over having it in DPPL mainly because TypedVarInfo is used here.

We could easily change that in DynamicPPL. To me it just seems weird to define a Sampler{<:PriorSampler} if we already have a SampleFromPrior - so why don't we use it for sampling from the prior? And as explained above, even if (so far) we only want to sample from the prior together with the other functionality in Turing, any implementation of the AbstractMCMC interface for SampleFromPrior and SampleFromUniform has to be part of DynamicPPL and not of Turing - it does not depend on anything Turing-specific and hence it is just bad type piracy to add it to Turing. This is also indicated by the fact that if someone else wants to write a Turing-alternative based on DynamicPPL, they still would want to use exactly the same implementation for sampling from the prior.

[yebai]

I lean towards to keep this in DynamicPPL if possible. It feels a bit redundant to have two samplers: PriorSampler and SampleFromPrior. We can return a vector of VarInfo in DynamicPPL if necessary. In that case, we only need to implement a method for converting VarInfo vector to Chain in Turing.

I also prefer this over having it in DPPL mainly because TypedVarInfo is used here.

@mohamed82008 I'm not sure I understand the issue here. Can you clarify what do you mean TypedVarInfo is in Turing?

[yebai]

Replaced by #1243