Interface design

[oschulz]

@devmotion, here my current draft for a very lightweight interface for densities (current master branch with docs at https://oschulz.github.io/DensityInterface.jl/dev/). Do you like this approach? If so, do you think the Turing ecosystem for example, could support it (would have to be move to some more official Julia org on GitHub first, obviously)? And maybe Distributions too?

To avoid conflicts with @tpapp's LogDensityProblems.jl, I've used the function name logdensityof instead of logdensity (see also tpapp/LogDensityProblems.jl#78) - any alternative suggestions for function names?

[devmotion]

Some minor comments:

• I think it would be good to remove the Base.Callable restrictions and just allow arbitrary types
• Usually @inline statements are not needed
• I wonder if it would be sufficient to return Base.Fix1(logdensityof, density) from logfuncdensity
• I wonder if it is actually a good idea to use logdensityof for both logdensityof(density) and logdensityof(density, x) - I think there's a risk that this API is confusing for both users and developers
• Is it actually necessary to have a LogDensityOf struct instead of just Base.Fix1(logdensityof, x)?
• Maybe instead of logfuncdensity one could use a tologdensity function that could be defined as

  function tologdensity(f)
    return if isdensity(f)
        f
    else
        LogFuncDensity(f)
    end
  end

  ? This would allow developers to obtain a logdensityof compatible object without having to handle the special case when the object at hand is already a density?

[oschulz]

I think it would be good to remove the Base.Callable restrictions and just allow arbitrary types

I just wanted to be a bit cautious. I don't mind relaxing that condition, though I've rarely come across callable objects that aren't Base.Callable - but maybe they're not that uncommon?

Usually @inline statements are not needed

Sure, though I thought they can't hurt here, just to make absolutely sure ... :-)

I wonder if it would be sufficient to return Base.Fix1(logdensityof, density) from logfuncdensity

When debugging likelihoods in BAT, I often encountered very hard-to-read types in the stacktrace, so I wanted something with a decent Base.show. Also, I will need to dispatch on the type in packages like ValueShapes to "forward" the varshape of the density to the log-density function, and so on.

The thing is, the person who defines the density wants to implement logdensityof(density, x), but the person using it typically wants to pass a log-density function somewhere, so the want logdensityof(density).

Do you think it would be clearer to use separate function names, like logdensityat(density, x) and logdensityof(density)?

I wonder if it is actually a good idea to use logdensityof for both logdensityof(density) and logdensityof(density, x) - I think there's a risk that this API is confusing for both users and developers Is it actually necessary to have a LogDensityOf struct instead of just Base.Fix1(logdensityof, x)?

I've used this kind of API in BAT.jl for a while now, and I really came to appreciate the ability to just to logdensityof(density). It often comes up during development and debugging, and is also very readable. Also, there's the nice symmetry between logdensityof and logfuncdensity, allowing back-and-forth conversion between a density object and a log-density function.

Maybe instead of logfuncdensity one could use a tologdensity function that could be defined as [...]

I think the density itself shouldn't have log in it's name - it's a density, we just typically want the log value of it in statistics. Though when integrating over a density, for example, we want it's linear value. So it would rather be logtodensity, not tologdensity, to make sure that the argument is using log-scale. That's why I chose logfuncdensity - "give me a density defined by this log-function". So I also think this function should not accept that's already a density.

Speaking of linear density - maybe we should include a lindensityof function (with a default implementation that uses exp) - kinda like in Distributions with logpdf and pdf?

[oschulz]

@devmotion would you be fine with having both logdensityof(density) and logdensityof(density, x)?

[oschulz]

(I'll start the registration for the package, so I can start using it from BAT.jl, but I'll still be open to API changes for v0.2, v0.3, ...)

[oschulz]

You were right, @devmotion :-) . I simplified logfuncdensity to default to Base.Fix1(logdensityof, density) and removed the Base.Callable restriction on logfuncdensity in v0.2 just now.

[oschulz]

@devmotion, would you be fine with the changed interface for inclusion in Distributions (we'd obviously have to move DensityInterface to JuliaStats or so first)?

[devmotion]

Sorry, I just noticed that I forgot to reply to your additional questions in this issue. I like the changes, and also the implementation seems fine (just skimmed through the code). Some minor things could be cleaned a bit (e.g., the precompile statement) or added (e.g. doctests or a test interface, not sure if needed), similar to what we did in InverseFunctions.

I am also fine with the logdensityof(density) syntax even though it seems I had some doubts initially :smile:

[devmotion]

Do you think it would be useful to add something like densityof with a fallback densityof(density, x) = exp(logdensityof(density, x))?

[devmotion]

I'd be fine with using it in Distributions as well. I guess we have to add something like

# BTW maybe we could use `isdensity` instead?
DensityInterface.isdensitytype(::Type{<:Distribution}) = true

# should this be more fine-tuned regarding types of `x`?
# and should `x` always represent a single sample?
# should it be restricted to ContinuousDistribution?
DensityInterface.logdensityof(d::Distribution, x) = logpdf(d, x)

[oschulz]

Do you think it would be useful to add something like densityof with a fallback densityof(density, x) = exp(logdensityof(density, x))

I had been thinking about that too. There are indeed use cases where the user wants the (linear) value of the density, not the logarithmic value, for example when integrating a non-normalized posterior to get the evidence.

I had though of calling it lindensityof or so, to prevent more novice users from using it instead of logdensityof by accident. But not having "lin" in the name would also be fine with me.

But we may want to consider the function name in general. I had a long Zulip chat with @cscherrer (see my last post on #3) which increased my understanding of measure theory. :-) But it also took us a long time to figure out what each other meant in certain respects, and it turned out that our choice of logdensity(of) as the central function name in DensityInterface was the main cause of it.

[oschulz]

I'd be fine with using it in Distributions as well. I guess we have to add something like [...]

Thanks - yes, that's exactly the code I wanted to propose!

Regarding isdensity vs. isdensitytype - do you mean isdensity(density::SomeDensity) or isdensity(::Type{SomeDensity})? I thought it would be better to decide based on the type, so it can be checked if you don't have an object in hand. But maybe isdensity(density::SomeDensity) would be better: I'm currently adding a function get_device() to KernelAbstractions (JuliaGPU/KernelAbstractions.jl#269), and I originally implemented it to decide based on array type. But Valentin Churavy asked me to change it and use the array instance, he pointed out "One of the learnings for me from Adapt.jl is that you don't want to work on types, working on instances is more reliable."

[devmotion]

I mainly thought about shortening the name to isdensity but it would be even better to base it on types instead of instances. I don't see a necessity to work with types here and instances provide you with both values and types. So I agree, it would be good to define it for instances instead (and as recommended also in the Julia docs only for instances and not for instances and types).

[oschulz]

Ok so I'll change it to isdensity(density::DensityType)?

[devmotion]

Yes, I think this would be good 🙂

[oschulz]

Ok, will do!

[tpapp]

I also find working with values (instead of types) easier, especially in the context I imagine this kind of method will be used, eg

function high_level_api(x)
    if isdensity(x)
        do_stuff(x)
    else
        d = gimme_densiy(x)
        do_stuff(d)
     end
end

[oschulz]

Thanks for joining in, @tpapp! One reason why I wanted to have an isdensity was indeed exactly the use case you describe: Supporting situations where the code using the density(-like) object needs to support more than one mechanism to get it's (log-)density value. Would you consider adding something like the above into LogDensityProblems, to support DensityInterface?

I'll change isdensity to using the value instead of the type, and try to have a "final draft" of DensityInterface ready quickly.

[tpapp]

Since

1. it is very hard to design APIs without context,
2. adding features is easier than removing them,

I would wait with adding isdensity until there is an actual application that demands it, and some discussion has taken place about its requirements (eg ability of the compiler to eliminate branches purely based on type) and alternatives.

Also, is a type is not actually a density, I am fine with getting a MethodError in some place and then requiring manual conversion from the caller (eg the opposite of applying gimme_density automagically as above). Just my 2 cents.

Regarding support from LogDensityProblems: the biggest obstacle for me at the moment is painless integration with AD. It would be great to have an abstract interface for that first.

[oschulz]

integration with AD. It would be great to have an abstract interface for that first.

Yes, I've been waiting for AbstractDifferentiation.jl to offer users AD abstraction in BAT.jl and so on, but it doesn't seem to be there yet (i.e. supported by actual AD packages).

[tpapp]

Yes, things take time to mature. Again, unless there is a good reason to move now (a concrete application that needs it) I would just wait with adding isdensity.

[oschulz]

I'd like to change one more thing: Maybe hasdensity would be a better name than isdensity.

As @phipsgabler wrote in TuringLang/AbstractPPL.jl#38, we want "have a generic hypernym covering loglikelihood, logprior, etc., which are all expressible in terms of logdensity of differently conditioned models". And the plan is that DensityInterface.jl will provide that via the function [log]densityof.

But while a likelihood (function) is a density, I think that @cscherrer will argue that from a point of measure theory, a prior/posterior/distribution is not a density, but a measure. But it can be said to have a density, via RD and it's implied base measure. So for densities, [log]densityof(d, x) is supposed to return the (log-)value at x, and for measures and the like we'll define it's semantics to return the (log-)value at x of the density associated to it via it's base measure.

So hasdensity would cover things nicely, I think: A density like a likelihood has a density (itself), and priors/posteriors/distributions/... have a density too.

[devmotion]

I don't mind, I think both are fine and reasonably clear and "correct" (I also don't think it's a math question). I'm fine with hasdensity if people prefer it. Is there a concrete use case for isdensity/hasdensity at all currently? Otherwise I agree with @tpapp that it might be better to not include it in the API and only add it later if it is needed somewhere.

[tpapp]

I would prefer hasdensity, as something can be a lot of other things besides supporting the API for densities.

[oschulz]

Is there a concrete use case for isdensity/hasdensity at all currently?

Yes, I'm using it in BAT to check early if what the users passes is actually density-compatible, helps a lot to generate early and clear error messages.

it might be better to not include it in the API and only add it later

If, as we hope, this API becomes widely used, that might require code changes in multiple packages down the line. I'd prefer to include it right away.

[devmotion]

Adding and exporting a new function is non-breaking and should not require any changes in downstream packages. But if you already use it, probably we should include it :slightly_smiling_face:

[oschulz]

Adding and exporting a new function is non-breaking and should not require any changes in downstream packages.

It would, in this case, if code that wants to use it to check if things have a density needs code that defines a density to implement that function as well. :-)

But if you already use it, probably we should include it

Thanks!

[oschulz]

I forgot to address there points, regarding Distributions.jl integration

@devmotion : DensityInterface.logdensityof(d::Distribution, x) = logpdf(d, x) # should this be more fine-tuned regarding types of `x`?

I would say we'll see what happens, but my intuition would be that the above is sufficient.

# and should `x` always represent a single sample?

I would strongly argue yes! If people want to use multiple samples, they should broadcast. Supporting both single and multiple samples in the same function just complicates implementations and invites dispatch ambiguities.

# should it be restricted to ContinuousDistribution?

I think we can support discrete distributions as well in a clean way, see @mschauer's question in #4.

[oschulz]

Closed via #4.