Add RationalKernel and rename GammaRationalQuadraticKernel

[devmotion]

Implements the suggestion in #238 to increase consistency. Not sure if it is the best solution to the problem though 😄

Fixes #238.

[st--]

The kernel is commonly called RationalQuadratic (or some variant thereof such as RQ or RatQuad), not Rational (e.g. GPML book, Scikit-Learn, GPflow, GPyTorch, GPy...). I think it'd be valuable to keep the name and convention consistent with the literature & other packages so as to make it easier for users. (If that helps to convince you, using the SqEuclidean distance saves an unnecessary sqrt() and so leads to marginally faster code.)

[devmotion]

This PR leaves the RationalQuadraticKernel unchanged. There's no deviation from the literature or other packages.

The PR only addresses the inconsistency explained in #238. Currently we have a GammaRationalQuadraticKernel which has not the form of a quadratic kernel and is not a simple generalization of the RationalQuadraticKernel due to an additional factor in the denominator.

As mentioned in the issue, an alternative would be to make the GammaRationalQuadraticKernel an actual generalization of the RationalQuadraticKernel by adding the additional factor in the denominator. In this case, however, one would lose the property that it converges to the GammaExponentialKernel as alpha goes to infinity. Thus then the group of ExponentialKernel, SqExponentialKernel (which is the limit of the RationalQuadraticKernel), and GammaExponentialKernel is not consistent with the RationalQuadraticKernel and the GammaRationalQuadraticKernel anymore.

Therefore the approach in this PR is to just rename the GammaRationalQuadraticKernel to GammaRationalKernel (to indicate that it is not derived as a simple generalization of the RationalQuadraticKernel) and to add a kernel that is naturally generalized by the GammaRationalKernel which I therefore called RationalKernel. This also consistent with the exponential kernels - in the limit, the RationalKernel goes to the ExponentialKernel, the RationalQuadraticKernel to the SqExponentialKernel, and the GammaRationalKernel to the GammaExponentialKernel.

[st--]

Ah, I misunderstood what was going on then. Thanks for the thorough explanation - that was helpful :)

[st--]

@devmotion do you still want to merge this in ?

[devmotion]

Yes, I'd still like to resolve #238.

[st--]

Great! What's missing / still needs doing before we can merge it & resolve the issue?:)

[devmotion]

Someone has to approve the proposal or suggest an alternative for how the inconsistency could be resolved.

[st--]

how about turning it into a rawstring so we can do away with all the escaping of LaTeX backslashes?

[st--]

I think this proposal is fine, I'm just wondering about the choice of default values for the arguments... should the default gamma of GammaRational be such that it's just a Rational kernel?

[devmotion]

No, this would break the consistency again if we do not change the default values of GammaExponentialKernel in the same way 😄

This also consistent with the exponential kernels - in the limit, the RationalKernel goes to the ExponentialKernel, the RationalQuadraticKernel to the SqExponentialKernel, and the GammaRationalKernel to the GammaExponentialKernel.

The GammaExponentialKernel with default arguments is not an ExponentialKernel but a SqExponentialKernel, so it seems fine that the GammaRationalKernel with default arguments is not a RationalKernel but a RationalQuadraticKernel. If we want to change this, the default values of both GammaRationalKernel and GammaExponentialKernel should be changed.

[st--]

The GammaExponentialKernel with default arguments is not an ExponentialKernel but a SqExponentialKernel

Not quite though; they differ by a factor of 2 in the exponent, no?

[st--]

I'm wondering if we need to provide default arguments? It might be clearer to signal to the user "Hey, here's this extra argument, think about it - if you don't want to think about it you've got the wrong kernel"

[devmotion]

Not quite though; they differ by a factor of 2 in the exponent, no?

True, I missed this. In any case, you don't get the ExponentialKernel with the default value 😉

I'm wondering if we need to provide default arguments? It might be clearer to signal to the user "Hey, here's this extra argument, think about it - if you don't want to think about it you've got the wrong kernel"

Sure, this could be done, but I think it would be better to drop default arguments in a separate PR since it is breaking and could be done for other kernels as well (e.g., MaternKernel which just gives a less efficient Matern32Kernel with its default value).

[st--]

True, I missed this. In any case, you don't get the ExponentialKernel with the default value

Not with the default value currently in this PR! What I meant was, I think if having a default value, setting it to gamma=1 makes more intuitive sense (to me, anyways) as in that case you do recover the Exponential kernel.

Sure, this could be done, but I think it would be better to drop default arguments in a separate PR since it is breaking and could be done for other kernels as well (e.g., MaternKernel which just gives a less efficient Matern32Kernel with its default value).

So how about changing the default values to gamma=1, then GammaExponentialKernel is just an ExponentialKernel with more flexibility and same for GammaRationalKernel and RationalKernel ? And opening an issue to keep track of separately removing all not-particularly-helpful/intuitive default values?

Did you have a particular reason not to use raw"" for the docstrings?

[devmotion]

I would strongly prefer to not change the default values in this PR since it would be a breaking change whereas the PR otherwise is non-breaking. Therefore I suggest to discuss this in a separate issue and fix it in a separate breaking PR.

To summarize more clearly the current state:

• currently, on master and in this PR, the default value for GammaExponentialKernel does not give you an ExponentialKernel (https://juliagaussianprocesses.github.io/KernelFunctions.jl/dev/kernels/#KernelFunctions.GammaExponentialKernel)
• this PR fixes the inconsistencies of GammaRationalQuadraticKernel and GammaExponentialKernel
• to keep the relations between the kernels consistent, one has to change the default values for GammaExponentialKernel and GammaRationalKernel in the same way

Did you have a particular reason not to use raw"" for the docstrings?

The docstring for RationalKernel is copied from the one for GammaRationalQuadraticKernel which does not use raw""". Personally I don't mind the backslashes, in particular since I expect that the docstring will not be edited frequently, and therefore it seemed unnecessary work to remove them.

[st--]

Yes that all sounds good. Do you want to create the issue? This PR has a merge conflict due to removing the deprecated.jl with the latest release. Happy to approve once that's fixed.

The backslashes are a bit of a pet peeve of mine, I'd be happy to go do that work myself in a separate PR (applied everywhere), would you be up for reviewing/approving that then?

[devmotion]

I noticed that I had also suggested in #238 to change the default value of gamma to 1 😄 So I thought I could already add a deprecation warning in this PR (I also added a warning to the docstring that is shown prominently on the webpage as well).

The backslashes are a bit of a pet peeve of mine, I'd be happy to go do that work myself in a separate PR (applied everywhere), would you be up for reviewing/approving that then?

Sure, I can review and approve such a PR.

[devmotion]

I merge this now since there were no additional comments or suggestions.