Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experimental: switch to DensityInterface #87

Closed
wants to merge 2 commits into from

Conversation

phipsgabler
Copy link

To summarize:

  • Some projects, like Turing, would depend on both DensityInterface and LogDensityProblems, because both provide functionality they require (cf. Use LogDensityProblems instead of gradient_logp TuringLang/Turing.jl#1877).
  • On a very high level, LDP provides functionality extending DI: nice handling of AD with various backends, and transformations via TransformVariables.
  • On a lower level, their interfaces differ slightly but significantly in names and expectations, leading to redundancies in packages like Turing (such as having both logdensity and logdensityof...).

There has already been talk of making DI a "parent package" of LDP, but the idea was rejected as DI is rather new and wasn't really well established back then.

I also don't feel comfortable messing radically with the interface of an existing package, deviating from the original considerations of the package author and surprising the existing user base, both of which make me hesitate. On the other hand, the situation as it has arisen in Turing is a bit unfortunate (even if not to the outside), and in the end, both packages are very close in what they want to cover.

Therfore, I'm opening this PR for the sake of discussion. The code as is passes all tests, but is not yet a serious proposal; you'll note that the interface is incompatibly changed in parts, and I cut some corners.

CC @oschulz @devmotion @cscherrer


An attempt to summarize the differences:

  • DI grew out of PPL needs, with the constraint that it should work for different kinds of objects and be flexible wrt. more or less strict semantics (hence all the discussions about DensityKinds and base measures; care was taken that everything can also be made to work in MeasureTheory.jl in a sensible way).
  • DI was also created as, and should remain, an extremely light interface package.
  • LDP incorporates an extensible and mature system to connect simple density calculation with gradients; this is strictly more than what DI does.
  • However, LDP's interface for AD and the actual AD glue could be separated.
  • LDP also provides an integration of densities and TransformVariables. This is somewhat orthogonal to the rest; there are other change-of-variables packages out there.
  • LDP starts from regular callable objects that are densities, and extends them with transformation and AD wrappers. DI, on the other hand, always assumes special objects that are either HasDensity or IsDensity, and which do not have to be callable (evaluation is always via logdensityof). (But DI provides logfuncdensity to wrap any callable object and treat it as a density). Perhaps IsDensity objects could actually expected to be callable... but this is nowhere said so or agreed upon.

@devmotion
Copy link
Collaborator

devmotion commented Aug 23, 2022

Basically copied from #78 (comment):

I think a bit less intrusive and breaking would be to start with just supporting the DensityInterface API on top but keeping logdensity and logdensity_and_gradient as they are without any deprecations. I.e., just adding logdensityof and DensityKind. Similar to how Distributions supports DensityInterface without removing or changing its main API.

I also think one should refrain from making additional changes unrelated to supporting DensityInterface such as adding StaticNumbers. I'm not convinced yet that this is a good idea since most functionality is not needed, and my experience with Static.jl leads me to believe that these kinds of packages are prone to causing a lot of invalidations.

@tpapp
Copy link
Owner

tpapp commented Aug 23, 2022

I am also reluctant to break the API for this. I would very much prefer a lightweight wrapper, ie take something that this package can work with and wrap in it a struct that supports DensityInterface. It could live in its little glue package, depending on LogDensityProblems and DensityInterface.

As I said in the other discussion: I realize that this package could be made leaner by removing AD glue code. That will happen once AbstractDifferentiation matures a bit more.

@oschulz
Copy link

oschulz commented Aug 24, 2022

refrain from making additional changes [...] my experience with Static.jl leads me to believe that these kinds of packages are prone to causing a lot of invalidations

I agree - though in fairness one should mention that the Static.jl devs have made major changes recently to reduce the number of invalidations.

@phipsgabler
Copy link
Author

Oh, sorry, please don't worry about the StaticNumbers stuff, that was just something I tried out when I tried to get capabilities closer to the trait style DensityInterface uses. Can be ignored for this discussion.

So @tpapp what you're talking about would be something like an approach with a new package (say, DensityInterfaceAD), just reusing LDP's wrappers and glue code and sticking it onto a new IsDensity object:

ℓ_ = DensityInterface.logfuncdensity(problem)

# wraps an ADGradient(:ReverseDiff, LDPWrapper(ℓ)) = DensityInterfaceAD.ADLogDensity(:ReverseDiff, ℓ) 

DensityInterface.logdensityof(ℓ, x) == LDP.logdensity(ℓ.wrapped, x)

DensityInterfaceAD.logdensity_and_gradient_of(ℓ, x) == LDP.logdensity_and_gradient(ℓ.wrapped, x) # or logdensityof_with_gradient?

DensityInterface.DensityKind(ℓ) == DensityInterface.IsDensity()

and everything would be completely independent, right?

(Although maybe it would still be nice to add

DensityInterface.logdensityof(::TransformedDensity, ::Any)
DensityInterface.logdensityof(::ADGradientWrapper, ::Any)

)

@phipsgabler
Copy link
Author

phipsgabler commented Aug 24, 2022

BTW, do I understand right that TransformedLogDensity could be easily generalized to ChangesOfVariables, except for the need to also define dimension somewhere? Could that be done in a @requires, like for the AD backends? Superseded by #88.

(And then probably the hypothetical DensityInterfaceAD spoken of above should also have ChangesOfVariables support somewhere.)

@tpapp
Copy link
Owner

tpapp commented Aug 25, 2022

@phipsgabler: yes, I think that's the cleanest approach. I just introduced such a mini-package in #89 for another purpose.

@phipsgabler
Copy link
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants