Road to DensityInterface? about bijectors.jl HOT 11 CLOSED

@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

from bijectors.jl.

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.

from bijectors.jl.

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.

from bijectors.jl.

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.

from bijectors.jl.

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.

from bijectors.jl.

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.

from bijectors.jl.

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?

from bijectors.jl.

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:)

from bijectors.jl.

OK, thanks everyone. For the sake of not mixing DPPL refactoring and Bijectors discussions too much:

• could someone list the concrete steps needing to be taken here or in ChangesOfVariables, and
• @torfjelde could you comment on the linked DPPL issue what changes are required there, especially what to use in the future instead of 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.

from bijectors.jl.

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 #199.

from bijectors.jl.

Great, then I will close this one.

from bijectors.jl.