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

Type stability of KernelSum #458

Closed
simsurace opened this issue Jun 23, 2022 · 6 comments
Closed

Type stability of KernelSum #458

simsurace opened this issue Jun 23, 2022 · 6 comments

Comments

@simsurace
Copy link
Member

simsurace commented Jun 23, 2022

using KernelFunctions
using Test
k = RBFKernel() + RBFKernel() * ExponentialKernel()
@inferred k(0.1, 0.2) # ERROR
@time k(0.1, 0.2)  # 0.000009 seconds (5 allocations: 176 bytes)

but e.g.

using KernelFunctions
using Test
k = RBFKernel() + RBFKernel() * LinearKernel()
@inferred k(0.1, 0.2) # works

Similar results hold for kernelmatrix and kernelmatrix_diag.
I haven't figured out why certain kernels show the problem while others don't.
This may be a variant of JuliaLang/julia#45748 and could be a Julia issue.
The similarity to the referenced Julia issue is that calling sum before affects type inference:

# restart Julia
using KernelFunctions
using Test
k = RBFKernel() + RBFKernel() * ExponentialKernel()
sum(f -> f(0.1, 0.2), k.kernels)
@inferred k(0.1, 0.2) # works
@time k(0.1, 0.2) # 0.000008 seconds (1 allocation: 16 bytes)

Despite this probably being a Julia issue, we may want to patch it anyways, as it can adversely impact AD for certain GP models.
I'll open a PR.

@devmotion
Copy link
Member

Despite this probably being a Julia issue, we may want to patch it anyways

What's a possible workaround? Can we just change

::KernelSum)(x, y) = sum(k(x, y) for k in κ.kernels)

to

function::KernelSum)(x, y)
    return sum.kernels) do k
        return k(x, y)
    end
end

etc.?

@simsurace
Copy link
Member Author

I'll double-check, but I think I tried that and it did not solve the issue. I think I'll use the init keyword, as the Julia issue seems to be related to the inability to figure that out automatically.

@devmotion
Copy link
Member

Maybe one has to add a let block to avoid the closure type inference bug?

@simsurace
Copy link
Member Author

Hmm, seemed to be a false alarm. The issue is that many things seem to work if the function is redefined after module import, but not in a new Julia session immediately after precompilation.

@willtebbutt
Copy link
Member

@simsurace can we close this now?

@simsurace
Copy link
Member Author

Sure. It should have been linked to #459 I guess, but it doesn't get linked automatically when non-admins use the closing keywords.

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

No branches or pull requests

3 participants