JuliaGaussianProcesses / KernelFunctions.jl

Julia package for kernel functions for machine learning
https://juliagaussianprocesses.github.io/KernelFunctions.jl/stable/
MIT License
267 stars 32 forks source link

Stable version of KernelProduct and added `test_type_stability` #486

Closed theogf closed 1 year ago

theogf commented 1 year ago

Summary This adresses #485 by bringing a similar solution from #459. It goes slightly differently by avoiding a closure entirely. This also add tests for checking that on the culprit kernel, the type stability is maintained. Regarding performance, it seems to not change anything (maybe marginally better).

master:

julia> @btime kernelmatrix($nested_k, $x);
  5.005 μs (23 allocations: 10.94 KiB)

this PR:

julia> @btime kernelmatrix($nested_k, $x);
  4.936 μs (18 allocations: 10.69 KiB)

This also refactors a part of TestUtils.jl to run with test_with_type so that we can reuse this functionality for different tests.

Breaking changes Non-breaking

codecov[bot] commented 1 year ago

Codecov Report

Base: 94.32% // Head: 82.64% // Decreases project coverage by -11.68% :warning:

Coverage data is based on head (f647b40) compared to base (eac3538). Patch coverage: 65.45% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #486 +/- ## =========================================== - Coverage 94.32% 82.64% -11.69% =========================================== Files 52 52 Lines 1358 1371 +13 =========================================== - Hits 1281 1133 -148 - Misses 77 238 +161 ``` | [Impacted Files](https://codecov.io/gh/JuliaGaussianProcesses/KernelFunctions.jl/pull/486?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGaussianProcesses) | Coverage Δ | | |---|---|---| | [src/TestUtils.jl](https://codecov.io/gh/JuliaGaussianProcesses/KernelFunctions.jl/pull/486/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGaussianProcesses#diff-c3JjL1Rlc3RVdGlscy5qbA==) | `75.67% <55.00%> (-19.41%)` | :arrow_down: | | [src/kernels/kernelproduct.jl](https://codecov.io/gh/JuliaGaussianProcesses/KernelFunctions.jl/pull/486/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGaussianProcesses#diff-c3JjL2tlcm5lbHMva2VybmVscHJvZHVjdC5qbA==) | `56.66% <87.50%> (-43.34%)` | :arrow_down: | | [src/kernels/kernelsum.jl](https://codecov.io/gh/JuliaGaussianProcesses/KernelFunctions.jl/pull/486/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGaussianProcesses#diff-c3JjL2tlcm5lbHMva2VybmVsc3VtLmps) | `57.14% <100.00%> (-42.86%)` | :arrow_down: | | [src/kernels/gibbskernel.jl](https://codecov.io/gh/JuliaGaussianProcesses/KernelFunctions.jl/pull/486/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGaussianProcesses#diff-c3JjL2tlcm5lbHMvZ2liYnNrZXJuZWwuamw=) | `0.00% <0.00%> (-88.89%)` | :arrow_down: | | [src/kernels/normalizedkernel.jl](https://codecov.io/gh/JuliaGaussianProcesses/KernelFunctions.jl/pull/486/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGaussianProcesses#diff-c3JjL2tlcm5lbHMvbm9ybWFsaXplZGtlcm5lbC5qbA==) | `0.00% <0.00%> (-82.50%)` | :arrow_down: | | [src/kernels/neuralkernelnetwork.jl](https://codecov.io/gh/JuliaGaussianProcesses/KernelFunctions.jl/pull/486/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGaussianProcesses#diff-c3JjL2tlcm5lbHMvbmV1cmFsa2VybmVsbmV0d29yay5qbA==) | `0.00% <0.00%> (-77.56%)` | :arrow_down: | | [src/kernels/overloads.jl](https://codecov.io/gh/JuliaGaussianProcesses/KernelFunctions.jl/pull/486/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGaussianProcesses#diff-c3JjL2tlcm5lbHMvb3ZlcmxvYWRzLmps) | `25.00% <0.00%> (-75.00%)` | :arrow_down: | | [src/kernels/scaledkernel.jl](https://codecov.io/gh/JuliaGaussianProcesses/KernelFunctions.jl/pull/486/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGaussianProcesses#diff-c3JjL2tlcm5lbHMvc2NhbGVka2VybmVsLmps) | `41.17% <0.00%> (-47.06%)` | :arrow_down: | | ... and [2 more](https://codecov.io/gh/JuliaGaussianProcesses/KernelFunctions.jl/pull/486/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGaussianProcesses) | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGaussianProcesses). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGaussianProcesses)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

theogf commented 1 year ago

It's really hard to say, when going with Cthulhu through the calls, it seems that the only type unstable calls are due to the DimensionMismatch errors thrown by Distances.jl

devmotion commented 1 year ago

Hmm errors shouldn't cause type instabilities. Otherwise you could basically never throw errors in any function.

theogf commented 1 year ago

I honestly don't know how to find the underlying issue... if you have a method please share 😊

theogf commented 1 year ago

Hmm errors shouldn't cause type instabilities. Otherwise you could basically never throw errors in any function.

I was thinking of an eventuality:

When using a generator or Base.Fix2 we create a closure, which x is part of. Maybe it's just too much for the compiler to treat in such nested cases?

willtebbutt commented 1 year ago

@theogf any progress on this on your end? I'm happy to finish up the PR if you're busy

theogf commented 1 year ago

@theogf any progress on this on your end? I'm happy to finish up the PR if you're busy

I was thinking of finishing up this week-end.

willtebbutt commented 1 year ago

Cool. I'll leave you to it then :)

theogf commented 1 year ago

Broadly LGTM. Just some formatting issues to resolve, then I think we're good to go

edit: also, sorry for taking a few days to review this -- have been busy with work etc :(

No worries I think we are all on the same boat now :laughing: