Closed phipsgabler closed 2 years ago
@torfjelde correct me if I'm wrong: I don't think anything needs to be done in Bijectors.jl regarding DensityInterface.jl.
Instead, we need to support InverseFunctions.jl and ChangesOfVariables.jl in Bijectors: #199
I agree, this was my impression as well. Most issues should be mentioned in the linked issue and the issue in ChangesOfVariables. Eg, Bijectors allows multiple samples implicitly whereas ChangesOfVariables makes application to multiple samples explicit with map
or broadcast
.
(BTW @oschulz I just merged and released the ChainRulesCore fix which blocked the CRTestutils PR which blocks the ChangesOfVariables PR - so hopefully the test errors are fixed soon 🙂)
It turns out that most of DynamicPPL's dependency on Distributions is really through logpdf_with_trans
Many samplers (such as eg AdvancedMH or EllipticalSliceSampling) have explicit dependencies on Distributions as well, so some additional work is needed even if DynamicPPL and Bijectors support other implementations of DensityInterface.
Regarding AdvancedMH and EllipticalSliceSampling I guess the dependencies or more internal (proposal distributions and their tuning and so on) and not related to that target functions they'll accept?
I guess AdvancedMH could easily be extended to support anything that implements DensityInterface, in addition AdvancedMH.DensityModel
?
AdvancedMH.DensityModel
is actually pretty much exactly what DensityInterface.LogFuncDensity
now provides, so AdvancedMH.DensityModel
could just be deprecated/removed in future versions of AdvancedMH.
Maybe AbstractMCMC.AbstractModel
could even go? Bit off topic-here, though.
OK so that means it's basically allowed to assume that logpdf_with_trans
is already compatible with non-Distribution density objects implementing DensityInterface, which additionally provide the necessary transformation routines?
If I got this right:
logpdf_with_trans(d, x, false)
== logdensityof(d, x)
== with_logabdet_jacobian(d, x)[1]
logpdf_with_trans(d, x, true)
== logdensityof(transformed(d), x)
== with_logabsdet_jacobian(transformed(d), x)[1]
== logdensityof(d, x) - with_logabsdet_jacobian(bijector(d), x)[2]
? (Please check my undestanding here -- as I have said I'm new to this corner...) How will this look in practice -- can there be some defaulting? Must DPPL change from programming agains logpdf_with_trans
to with_logabsdet_jacobian
?
Many samplers (such as eg AdvancedMH or EllipticalSliceSampling) have explicit dependencies on Distributions as well, so some additional work is needed even if DynamicPPL and Bijectors support other implementations of DensityInterface.
Yeah, right, I was only referring to the interface part in DPPL.
No, I think
logpdf_with_trans(d, x, false)
== logdensityof(d, x)
logpdf_with_trans(d, x, true)
== logdensityof(transformed(d), x)
== logdensityof(d, x) - with_logabsdet_jacobian(bijector(d), x)[2]
would be correct, but the wouldn't be a with_logabdet_jacobian(d, x)
or with_logabsdet_jacobian(transformed(d), x)
in there. with_logabsdet_jacobian
operates on transformation functions, not on distributions/densities/measures.
I'm not an expert on the Turing ecosystem at all, but the interface of logpdf_with_trans
strikes me as suboptimal - it seems prone to type instability and AD-overhead due to the conditional statements necessary to handle the transform::Bool
argument.
Must DPPL change from programming agains logpdf_with_trans to with_logabsdet_jacobian
I expect Bijectors would continue to provide logpdf_with_trans
without any changes. As for using it in DynamicPPL: I'm not an expert on the Turing ecosystem at all, but the interface of logpdf_with_trans
strikes me as suboptimal - it seems prone to type instability and AD-overhead due to the conditional statements necessary to handle the transform::Bool
argument.
I don't know DPPL well enough to understand how much it will needs to depend on Bijectors itself in the long run, now that we have ChangesOfVariables, InverseFunctions and DensityInterface. It would certainly be nice if the models created with DPPL themselves would support DensityInterface, of course.
That'd be the plan, yes. I think @torfjelde has plans to change DPPL from "internal" automatic transformations to make them explicit and external, right?
I expect Bijectors would continue to provide logpdf_with_trans without any changes. As for using it in DynamicPPL: I'm not an expert on the Turing ecosystem at all, but the interface of logpdf_with_trans strikes me as suboptimal - it seems prone to type instability and AD-overhead due to the conditional statements necessary to handle the transform::Bool argument.
Yup. It's left-over from an older code-base that we haven't gotten to deprecating properly yet (exactly because it requires a lot of changes upstream).
That'd be the plan, yes. I think @torfjelde has plans to change DPPL from "internal" automatic transformations to make them explicit and external, right?
Not by default, but allowing this, yes:)
OK, thanks everyone. For the sake of not mixing DPPL refactoring and Bijectors discussions too much:
logpdf_with_trans
if it's "almost deprecated", ensuring DensityInterface generality?I'm also ready to get involved in PRs here if that speeds things up, someone just needs to tell me what to do.
could someone list the concrete steps needing to be taken here or in ChangesOfVariables, and
I think this is discussed and possibly can be concretized in https://github.com/TuringLang/Bijectors.jl/issues/199.
Great, then I will close this one.
It turns out that most of DynamicPPL's dependency on Distributions is really through
logpdf_with_trans
, see https://github.com/TuringLang/DynamicPPL.jl/pull/342.Not knowing much about Bijectors: what needs to be done to make the change to things which are generic in DensityInterface? Is a
logdensity_with_trans
something feasible? Or must th "interface merge" happen at the level of the new ChangeOfVariables package?CC @oschulz @torfjelde @devmotion