Fixed externalsampler

[torfjelde]

We recently made some improvements to the construction of the chains (#2071), and in the process seem to have broken the externalsampler (though I'm somewhat confused as to how tests were passing).

[github-actions]

Pull Request Test Coverage Report for Build 6454918003

• 0 of 5 (0.0%) changed or added relevant lines in 2 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage remained the same at 0.0%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/mcmc/Inference.jl	0	2	0.0%
src/mcmc/abstractmcmc.jl	0	3	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/mcmc/Inference.jl	1	0.0%

Totals
Change from base Build 6441878489:	0.0%
Covered Lines:	0
Relevant Lines:	1451

---

💛 - Coveralls

[codecov]

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (4ffe2cd) 0.00% compared to head (0d9fdfc) 0.00%.

Additional details and impacted files

@@          Coverage Diff           @@
##           master   #2089   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          21      21           
  Lines        1451    1451           
======================================
  Misses       1451    1451           

Files	Coverage Δ
src/mcmc/Inference.jl	0.00% <0.00%> (ø)
src/mcmc/abstractmcmc.jl	0.00% <0.00%> (ø)

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

[devmotion]

According to codecov, both the old and new implementation is not covered.

Related to external samplers,

Turing.jl/src/mcmc/Inference.jl

Line 107 in ed410b1

const ESLogDensityFunction{M<:Model,S<:Sampler{<:ExternalSampler},V<:AbstractVarInfo} = Turing.LogDensityFunction{V,M,<:DynamicPPL.DefaultContext}

seems wrong - the sampler should be part of the right-hand side, shouldn't it? I think it should be changed to

const ESLogDensityFunction{M<:Model,S<:Sampler{<:ExternalSampler},V<:AbstractVarInfo} = Turing.LogDensityFunction{V,M,<:DynamicPPL.SamplingContext{<:S}}

[torfjelde]

According to codecov, both the old and new implementation is not covered.

This is quite strange though, as test/mcmc/abstractmcmc.jl is clearly included in test/runtests.jl:

Turing.jl/test/runtests.jl

Line 79 in 3780ca8

@timeit_include("mcmc/abstractmcmc.jl")

[torfjelde]

seems wrong - the sampler should be part of the right-hand side, shouldn't it? I think it should be changed to

I applied your suggestion a bit too quickly here:) No, the original impl was correct.

externalsampler only works for samplers which requires nothing but the LogDensityProblems interface. The SamplingContext is not necessary here because these samplers will not be overloading any of the tilde statements; they all just need logdensity and/or logdensity_and_gradient, hence why we just use DefaultContext.

[devmotion]

I see. The line still seems wrong to me - since the (external) sampler is not used on the RHS, it means that anything defined for ESLogDensityFunction will in fact be defined for any TuringLogDensityFunction with a DefaultContext, everything defined for ESLogDensityFunction{M<:Model} will be defined for any TuringLogDensityFunction{M} with a DefaultContext, and even ESLogDensityFunction{M<:Model,S<:Sampler{<:ExternalSampler}} will be defined for any TuringLogDensityFunction{M} with a DefaultContext.

[torfjelde]

Ah great point and 100% agree 👍
It seems the only point where it is used is here:

Turing.jl/src/mcmc/Inference.jl

Lines 108 to 116 in f8876d6

	function LogDensityProblems.logdensity(f::ESLogDensityFunction, x::NamedTuple)
	return DynamicPPL.logjoint(f.model, DynamicPPL.unflatten(f.varinfo, x))
	end
	# TODO: make a nicer `set_namedtuple!` and move these functions to DynamicPPL.
	function DynamicPPL.unflatten(vi::TypedVarInfo, θ::NamedTuple)
	set_namedtuple!(deepcopy(vi), θ)
	return vi
	end

which is an unfortunate hack we need to be compatible with AdvancedMH.jl IIRC.

But yeah, we should def just remove ESLogDensityFunction at the very least

[devmotion]

This is type piracy (I know it was already there 😉 - and BTW DynamicPPL.unflatten below is another instance of type piracy). In DynamicPPL, we explicitly decided to not add this overload: TuringLang/DynamicPPL.jl#501 Can we just remove it completely?

[torfjelde]

Yeaaah I'm aware of both and I don't like them 😕

It's done to be compatible with AdvancedMH.jl which allows you to specify the proposal distribution using NamedTuple 😕 So we can't just remove it without messing with this; we'd have to do some more work for that, unfortunately.

[torfjelde]

If you go to that PR, you can see that it was closed it in favour of another commit that did basically the same thing 😅

[devmotion]

Ohh haha I missed that, I just remembered that the issue had been closed 😅

[torfjelde]

You okay with this leaving this for now @devmotion ? This PR is meant to fix existing code, so feel like maybe we should get this merged and maybe make an issue / separate PR?

[devmotion]

Yes, let's address this separately.

[JaimeRZP]

How can I help to get this merged?
We need this to be able to finally merge TuringLang/docs#403

[JaimeRZP]

LGTM!