[WIP] PR for issue #634

[trappmartin]

This is a work in progress PR refactoring the assume and observe interface (#634). Do not merge!

---

Todo:

• Define abstract runner interface.
• Define runner for particle based inference.
• Define runner to evaluate logjoint(m::Model, v::VarInfo).
• Define runner to evaluate logpdf(m::Model, v::VarInfo).
• Separate compiler and other core parts from SampleFromPrior.
• Use runners inside of samplers instead of overloading assume and observe.
• [ ] Fix Vectorisation issues: Assume in vectorisation of HMC bug. #760 Observe vectorisation issue. #761 .

---

[yebai]

Why do we call setgid! only if assume is called for a single distribution during HMC sampling.
Why do we increase vi.num_produce on observe only for a single distribution and not during vectorisation? Is this a bug?

These are likely bugs in the vectorisation implementation (related #476).

We sometimes increase the logp of VarInfo using acclogp!(vi, ...) and sometimes not. Is there a rational behind this?

I'm not sure I understand the issue here. Can you clarify a bit?

I'd prefer if we have a more flexible and consistent interface for manipulating the lp_ variable. Currently, the user has no chance to manipulate this if needed. I feel something similar to acclogp!(vi, ...) but without the need of passing vi in the form of a macro would be a solution. Any objections?

Have an API for manipulating logp sounds good!

[trappmartin]

We sometimes increase the logp of VarInfo using acclogp!(vi, ...) and sometimes not. Is there a rational behind this?

I'm not sure I understand the issue here. Can you clarify a bit?

We increase log p inside the compiler using: $vi.logp += $lp and additionally increase this values inside the particle Gibbs sampler:

./src/inference/pgibbs.jl:174:        acclogp!(vi, logpdf_with_trans(dist, r, istrans(vi, vn)))

I suspect this is a bug.

[yebai]

Places where we modify logp:

Inside the compiler:

• Turing.jl/src/core/compiler.jl

  Line 52 in 820051e

  $vi.logp += Turing.observe($sampler, $dist, $observation, $vi)

• Turing.jl/src/core/compiler.jl

  Line 104 in 820051e

  $vi.logp += $lp

Inside PG:

• Turing.jl/src/inference/pgibbs.jl

  Line 174 in e585eea

  acclogp!(vi, logpdf_with_trans(dist, r, istrans(vi, vn)))

It does seem L174 in PG is a duplicate of L104 in the compiler. Since PG is not making use of VarInfo.logp in the resampling step, this is not hurting us yet...

[yebai]

UPDATE:

Actually, the following line "cancels" the increment of logp in the compiler (i.e. L104), since it returns 0.

Turing.jl/src/inference/pgibbs.jl

Line 176 in e585eea

return r, zero(Real)

[trappmartin]

It seems it's a very good idea that we refactor that bit of the code base.

[cpfiffer]

Suggested change

	# Compute the log joint Runner. #
	# Compute the particle filtering Runner. #

[mohdibntarek]

Thanks Martin for this PR.

I suggest we keep the vectorization issues out of this, since that's a totally different beast.

About this PR and its underlying issue, while I am not exactly against them, I just don't get the appeal of introducing a plethora of new types to do exactly the same thing we are doing now without these types at all. The abstractions we are introducing here don't add much functional value for now as far as I can see. If anything they are a bit confusing. For example, the relation between SampleFromDistribution, ComputeLogJointDensity and Sampler is a bit grey in my eyes since they all subtype AbstractRunner but are used in very different situations. For instance, why is ComputeLogJointDensity not just a function? Why do I have to do Sampler(ComputeLogJointDensity(), selector) which is unnecessarily complicated and obscure imo. What are we sampling? What are we selecting? Can any AbstractRunner be passed to Sampler as a first argument? Another question is when should we use SampleFromPrior vs nothing when trying to run the model initially. Why was nothing introduced back when we removed it earlier and replaced it with SampleFromPrior or SampleFromUniform everywhere?

I think what we need to do is work from the use-cases backwards. So firstly, we specify the API functions that we want to work, e.g. rand(::Sampler(::Model, ::InferenceAlgorithm)) then we write minimal code to define these functions. There is no need to introduce a new type if it doesn't add functional value via dispatch or data organization. There might be a philosophical appeal to abstracting everything and introducing a new type for everything, but that's just code that needs to be maintained later so simple is usually better.

I may be just failing to see the value of this re-structuring so please explain it to me if I got it all wrong. Sorry for the late night rant :)

[yebai]

About this PR and its underlying issue, while I am not exactly against them, I just don't get the appeal of introducing a plethora of new types to do exactly the same thing we are doing now without these types at all.

This is meant to avoid direct dispatch assume and observe on Sampler types, and implement a standard set of APIs (e.g. logpdf, rand). The current code bases contain redundant assume and observe implementations, which are largely unnecessary. We can keep modifying the signature, e.g.

assume(spl::{HMC, NUTS, SGLD}, ...)

But this looks a bit ugly and doesn't support plug-and-play inference. Introducing an intermediate type such as Runner allows samplers to share the same set of assume and observe implementations for many use cases.

For instance, why is ComputeLogJointDensity not just a function?

This is meant to reduce the number of functions the Turing compiler has to generate. By lowering the model into IRs and allow users to overload assume and observe, we save a lot of compiler transformations. I like to keep the compiler as simple as possible (though it's already quite complex) and do most stuff via standard Julia features such as multi-dispatching. I think this is beneficial in the longer term.

[xukai92]

It seems that the issue is not introducing these new runner types, but the fact these types are currently used to construct Sampler are not very intuitive. Not sure how to change this though.

[trappmartin]

I agree. I'll create a new PR in which I will try to find a solution that doesn't require to construct Sampler.

[xukai92]

What's the plan for this issue?

[trappmartin]

Once the inference changeover is merged I’ll work on a new PR that contains the main features of this PR.

[yebai]

I suspect that a lot of features planned in this PR is now added in #965, e.g. the Runner type is replaced by a similar type Context. I propose we close this PR for now, and move useful code contained in this PR to a new PR if needed.