observe and assume

[trappmartin]

I think the following points regarding the current code might be problematic and changing them could improve our codebase.

1. Assume and observe dispatch on the sampler type (which is fine) but also on Nothing. This forces us to pass nothing to the functions if no sampler is defined and have a Union{Nothing, Sampler} for the model functions. I think we can circumvent this by having different function signatures, one with and one without the sampler argument. Or do I miss something here?

2. Currently assume and observe do in-place operations and not in-place operations at the same time More precisely, they change the VarInfo variable and also return values that are used later on. This is inconsistent and might lead to strange behaviour. I would prefer to make this consistent and ensure that all operations are either in-place or not. I feel the mixed behaviour is not very clean. See for example:

   Turing.jl/src/samplers/sampler.jl

   Line 93 in 7cb84a5

   vi.num_produce += 1

   and

   Turing.jl/src/samplers/sampler.jl

   Line 42 in 7cb84a5

   push!(vi, vn, r, dist, 0)

   
   Or is there a reason why cannot do all operations in-place?

cc: @yebai

[xukai92]

Assume and observe dispatch on the sampler type (which is fine) but also on Nothing. This forces us to pass nothing to the functions if no sampler is defined and have a Union{Nothing, Sampler} for the model functions. I think we can circumvent this by having different function signatures, one with and one without the sampler argument. Or do I miss something here?

I think this should work. Actually, when assume is used for initialisation, the spl is not used at all. Maybe we can have a signature where there is not spl in the argument, and call that function when spl is nothing.

Or is there a reason why cannot do all operations in-place?

This in-place + non in-place signature was introduced when we moved to ReverseDiff, for which initialising a _lp in the @model scope and accumulating that in the main loop instead of doing in-place accumulation in VarInfo gives huge performance gain, probably because of ReverseDiff's pre-compilation of the code (see discussion here JuliaDiff/ReverseDiff.jl#102)

[willtebbutt]

Currently assume and observe do in-place operations and not in-place operations at the same time More precisely, they change the VarInfo variable and also return values that are used later on. This is inconsistent and might lead to strange behaviour. I would prefer to make this consistent and ensure that all operations are either in-place or not. I feel the mixed behaviour is not very clean. See for example:

I don't know how straightforward it will be to move towards immutability using the current VarInfo design, which is entirely formulated entirely around mutation. I do not believe that the current mutating design is a good choice for a number of reasons, however, it will be a substantial amount of work to move away from it. Consequently, the only viable resolution to your concern at the minute is to do everything via mutation. On the other hand, how much of an issue actually is this? I'm not aware of any conventions that advocate for avoiding it, although I do agree that it's not the most obvious design.

Also bear in mind that we're not getting the performance gains anymore as we're not using ReverseDiff (not that we should be relying on our AD to achieve good performance).

[yebai]

Assume and observe dispatch on the sampler type (which is fine) but also on Nothing. This forces us to pass nothing to the functions if no sampler is defined and have a Union{Nothing, Sampler} for the model functions. I think we can circumvent this by having different function signatures, one with and one without the sampler argument. Or do I miss something here?

This is fixed by #706

Currently assume and observe do in-place operations and not in-place operations at the same time More precisely, they change the VarInfo variable and also return values that are used later on. This is inconsistent and might lead to strange behaviour. I would prefer to make this consistent and ensure that all operations are either in-place or not. I feel mixed behaviour is not very clean. See for example:

This is still not fixed. Maybe we can address this after #750

[yebai]

Closed by #995