add a DistributionLogPotential

[miguelbiron]

This PR a follow-up after the comments received on #99. It simply aims to have a container for Distributions objects that allow us to use these as references for Pigeons. This can be achieved by minimally affecting the current codebase -- basically, it only requires modifying the default behavior of initialization methods when constructing Replicas.

To see example usage, look at the test file.

[codecov-commenter]

Codecov Report

Merging #102 (05bb2e8) into main (49c0560) will increase coverage by 0.70%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #102      +/-   ##
==========================================
+ Coverage   84.28%   84.99%   +0.70%     
==========================================
  Files          79       80       +1     
  Lines        2214     2232      +18     
==========================================
+ Hits         1866     1897      +31     
+ Misses        348      335      -13     

Files Changed	Coverage Δ
src/Pigeons.jl	100.00% <ø> (ø)
src/targets/target.jl	84.61% <ø> (+7.69%)	⬆️
src/pt/Inputs.jl	100.00% <100.00%> (ø)
src/references/DistributionReference.jl	100.00% <100.00%> (ø)
src/replicas/replicas.jl	79.31% <100.00%> (+4.31%)	⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[miguelbiron]

Using some inner constructor magic, I was able to simplify the interface to

        pt = pigeons(
            target    = (x -> logpdf(Normal(3,1), x[begin])),
            reference = Normal(-3,1),
            ...
        )

which should feel much nicer compared to the previous one which required the user to wrap the reference

        pt = pigeons(
            target    = (x -> logpdf(Normal(3,1), x[begin])),
            reference = DistributionReference(Normal(-3,1)),
            ...
        )

This does complicate maintaining the code for Inputs. At the same time, I think that making the API nicer will necessarily involve having more complex entry points to pigeons -- and Inputs is perhaps the most relevant point of entry. Still, I'm not too sure if having this simplification is worth the extra code or not. Thoughts @alexandrebouchard ?

[alexandrebouchard]

Thanks Miguel to keep pushing this, I think it is important indeed.

The part of the PR about the new initialization() dispatch I definitely like. Minor comment on this aspect is that DistributionReference should be renamed DistributionLogPotential or something like that to (1) mirror similar structs, e.g. TuringLogPotential, as I think Nikola mentioned earlier (2) strictly speaking we may want to allow that wrapper for targets as well (* maybe? not 100% sure if the Distributions stuff might get used for intractable distributions?).

About the other part of the PR, i.e the "syntactic sugar" to avoid having to write the name of the wrapper: I am undecided. On one hand I agree this is useful to simplify frequently-used syntax. So for the user-facing behaviour I like it too. But the internal implementation I am not a huge fan of. One concern is if we add auto-wrapping behaviour for one type of LogPotential, I think we want to have a mechanism that would scale to doing the same for other LogPotentials. I can see at least one similar example: to skip the wrapper around Turing models. And to allow doing so both in the target slot and in the reference slot potentially (see * above, less sure about this second point). The proposed internal implementation might get even messier in that case (feel free to convince me otherwise).

Can we quickly explore if there might not be alternatives for implementing the wrapping of Inputs argument(s)?

One idea: can we use the args... syntax instead? In the worst case, maybe the wrapping could be done in pigeons(...) function instead of doing it as inner constructor (I do see the benefit of doing it in Inputs to keep the symmetry between Inputs and pigeons(..)). I.e. see if has the key :target and/or :reference are ther, pass its contents to a function like wrap_log_potential() or something like that where the dispatch are now on a single argument; then reassign the output to the dictionary.

Another route would be to search for all .reference, replace by a function call and do the wrapping there, just in time basically (i.e. store the Distributions.jl stuff in Inputs, translate it just when it is needed).

I wonder also if Julia's built-in but extensible convert() stuff can be useful here?

[miguelbiron]

But the internal implementation I am not a huge fan of. One concern is if we add auto-wrapping behaviour for one type of LogPotential, I think we want to have a mechanism that would scale to doing the same for other LogPotentials.

100%. I was on the verge here too. Happy to roll back. Let's keep this PR to the DistributionLogPotential. Perhaps the wrapping part can be done as part of a more ambitious BreadCrumbs PR. Which we should discuss over 🍻.

[alexandrebouchard]

Looks good to me!

[alexandrebouchard]

Thanks Miguel!!