Invalid test case for `Gibbs`?

[torfjelde]

I'm currently updating Turing.jl to be compatible with the new version of DynamicPPL.jl and ran into something that seems like a bug.

Currently, we implement the logdensity-evaluation required for MH here:

Turing.jl/src/inference/mh.jl

Lines 251 to 263 in d1d4071

	function (f::MHLogDensityFunction)(x)
	sampler = f.sampler
	vi = f.vi
	x_old, lj_old = vi[sampler], getlogp(vi)
	set_namedtuple!(vi, x)
	f.model(vi)
	lj = getlogp(vi)
	vi[sampler] = x_old
	setlogp!(vi, lj_old)
	return lj
	end

But now that we have a clear separation between sampling (SamplingContext) and evaluation (DefaultContext), this seems like it should now be:

function (f::MHLogDensityFunction)(x)
    sampler = f.sampler
    vi = f.vi

    x_old, lj_old = vi[sampler], getlogp(vi)
    set_namedtuple!(vi, x)
    f.model(vi, DefaultContext()) # CHANGED
    lj = getlogp(vi)
    vi[sampler] = x_old
    setlogp!(vi, lj_old)

    return lj
end

To ensure that we're only evaluating, not sampling.

But making this change results in the following test-case to fail:

Turing.jl/test/inference/gibbs.jl

Lines 91 to 111 in d1d4071

	@model imm(y, alpha, ::Type{M}=Vector{Float64}) where {M} = begin
	N = length(y)
	rpm = DirichletProcess(alpha)
	z = tzeros(Int, N)
	cluster_counts = tzeros(Int, N)
	fill!(cluster_counts, 0)
	for i in 1:N
	z[i] ~ ChineseRestaurantProcess(rpm, cluster_counts)
	cluster_counts[z[i]] += 1
	end
	Kmax = findlast(!iszero, cluster_counts)
	m = M(undef, Kmax)
	for k = 1:Kmax
	m[k] ~ Normal(1.0, 1.0)
	end
	end
	model = imm(randn(100), 1.0);
	sample(model, Gibbs(MH(:z), HMC(0.01, 4, :m)), 100);

It complains that a component of m is missing. The issue is that once z has been sampled, the number of components has changed and so the size of m has changed. Currently this test will pass because MH will just silently sample this component from the prior when evaluating the logpdf of the proposed state.

It's unclear whether:

1. This was intended in the first place.
2. If it's even valid to do.

[cpfiffer]

Uh yeah that seems quite wrong. The line you added

f.model(vi, DefaultContext()) # CHANGED

seems correct to me and it should be merged in.

As to the failing test, I am not sure -- I don't recall working with this model but I would expect this behavior to be supported.

[torfjelde]

@yebai Mentioned that it might "accidentally" end up doing the correct thing, but even so we probably should do this in a separate/explicit way rather than implicit like currently done.

[yebai]

Closed in favour of #1904