Update to tilde overloads in mh.jl

[torfjelde]

@penelopeysm I was looking at your PR #2341 and found some bad bugs in the existing codebase for the MH, so I figured we should just get these fixed too.

[codecov]

Codecov Report

Attention: Patch coverage is 33.33333% with 6 lines in your changes missing coverage. Please review.

Project coverage is 84.36%. Comparing base (452d0d0) to head (679989a).
Report is 1 commits behind head on ch.

Files with missing lines	Patch %	Lines
src/mcmc/mh.jl	33.33%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##               ch    #2360      +/-   ##
==========================================
+ Coverage   83.86%   84.36%   +0.50%     
==========================================
  Files          24       24              
  Lines        1580     1573       -7     
==========================================
+ Hits         1325     1327       +2     
+ Misses        255      246       -9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[penelopeysm]

It occurs to me that the value generated by DynamicPPL.assume(..., SampleFromPrior(), ...) is only used if a proposal isn't specified – if we have one then is this doing extra computation?

I don't know if there's an easy fix for this at all though.

[torfjelde]

It occurs to me that the value generated by DynamicPPL.assume(..., SampleFromPrior(), ...) is only used if a proposal isn't specified – if we have one then is this doing extra computation?

SampleFromPrior just extracts the value from vi if it's "not to be sampled", so it shouldn't be doing anything extra here AFAIK. Can you maybe specify a bit what you mean here?

[penelopeysm]

Oh, I see, I'm just confused. The values are stored in varinfo from the previous iteration when calculating the lp and so it just extracts those values. I had assumed it would call rand() on the prior, but now I observe that that's not the case ;)

edit; oh, I see it's this function that's doing it.

Turing.jl/src/mcmc/mh.jl

Lines 267 to 282 in 40a0d84

	function LogDensityProblems.logdensity(f::MHLogDensityFunction, x::NamedTuple)
	# TODO: Make this work with immutable `f.varinfo` too.
	sampler = DynamicPPL.getsampler(f)
	vi = f.varinfo
	x_old, lj_old = vi[sampler], getlogp(vi)
	set_namedtuple!(vi, x)
	vi_new = last(DynamicPPL.evaluate!!(f.model, vi, DynamicPPL.getcontext(f)))
	lj = getlogp(vi_new)
	# Reset old `vi`.
	setindex!!(vi, x_old, sampler)
	setlogp!!(vi, lj_old)
	return lj
	end

[penelopeysm]

Sorry, not trying to be annoying and get a final word in, but having spent a good amount of time figuring out the interplay between Turing and AdvancedMH I feel like this behaviour is a bit unexpected (and I actually now understand why it happens 😂):

@model function gdemo(x, y)
    s² ~ InverseGamma(2, 3)
    m ~ Normal(0, sqrt(s²))
    x ~ Normal(m, sqrt(s²))
    y ~ Normal(m, sqrt(s²))
end
chain = sample(
    gdemo(1.5, 2.0),
    MH(:m => AdvancedMH.RandomWalkProposal(Normal(0, 0.25))),
    10
)

Here s² is sampled once at the start of the chain and it's never touched again. I feel like people (perhaps just me 😄) would expect that it either defaults to a static proposal (the prior) or that it errors. If you agree I can open a separate issue :) this is obviously not part of this PR

[torfjelde]

Very much agree with you that this is very confusing 😕 IMO we should be erroring if we some variables don't have a Gibbs sampler assigned to them

[mhauru]

Maybe worth checking if there is a similar issue with the new Gibbs sampler? If not, #2328 might fix this by accident. EDIT: Never mind, I hadn't understood the problem, #2328 does not change this.