search

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

Delta type restrictions #479

Closed willtebbutt closed 1 year ago

willtebbutt commented 1 year ago

@theogf do you know why our Delta metric is constrained to work for numeric-ish types? Should it not work for anything that has == defined on it?

cc @glennmoy

theogf commented 1 year ago

Yeah no idea why I did this... This should be changed...

willtebbutt commented 1 year ago

Cool. We should add a test when we do. I think we should probably do a more general audit, but we shouldn't block this change in the short-term.

@molet do you have any other examples of kernels where you'd have thought things should just work out-of-the-box with non-numeric data types and they don't?

glennmoy commented 1 year ago

_wrap is another example (where we initially came across this)

Restricted to AbstractVector{<:Real} and AbstractMatrix{<:Real}

https://github.com/JuliaGaussianProcesses/KernelFunctions.jl/blob/d425983cd734c5c9c2e8c7573a97491ea5ecaa52/src/transform/selecttransform.jl#L31-L32

molet commented 1 year ago

Although there might be some exceptions like kernels in constant.jl, I think in general it is a good approach to require numeric types for most of the kernels.

So far, I successfully used FunctionTransform for many situations when I needed to convert a non-numeric type to a numeric one to be able to use specific numeric type based kernels. I think this is a good practice.

Therefore, I would rather focus on making some transform functions more general and let them take non-numerical types: SelectTransform and FunctionTransform would definitely need this feature (so we might want to change this line as well to be more general).

glennmoy commented 1 year ago

Although there might be some exceptions like kernels in constant.jl, I think in general it is a good approach to require numeric types for most of the kernels.

If this is the desired interface it would also be good to document it.

theogf commented 1 year ago

Although there might be some exceptions like kernels in constant.jl, I think in general it is a good approach to require numeric types for most of the kernels.

I think that's already true with the kappa function. But for things like Transform and Metric types, this should not be the case. We aim at being as agnostic as possible to the type of data.

willtebbutt commented 1 year ago

I believe that this has been resolved via #480 so will close this issue. Please let me know if there's anything missing.