BreadCrumbs interface: an easier way to feed pigeons

[miguelbiron]

No description provided.

[codecov-commenter]

Codecov Report

Merging #99 (e9bb079) into main (49c0560) will decrease coverage by 0.06%.
The diff coverage is 77.77%.

❗ 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      #99      +/-   ##
==========================================
- Coverage   84.28%   84.22%   -0.06%     
==========================================
  Files          79       80       +1     
  Lines        2214     2232      +18     
==========================================
+ Hits         1866     1880      +14     
- Misses        348      352       +4     

Files Changed	Coverage Δ
src/Pigeons.jl	100.00% <ø> (ø)
src/pt/BreadCrumbs.jl	76.47% <76.47%> (ø)
src/api.jl	100.00% <100.00%> (ø)

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

[nikola-sur]

I think the name BreadCrumbs is super clever and cute, but maybe too cute. Can we name it something more descriptive?

[miguelbiron]

@nikola-sur yeah we can definitely change it. To make it very explicit, we could call it "FunctionDistributionPair" or something like that. It conveys the specific inputs requested.

At the same time, we should think if there are other use cases that could be captured with a similar interface. E.g. instead of a Distribution object, we could specify the reference as a generic logpdf function and a sampler function.

[nikola-sur]

Maybe we call it something like CustomLogPotential to keep in line with TuringLogPotential, Stan, Comrade? Then CustomLogPotential() can accept various forms of input (two or three arguments):

• target logpdf + Distributions object for the reference from which you know how to sample and evaluate
• target logpdf and reference logpdf, defaults to identity kernel with warning
• target logpdf, reference logpdf, reference rand for iid samples

Also, can we compose distributions like Uniform * Uniform, Uniform ^100, etc? Or is that dangerous?

[nikola-sur]

Because Turing implicitly couples a reference and target and that's all contained in TuringLogPotential, maybe we should unify and do CustomLogPotential? That's my line of thought but am curious what others think.

[alexandrebouchard]

Very nice!!! See some thoughts below

[alexandrebouchard]

SliceSampler() is already the global default, no need to have that line.

[alexandrebouchard]

Can you use @auto instead? Otherwise the standard out gets cluttered with pages of mostly useless type information when showing the stack trace.

Also ref should be more general. We don't want Distributions to only work in BreadCrumbs. It should be seamless with the reference = .. method as well. (unless we remove the reference = .. method; which I don't think is a good idea since it works well for PPLs).

Conversely, things that can be fed into reference = .. should work in the target_log_potential argument. Similarly things that gets fed into target = should work in target_log_potential (I think this is already the case?).

[alexandrebouchard]

This seems superfluous?

[alexandrebouchard]

I.e. instead can we write the dispatches on Distributions types directly?

[miguelbiron]

Ah! I think you're right! At least for the reference. I don't think people are thinking of passing the loglikelihood as a Distribution on the data given the parameter. That would also be very restrictive given that Distributions.jl only has simple models.

[alexandrebouchard]

isn't this equivalent to bct::Distributions.UnivariateDistribution? etc for the other arguments

[alexandrebouchard]

Sounds good to export it, but then the PR should contain documentation (in code and in the website).

[alexandrebouchard]

About the naming: since it's subjective I suggest we discuss in person when I come back--or even better, over beers :)

[alexandrebouchard]

Further comments: one idea to get this merged faster is to split into two phases.

First, a non-breaking PR which just adds Distribution.jl support to the existing API, i.e. via reference = ... To achieve this, we would first internally dispatch on a more informative version of initialization() where the first argument is the full path of distributions instead of just the target. This way when the reference is specified with a distributions.jl (or similar situations), we can just use the reference to initialize. But also by default we dispatch to the current version of initialization taking only the target so the change is not too drastic. With this the one liner could be something like that for a truncated Gaussian example:

pigeons(reference = Uniform()^2) do x 
   -0.5 * sum(abs2, x)
end

Then we can discuss further API-changing PRs, e.g. user-facing BreadCrumbs, unifications of LogPotentials, etc.

[alexandrebouchard]

(in the above, we partly use the fact that the type Function already conforms the informal interface log_potential.)

[miguelbiron]

@nikola-sur

Also, can we compose distributions like Uniform * Uniform, Uniform ^100, etc? Or is that dangerous?

Yeah that would be type piracy... But it would be nice to suggest them implement the product and power operator for distributions!

[miguelbiron]

@alexandrebouchard

To achieve this, we would first internally dispatch on a more informative version of initialization() where the first argument is the full path of distributions instead of just the target.

This was precisely the reason why I needed to have special types so that initialization could talk to the reference through the pointer in the target object. But changing initialization seems like a more robust option going forward.

Also: splitting the PR sounds good to me!

[miguelbiron]

@alexandrebouchard unrelated to this PR but I had to change this limit so that the test would pass. Are you ok with lowering this limit?

[miguelbiron]

@alexandrebouchard Since initialization is only used here

Pigeons.jl/src/replicas/replicas.jl

Line 90 in 49c0560

states = [initialization(inputs.target, split_rngs[i], my_global_indices[i]) for i in eachindex(split_rngs)]

It seems like the best option would be to make it accept the full Inputs object -- which already holds the pointer to the reference object -- and then have a default method that dispatches on the target alone. We could then also have methods that dispatch on the type of the reference object or any other thing in the Inputs. This approach would probably break the least stuff.

[alexandrebouchard]

Dispatching on full input obj may be a bit messy BC inputs has several type params, and for maintenance if we change input type params that would be on more thing to change. But not opposed if that's what makes most sense

[miguelbiron]

It seems doable! I'm almost done with a prototype but I have encountered a different difficulty. The issue with using a distribution directly as a reference is that making them callable within Pigeons would constitute type piracy, if I'm not mistaken. From this point of view, it would make sense to wrap the Distribution object inside another one created by us. What do you think?

[miguelbiron]

Ok I expanded on the above idea on a separate PR (#102 ) with a wrapping struct called DistributionReference. We can leave the BreadCrumbs name for a more ambitious revamp of the API.