Gibbs soapbox

[penelopeysm]

Turing.jl/src/mcmc/gibbs.jl

Lines 413 to 423 in fc32e10

	# Take initial step with the current sampler.
	_, new_state = step_function(
	rng,
	conditioned_model,
	sampler;
	# FIXME: This will cause issues if the sampler expects initial params in unconstrained space.
	# This is not the case for any samplers in Turing.jl, but will be for external samplers, etc.
	initial_params=initial_params_local,
	kwargs...,
	)
	new_vi_local = varinfo(new_state)

This implicitly assumes that step_function(...) returns something with a varinfo -- which in turn depends on the sampler -- needs to be either documented or enforced through types

Turing.jl/src/mcmc/gibbs.jl

Lines 454 to 456 in fc32e10

	samplers = alg.samplers
	states = state.states
	@assert length(samplers) == length(state.states)

Nasty @assert.

Turing.jl/src/mcmc/gibbs.jl

Lines 214 to 218 in fc32e10

	# Returns
	- A new model with the variables _not_ in `target_variables` conditioned.
	- The `GibbsContext` object that will be used to condition the variables. This is necessary
	because evaluation can mutate its `global_varinfo` field, which we need to access later.
	"""

Under what conditions exactly can that global_varinfo be mutated?

@model function inner()
    @show __context__
    x ~ Normal()
end
@model function outer()
    a ~ to_submodel(inner())
    b ~ to_submodel(inner())
end
spl = Gibbs(@varname(a.x) => MH(), @varname(b.x) => MH())
sample(outer(), spl, 100)

__context__ = DynamicPPL.ValuesAsInModelContext{Turing.Inference.GibbsContext{Tuple{AbstractPPL.VarName{:a, Accessors.PropertyLens{:x}}}, Base.RefValue{DynamicPPL.VarInfo{@NamedTuple{a::DynamicPPL.Metadata{Dict{AbstractPPL.VarName{:a, Accessors.PropertyLens{:x}}, Int64}, Vector{Normal{Float64}}, Vector{AbstractPPL.VarName{:a, Accessors.PropertyLens{:x}}}, Vector{Float64}}, b::DynamicPPL.Metadata{Dict{AbstractPPL.VarName{:b, Accessors.PropertyLens{:x}}, Int64}, Vector{Normal{Float64}}, Vector{AbstractPPL.VarName{:b, Accessors.PropertyLens{:x}}}, Vector{Float64}}}, Float64}}, DynamicPPL.PrefixContext{AbstractPPL.VarName{:a, typeof(identity)}, DynamicPPL.DefaultContext}}}(OrderedDict{Any, Any}(), true, Turing.Inference.GibbsContext{Tuple{AbstractPPL.VarName{:a, Accessors.PropertyLens{:x}}}, Base.RefValue{DynamicPPL.VarInfo{@NamedTuple{a::DynamicPPL.Metadata{Dict{AbstractPPL.VarName{:a, Accessors.PropertyLens{:x}}, Int64}, Vector{Normal{Float64}}, Vector{AbstractPPL.VarName{:a, Accessors.PropertyLens{:x}}}, Vector{Float64}}, b::DynamicPPL.Metadata{Dict{AbstractPPL.VarName{:b, Accessors.PropertyLens{:x}}, Int64}, Vector{Normal{Float64}}, Vector{AbstractPPL.VarName{:b, Accessors.PropertyLens{:x}}}, Vector{Float64}}}, Float64}}, DynamicPPL.PrefixContext{AbstractPPL.VarName{:a, typeof(identity)}, DynamicPPL.DefaultContext}}((a.x,), Base.RefValue{DynamicPPL.VarInfo{@NamedTuple{a::DynamicPPL.Metadata{Dict{AbstractPPL.VarName{:a, Accessors.PropertyLens{:x}}, Int64}, Vector{Normal{Float64}}, Vector{AbstractPPL.VarName{:a, Accessors.PropertyLens{:x}}}, Vector{Float64}}, b::DynamicPPL.Metadata{Dict{AbstractPPL.VarName{:b, Accessors.PropertyLens{:x}}, Int64}, Vector{Normal{Float64}}, Vector{AbstractPPL.VarName{:b, Accessors.PropertyLens{:x}}}, Vector{Float64}}}, Float64}}(DynamicPPL.VarInfo{@NamedTuple{a::DynamicPPL.Metadata{Dict{AbstractPPL.VarName{:a, Accessors.PropertyLens{:x}}, Int64}, Vector{Normal{Float64}}, Vector{AbstractPPL.VarName{:a, Accessors.PropertyLens{:x}}}, Vector{Float64}}, b::DynamicPPL.Metadata{Dict{AbstractPPL.VarName{:b, Accessors.PropertyLens{:x}}, Int64}, Vector{Normal{Float64}}, Vector{AbstractPPL.VarName{:b, Accessors.PropertyLens{:x}}}, Vector{Float64}}}, Float64}((a = DynamicPPL.Metadata{Dict{AbstractPPL.VarName{:a, Accessors.PropertyLens{:x}}, Int64}, Vector{Normal{Float64}}, Vector{AbstractPPL.VarName{:a, Accessors.PropertyLens{:x}}}, Vector{Float64}}(Dict(a.x => 1), [a.x], UnitRange{Int64}[1:1], [0.9872125141088967], Normal{Float64}[Normal{Float64}(μ=0.0, σ=1.0)], [0], Dict{String, BitVector}("del" => [0], "trans" => [0])), b = DynamicPPL.Metadata{Dict{AbstractPPL.VarName{:b, Accessors.PropertyLens{:x}}, Int64}, Vector{Normal{Float64}}, Vector{AbstractPPL.VarName{:b, Accessors.PropertyLens{:x}}}, Vector{Float64}}(Dict(b.x => 1), [b.x], UnitRange{Int64}[1:1], [-0.7010216625255767], Normal{Float64}[Normal{Float64}(μ=0.0, σ=1.0)], [0], Dict{String, BitVector}("del" => [0], "trans" => [0]))), Base.RefValue{Float64}(-2.570887026081012), Base.RefValue{Int64}(0))), DynamicPPL.PrefixContext{AbstractPPL.VarName{:a, typeof(identity)}, DynamicPPL.DefaultContext}(a, DynamicPPL.DefaultContext())))

What even the

[penelopeysm]

Maybe (shock and horror) GibbsContext should be a leaf context

[penelopeysm]

Can't believe I spent 1.5 hours figuring out a 'bug', where the bug was that I had made the right fix for tilde_assume(::GibbsContext, ...) but didn't apply it to tilde_assume(rng, ::GibbsContext, ...). Fun. Can't we cut this code duplication?

[penelopeysm]

The code duplication is gone in #2676 (prompted by upstream changes to DPPL's tilde pipeline). Rest of the things should probably be separate issues, but eh.