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

Fix method ambiguities #483

Closed devmotion closed 1 year ago

devmotion commented 1 year ago

Fixes #481.

I assume the only questionable change is that fixing the method ambiguity of map(t::Transform, ::AbstractVector), caused by map(::Tf, ::SparseVector) where {Tf} in SparseArrays, requires adding a dependency on SparseArrays.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 80.00% and project coverage change: -0.10 :warning:

Comparison is base (9da7bfd) 94.25% compared to head (143c8b0) 94.16%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #483 +/- ## ========================================== - Coverage 94.25% 94.16% -0.10% ========================================== Files 52 52 Lines 1375 1387 +12 ========================================== + Hits 1296 1306 +10 - Misses 79 81 +2 ``` | [Impacted Files](https://codecov.io/gh/JuliaGaussianProcesses/KernelFunctions.jl/pull/483?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGaussianProcesses) | Coverage Δ | | |---|---|---| | [src/chainrules.jl](https://codecov.io/gh/JuliaGaussianProcesses/KernelFunctions.jl/pull/483?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGaussianProcesses#diff-c3JjL2NoYWlucnVsZXMuamw=) | `87.65% <ø> (ø)` | | | [src/utils.jl](https://codecov.io/gh/JuliaGaussianProcesses/KernelFunctions.jl/pull/483?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGaussianProcesses#diff-c3JjL3V0aWxzLmps) | `91.46% <50.00%> (ø)` | | | [src/transform/transform.jl](https://codecov.io/gh/JuliaGaussianProcesses/KernelFunctions.jl/pull/483?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGaussianProcesses#diff-c3JjL3RyYW5zZm9ybS90cmFuc2Zvcm0uamw=) | `81.81% <75.00%> (-18.19%)` | :arrow_down: | | [src/kernels/overloads.jl](https://codecov.io/gh/JuliaGaussianProcesses/KernelFunctions.jl/pull/483?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGaussianProcesses#diff-c3JjL2tlcm5lbHMvb3ZlcmxvYWRzLmps) | `100.00% <100.00%> (ø)` | | | [src/transform/chaintransform.jl](https://codecov.io/gh/JuliaGaussianProcesses/KernelFunctions.jl/pull/483?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGaussianProcesses#diff-c3JjL3RyYW5zZm9ybS9jaGFpbnRyYW5zZm9ybS5qbA==) | `81.48% <100.00%> (+1.48%)` | :arrow_up: | 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 in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

devmotion commented 1 year ago

I assume the only questionable change is that fixing the method ambiguity of map(t::Transform, ::AbstractVector), caused by map(::Tf, ::SparseVector) where {Tf} in SparseArrays, requires adding a dependency on SparseArrays.

I guess it's even more questionable since actually it is not sufficient to fix the method ambiguities in the tests which load additional packages (see, e.g., https://github.com/JuliaGaussianProcesses/KernelFunctions.jl/actions/runs/3268189406/jobs/5374299901#step:6:222 and https://github.com/JuliaGaussianProcesses/KernelFunctions.jl/actions/runs/3268189406/jobs/5374301250#step:6:224):

   Evaluated: isempty(Tuple{Method, Method}[(map(f::Tf, A::SparseArrays.AbstractCompressedVector) where Tf @ SparseArrays.HigherOrderFns /opt/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.9/SparseArrays/src/higherorderfns.jl:152, map(t::Transform, x::AbstractVector) @ KernelFunctions ~/work/KernelFunctions.jl/KernelFunctions.jl/src/transform/transform.jl:11), (map(f, A::NamedDims.NamedDimsArray) @ NamedDims ~/.julia/packages/NamedDims/kCxfn/src/functions.jl:157, map(t::Transform, x::AbstractVector) @ KernelFunctions ~/work/KernelFunctions.jl/KernelFunctions.jl/src/transform/transform.jl:11), (map(f, a1::StaticArraysCore.StaticArray, as::AbstractArray...) @ StaticArrays ~/.julia/packages/StaticArrays/PUoe1/src/mapreduce.jl:30, map(t::Transform, x::AbstractVector) @ KernelFunctions ~/work/KernelFunctions.jl/KernelFunctions.jl/src/transform/transform.jl:11), (map(f, As::AxisArray{T, N, D, Ax}...) where {T, N, D, Ax<:Tuple{Vararg{Axis}}} @ AxisArrays ~/.julia/packages/AxisArrays/v7vf4/src/core.jl:431, map(t::Transform, x::AbstractVector) @ KernelFunctions ~/work/KernelFunctions.jl/KernelFunctions.jl/src/transform/transform.jl:11), (map(f, A::AxisArray) @ AxisArrays ~/.julia/packages/AxisArrays/v7vf4/src/core.jl:422, map(t::Transform, x::AbstractVector) @ KernelFunctions ~/work/KernelFunctions.jl/KernelFunctions.jl/src/transform/transform.jl:11), (map(f::Tf, A::Union{SparseArrays.AbstractCompressedVector, SparseArrays.AbstractSparseMatrixCSC}, Bs::Vararg{Union{SparseArrays.AbstractCompressedVector, SparseArrays.AbstractSparseMatrixCSC}, N}) where {Tf, N} @ SparseArrays.HigherOrderFns /opt/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.9/SparseArrays/src/higherorderfns.jl:156, map(t::Transform, x::AbstractVector) @ KernelFunctions ~/work/KernelFunctions.jl/KernelFunctions.jl/src/transform/transform.jl:11)])
devmotion commented 1 year ago

So I am not sure if we can fix the map method ambiguity issues at all.

theogf commented 1 year ago

@devmotion Can we get this merged anyway (we can leave some ambiguities)

theogf commented 1 year ago

Lgtm when the tests are fixed

devmotion commented 1 year ago

@theogf I managed to fix all method ambiguities by completely removing the definition of map(t::Transform, x::AbstractVector) (causes problems with basically every array package since they tend to implement map(f, x::MyArray)). Instead I only defined map(t::Transform, x::Union{ColVecs,RowVecs}).