Add kappa to stationary Gabor kernel

[sharanry]

Since Gabor kernel is a Stationary and and Isotropic kernel, I thought it would be a good idea to add kappa of the form kappa(κ::GaborKernel, d::Real). Missed it earlier in #52 .

[theogf]

Could you just change the basekernel path? Then we can merge!

[sharanry]

@willtebbutt @theogf @devmotion Is it ready to merge?

[devmotion]

Do we still need this? Can't we just use the generic implementation in

KernelFunctions.jl/src/generic.jl

Line 7 in 89baf9c

kappa(κ::Kernel, x, y) = kappa(κ, evaluate(metric(κ), x, y))

now that a metric is defined for the Gabor kernel?

Suggested change

	kappa(κ::GaborKernel, x, y) = kappa(κ.kernel, x, y)

[sharanry]

I was wondering why without this the test kappa(k, x, y) == kappa(k, evaluate(KernelFunctions.metric(k), x, y)) was failing without this line. There was an error in the kappa(::GaborKernel, d::Real).

Apparently, kappa(k::TransformedKernel, d) = kappa(k.kernel, d), so it doesn't do the transformation before applying the kernel using the distance measure. I think this is a bug?

[sharanry]

Present fix is to modify the kappa to give the intended output of gabor kernel

function kappa(κ::GaborKernel, d::Real)
    return kappa(κ.kernel.kernels[1], (d/κ.ell)^2) * kappa(κ.kernel.kernels[2], d/κ.p)
end

[devmotion]

That's not something we want to do since ell and p do not have to be scalar valued. It seems it just doesn't make sense to define kappa(::GaborKernel, ::Real) and metric(::GaborKernel), and we should just keep the old implementation of forwarding kappa to the underlying kernel? If we want to do any optimization based on the metric, it should be done in KernelProd (see the PR for its refactoring).

[devmotion]

Unfortunately, IMO this PR should be closed since it is not guaranteed that parameters p and ell are scalar valued (see #79 (comment)).

[willtebbutt]

To be honest, I'm a bit confused as to why we have this implemented as a kernel at all. It seems like something that would be best implemented by a helper function in terms of Prod. Am I missing something?

[devmotion]

I agree, I'm still not fully convinced that we should have a separate type. We had a discussion about this when it was added (see #52 (comment) and the comments below). IIRC one argument for having a dedicated type was that otherwise the parameters of the Gabor kernel can't be accessed easily.

[sharanry]

Yes accessing the Gabor's parameters was the only issue if it were just a simple function returning a KernelProduct. This would not affect making the kernel trainable in any way. If the parameters are not really necessary to be accessed, I can't seem to think of any other good reason to keep a separate type.

[sharanry]

I don't think this PR has any value anymore. Gabor kernel is a KernelProduct now. kappa is still cannot be defined for it because it is not defined for KernelProduct. Should we consider refining it?

[theogf]

I guess we should close this PR because kappa only make sense for SimpleKernel