Make tilde_{assume,observe} functions behave more uniformly

[phipsgabler]

Seems like I'm too late to veto the release, but anyway...

The PR changes the tilde functions used in the generated code to include acclogp!, and always just return the sampled value (instead of sometimes the logp, sometimes the logp and the value). This is in light with the now consistent behaviour of making tildes always return their value, and cleans up the generated code a bit.

It shouldn't make any difference to the outside. The [dot_]tilde_{assume,observe} functions were only added recently in #53 and aren't really part of the public interface -- tilde and dot_tilde work as before.

[codecov]

Codecov Report

Merging #76 into master will increase coverage by 0.03%.
The diff coverage is 85.00%.

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
+ Coverage   77.46%   77.49%   +0.03%     
==========================================
  Files          13       13              
  Lines         821      831      +10     
==========================================
+ Hits          636      644       +8     
- Misses        185      187       +2     

Impacted Files	Coverage Δ
src/context_implementations.jl	57.03% <83.33%> (+2.56%)	⬆️
src/compiler.jl	87.82% <100.00%> (-0.21%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7159cb...dfb4b74. Read the comment docs.

[devmotion]

Hopefully #84 fixes the test errors on master.

[yebai]

This looks good to me. However, it is worth noting some sampling algorithms override these assume and observe functions, e.g.

MH

• https://github.com/TuringLang/Turing.jl/blob/680c81a708ed50841f375cf466dde71c76e70ca2/src/inference/mh.jl#L250
• https://github.com/TuringLang/Turing.jl/blob/680c81a708ed50841f375cf466dde71c76e70ca2/src/inference/mh.jl#L264

IS

• https://github.com/TuringLang/Turing.jl/blob/680c81a708ed50841f375cf466dde71c76e70ca2/src/inference/is.jl#L76
• https://github.com/TuringLang/Turing.jl/blob/680c81a708ed50841f375cf466dde71c76e70ca2/src/inference/is.jl#L81

HMC

• https://github.com/TuringLang/Turing.jl/blob/680c81a708ed50841f375cf466dde71c76e70ca2/src/inference/hmc.jl#L450
• https://github.com/TuringLang/Turing.jl/blob/680c81a708ed50841f375cf466dde71c76e70ca2/src/inference/hmc.jl#L464
• https://github.com/TuringLang/Turing.jl/blob/680c81a708ed50841f375cf466dde71c76e70ca2/src/inference/hmc.jl#L476

Their return values need to be updated similarly too to avoid surprises.

[phipsgabler]

Well, there's a bit of historically evolved convolution here. tilde_assume and tilde_observe fall back to different methods of tilde/dot_tilde, which are distinguished by their arguments. And these methods in turn call the assume and observe methods from the inference algorithm. Some inference algorithms also (or instead) overload methods of tilde directly.

But since tilde is the "most internal" function that inference algorithms ever see, none of them should have to change behaviour, I think. At least until now the tilde_{observe,assume} methods are only an internal wrapper and work backwards compatibly -- or so I would have thought.

Nevertheless, it would be a good idea, IMO, to untangle all this into

• assume, observe, and dot_{assume,observe} to be overloaded and used by inference algorithms, and
• tilde_{assume,observe} and dot_tilde_{assume,observe} for internal usage (unpacking stuff, and unifying the return values).

[yebai]

Nevertheless, it would be a good idea, IMO, to untangle all this into

• assume, observe, and dot_{assume,observe} to be overloaded and used by inference algorithms, and
• tilde_{assume,observe} and dot_tilde_{assume,observe} for internal usage (unpacking stuff, and unifying the return values).

Sounds like a good design to me. Can you also take care of changes on the Turing side?

cc @xukai92 @mohamed82008 @cpfiffer

[yebai]

@phipsgabler It would be useful to add this distinction between {assume,observe} and tilde_{assume,observe} to docstrings.

[phipsgabler]

All the tilde_{observe,assume} methods are documented already. The others have no docstrings, as before -- but that's not my fault :)

I'll make an issue for the refactoring, but can't currently commit to it. There would be stuff to update in Turing as well, then, which makes it more fiddly.