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

Bijector not defined for FiniteGP #113

Closed
ElOceanografo opened this issue Jun 19, 2020 · 13 comments · Fixed by #124
Closed

Bijector not defined for FiniteGP #113

ElOceanografo opened this issue Jun 19, 2020 · 13 comments · Fixed by #124

Comments

@ElOceanografo
Copy link
Contributor

I found this when trying to run the Turing integration example. When I attempted to sample from the model (line 78), the error below is produced.

Not sure if this belongs here or in Bijectors...I can open an issue there if that's more appropriate.

MethodError: no method matching bijector(::Stheno.FiniteGP{GP{Stheno.ZeroMean{Float64},Stheno.Scaled{Array{Float64,1},Stheno.Stretched{Array{Float64,1},EQ,typeof(identity)},typeof(identity)}},ColVecs{Float64,Array{Float64,2}},LinearAlgebra.Diagonal{Float64,FillArrays.Fill{Float64,1,Tuple{Base.OneTo{Int64}}}}})
Closest candidates are:
  bijector(!Matched::KSOneSided) at C:\Users\sam.urmy\.julia\packages\Bijectors\bHaf6\src\transformed_distribution.jl:58
  bijector(!Matched::Product{Discrete,T,V} where V<:AbstractArray{T,1} where T<:Distribution{Univariate,Discrete}) at C:\Users\sam.urmy\.julia\packages\Bijectors\bHaf6\src\transformed_distribution.jl:39
  bijector(!Matched::Product{Continuous,T,Tdists} where Tdists<:(FillArrays.Fill{T,1,Axes} where Axes) where T<:Distribution{Univariate,Continuous}) at C:\Users\sam.urmy\.julia\packages\Bijectors\bHaf6\src\compat\distributionsad.jl:16
  ...
in top-level scope at untitled:78
in sample at Turing\GMBTf\src\inference\Inference.jl:153
in #sample#1 at Turing\GMBTf\src\inference\Inference.jl:153 
in sample at Turing\GMBTf\src\inference\Inference.jl:163 
in #sample#2 at Turing\GMBTf\src\inference\Inference.jl:163
in Sampler at Turing\GMBTf\src\inference\hmc.jl:376 
in DynamicPPL.Sampler at Turing\GMBTf\src\inference\hmc.jl:384
in Turing.Inference.HMCState at Turing\GMBTf\src\inference\hmc.jl:609
in #HMCState#58 at Turing\GMBTf\src\inference\hmc.jl:612
in link! at DynamicPPL\9OFG0\src\varinfo.jl:697 
in _link! at DynamicPPL\9OFG0\src\varinfo.jl:700
in macro expansion at DynamicPPL\9OFG0\src\varinfo.jl:709 
in link at Bijectors\bHaf6\src\Bijectors.jl:118
@ElOceanografo
Copy link
Contributor Author

For reference, running the following (copied from here and here) before running the Turing model fixes the error:

Bijectors.bijector(d::Stheno.FiniteGP) = Identity{1}()
for T in (:VectorOfMultivariate, :FillVectorOfMultivariate)
    @eval begin
        Bijectors.bijector(d::Bijectors.$T{Continuous, <:Stheno.FiniteGP}) = Identity{2}()
    end
end

@willtebbutt
Copy link
Member

Thanks for opening this. I'll try and take a look soon.

@nucklass
Copy link

Ran into a similar problem trying to use a logit link and bernoulli likelihood function. Is the above still the best solution to this?

@willtebbutt
Copy link
Member

Hmm I guess.

@torfjelde do you know what's going on here?

@torfjelde
Copy link

torfjelde commented Aug 11, 2020

Yeah, I'd say so. Not too long ago we removed the "default"-implementations, which was to just use Identity if we didn't recognize the distribution. Unfortunately that lead to a lot of silent bugs, hence we now require explicit implementation of bijector. And the above solution by @ElOceanografo is good:)

EDIT: Ideally that should just be made part of the package implementing the distribution, but we're probably not quite there yet for Bijectors.jl.

@willtebbutt
Copy link
Member

Ah I see. @torfjelde what's the correct place to solve this? Bijectors has quite a lot of deps that I'm not keen to take on in Stheno, so I would prefer not to have to use it here just to solve Turing integration.

@torfjelde
Copy link

Well, I guess that kind means that we haven't done our job correctly, haha. The idea was for Bijectors.jl to be lightweight so that anyone who's using Distributions.jl wouldn't have any issues also depending on Bijectors.jl.

Are there any particular dependencies that makes this an issue?

With the exception of Roots, NNlib and MappedArrays I think we have essentially the same deps as Distributions.jl, no?

@willtebbutt
Copy link
Member

Oh interesting. I actually hadn't noticed that / hadn't realised how lightweight all of those packages are.

I think I'm probably okay depending on Bijectors then. Could someone @torfjelde @ElOceanografo make a PR to Stheno to incorporate this?

A good place to add this would be another file in the util directory. Should be included below here in Stheno.jl:
https://github.com/willtebbutt/Stheno.jl/blob/f0f78aae68122d01b582296fea186715ecf88a96/src/Stheno.jl#L65

@willtebbutt
Copy link
Member

Although, actually, @torfjelde is bijector defined for AbstractMvNormal, or just MvNormal? If it's the former then the FiniteGP could just subtype AbstractMvNormal instead of ContinuousMultivariateDistribution.

@torfjelde
Copy link

Ah, awesome!:)

And we're currently implementing it for MvNormal, but don't see any reason why we shouldn't do it for AbstractMvNormal, so we could at least make that change on Bijectors.jl's end 👍

@willtebbutt
Copy link
Member

Fantastic. I'll change things on my end, then hopefully it will "just work".

@willtebbutt
Copy link
Member

Will leave this open until we've got new versions tagged.

@torfjelde
Copy link

Sweet! I'll make a PR for Bijectors.jl 👍

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 a pull request may close this issue.

4 participants