Skip to content

Some fixes for MaternKernel #121

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

Merged
merged 3 commits into from
Jun 16, 2020

Conversation

devmotion
Copy link
Member

This PR removes the loggamma definition and adjoint, removes the custom adjoint for kappa(::MaternKernel, ::Real), and fixes a type instability of this function. The loggamma function is already defined in SpecialFunctions and the custom adjoint is not needed when one uses ifelse instead of _ ? _ : _, as suggested in #114 (comment):

julia> using Zygote

julia> Zygote.gradient(0.0) do x
        iszero(x) ? one(x) : x^2
       end
(nothing,)

julia> Zygote.gradient(1.0) do x
        iszero(x) ? one(x) : x^2
       end
(2.0,)

julia> Zygote.gradient(0.0) do x
        ifelse(iszero(x), one(x), x^2)
       end
(0.0,)

julia> Zygote.gradient(1.0) do x
        ifelse(iszero(x), one(x), x^2)
       end
(2.0,)

Moreover, the PR fixes a type instability of kappa(::MaternKernel, ::Real). Currently

julia> @code_warntype KernelFunctions.kappa(MaternKernel= 3.0), 1)
Variables
  #self#::Core.Compiler.Const(KernelFunctions.kappa, false)
  κ::MaternKernel{Float64}
  d::Int64
  ν::Float64

Body::Union{Float64, Int64}
1nothing%2 = Base.getproperty(κ, )::Array{Float64,1}
│        (ν = KernelFunctions.first(%2))
│   %4 = KernelFunctions.iszero(d)::Bool%5 = KernelFunctions.one(d)::Core.Compiler.Const(1, false)
│   %6 = KernelFunctions._matern(ν, d)::Float64%7 = KernelFunctions.ifelse(%4, %5, %6)::Union{Float64, Int64}
└──      return %7

whereas with this PR

julia> @code_warntype KernelFunctions.kappa(MaternKernel= 3.0), 1)
Variables
  #self#::Core.Compiler.Const(KernelFunctions.kappa, false)
  κ::MaternKernel{Float64}
  d::Int64
  result::Float64

Body::Float64
1nothing%2 = Base.getproperty(κ, )::Array{Float64,1}%3 = KernelFunctions.first(%2)::Float64
│        (result = KernelFunctions._matern(%3, d))
│   %5 = KernelFunctions.iszero(d)::Bool%6 = KernelFunctions.one(result)::Core.Compiler.Const(1.0, false)
│   %7 = KernelFunctions.ifelse(%5, %6, result)::Float64
└──      return %7

@devmotion
Copy link
Member Author

The test failures are very unexpected (but unrelated) - why would we test AD with negative values for the sigma^2 in ScaledKernel? And why was that not discovered when these tests were added? Maybe we have to set a seed somewhere?

@theogf
Copy link
Member

theogf commented Jun 16, 2020

Oh you're right I did not use the exp. transform here

test_ADs(x->x[1] * SqExponentialKernel(), rand(1))

I will make a very short PR

@devmotion
Copy link
Member Author

I rebased the PR.

@devmotion devmotion merged commit 395a629 into JuliaGaussianProcesses:master Jun 16, 2020
@devmotion devmotion deleted the maternad branch June 16, 2020 11:51
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.

2 participants