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 😄

[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 🙂