TuringLang / Bijectors.jl

Implementation of normalising flows and constrained random variable transformations
https://turinglang.org/Bijectors.jl/
MIT License
199 stars 32 forks source link

`logpdf` of `UnivariateTransformed` produces `StackOverflowError` #251

Open lgrozinger opened 1 year ago

lgrozinger commented 1 year ago

Taking the logpdf of a MultivariateTransformed works.

julia> td = transformed(MvNormal(zeros(2), ones(2)), PlanarLayer(2))
MultivariateTransformed{DiagNormal, PlanarLayer{Vector{Float64}, Vector{Float64}}}(
dist: DiagNormal(
dim: 2
μ: [0.0, 0.0]
Σ: [1.0 0.0; 0.0 1.0]
)

transform: PlanarLayer(w = [-0.18499400975237437, 0.7807898767400905], u = [-0.8954510142425457, 0.6844620924181255], b = [0.0004760280592709614])
)

julia> logpdf(td, [1, 1])
-3.356783325529313

This is great! All hail the developers! But for a UnivariateTransformed it does not work.

julia> td = transformed(Normal(), flow)
UnivariateTransformed{Normal{Float64}, PlanarLayer{Vector{Float64}, Vector{Float64}}}(
dist: Normal{Float64}(μ=0.0, σ=1.0)
transform: PlanarLayer(w = [0.16965000037825323], u = [-0.7102609218532135], b = [2.6289813900673975])
)

julia> logpdf(td, 1)
ERROR: StackOverflowError:
Stacktrace:
     [1] transform(t::Inverse{PlanarLayer{Vector{Float64}, Vector{Float64}}}, x::Int64)
       @ Bijectors ~/.julia/packages/Bijectors/vKGbw/src/interface.jl:60
     [2] with_logabsdet_jacobian(ib::Inverse{PlanarLayer{Vector{Float64}, Vector{Float64}}}, y::
Int64)                                                                                         
       @ Bijectors ~/.julia/packages/Bijectors/vKGbw/src/interface.jl:177
--- the last 2 lines are repeated 39990 more times ---
 [79983] transform(t::Inverse{PlanarLayer{Vector{Float64}, Vector{Float64}}}, x::Int64)
       @ Bijectors ~/.julia/packages/Bijectors/vKGbw/src/interface.jl:60

I understand that most use cases for this package will want MultivariateTransformed, but I would have expected this to work. Either I'm missing something or this is a bug. Can anyone confirm either way?

I am using Bijectors v0.12.3.

torfjelde commented 1 year ago

All hail the developers!

Much appreciated :pray:

But for a UnivariateTransformed it does not work.

Unfortunately it doesn't work because the PlanarLayer works with vectors, not Real, i.e. this particular pair of distribution and transformation is not compatible. The resulting error message is not particularly informative, but this is due to some useful default implementations.

lgrozinger commented 1 year ago

Unfortunately it doesn't work because the PlanarLayer works with vectors, not Real, i.e. this particular pair of distribution and transformation is not compatible.

Hmmm ok, but I assume you mean that the Bijectors.PlanarLayer works only with vectors, not the mathematical object? Maybe I'm wrong, but the definition in Variational Inference with Normalizing Flows doesn't mention anything about restricting to only vectors?

lgrozinger commented 1 year ago

The resulting error message is not particularly informative, but this is due to some useful default implementations.

If it really cannot be done, perhaps we could return a useful error message when someone tries PlanarLayer(1)?

torfjelde commented 1 year ago

Hmmm ok, but I assume you mean that the Bijectors.PlanarLayer works only with vectors, not the mathematical object?

Yeah, the actual transformation does work with a real IIRC.

If it really cannot be done, perhaps we could return a useful error message when someone tries PlanarLayer(1)?

It can be done, but the current implementation assumes we're working with an array and so we'd need to implement something that works with a Real.

IMO this doesn't quite seem worth it since you can always just do PlanarLayer(1) and then wrap your x::Real as an array, i.e. [x].

That's not sufficient because PlanarLayer(1) means "we're going to work with input AbstractVector of length 1" which is different from working with a Real.

I agree we should error if you try to call PlanarLayer on a Real though; this is definitively something we should add:)

torfjelde commented 1 year ago

So for now, instead of doing transformed(Normal(), ...), just do transformed(MvNormal(zeros(1), LinearAlgebra.I), ...).

lgrozinger commented 1 year ago

Ok thank you for your help.

Perhaps when I have a bit of time I can get to grips with the codebase and propose a fix.

torfjelde commented 1 year ago

That would be very much appreciated @lgrozinger ! Will keep an eye out:)