Use only docstrings in documentation + fix them #236

devmotion · 2021-01-19T02:16:06Z

Addresses #218 and #216 and uses the docstrings in the documentation of kernel functions and transformation.

The docstrings are updated and synced with the documentation, a bunch of errors are fixed (e.g., incorrect Matérn kernel functions and rational quadratic kernels), formatting of most docstrings of kernels and transformations is fixed (basically everything apart from the Wiener kernel and spectral mixture kernel, I was too tired to fix the mess there), doctests are added to most transformations, and incorrect explanations and outdated code of the transformations are fixed.

docs/src/kernels.md

devmotion · 2021-01-19T10:24:36Z

src/basekernels/exponential.jl

-        @check_args(GammaExponentialKernel, γ, γ >= zero(T), "γ > 0")
-        return new{T}([γ])
+    function GammaExponentialKernel(; gamma::Real=2.0, γ::Real=gamma)
+        @check_args(GammaExponentialKernel, γ, zero(γ) < γ ≤ 2, "γ ∈ (0, 2]")


Otherwise the kernel is not positive definite (the case 0 is not interesting and was excluded in the docstring but not in the check itself).

devmotion · 2021-01-19T10:25:37Z

src/basekernels/fbm.jl

-        @assert 0.0 <= h <= 1.0 "FBMKernel: Given Hurst index h is invalid."
-        return new{T}([h])
+    function FBMKernel(; h::Real=0.5)
+        @check_args(FBMKernel, h, zero(h) ≤ h ≤ one(h), "h ∈ [0, 1]")


Also here h=0 is uninteresting.

devmotion · 2021-01-19T10:27:36Z

src/basekernels/matern.jl

+
+For inputs ``x, x' \\in \\mathbb{R}^d``, the Matérn kernel of order ``3/2`` is given by
+```math
+k(x, x') = \\big(1 + \\sqrt{3} \\|x - x'\\|_2 \\big) \\exp\\big(- \\sqrt{3}\\|x - x'\\|_2\\big).


The current documentation is incorrect, it is missing the minus sign inside the exponential.

devmotion · 2021-01-19T10:28:03Z

src/basekernels/matern.jl

+For inputs ``x, x' \\in \\mathbb{R}^d``, the Matérn kernel of order ``5/2`` is given by
+```math
+k(x, x') = \\bigg(1 + \\sqrt{5} \\|x - x'\\|_2 + \\frac{5}{3}\\|x - x'\\|_2^2\\bigg)
+           \\exp\\big(- \\sqrt{5}\\|x - x'\\|_2\\big).


Same here, the current docs are missing the minus sign.

devmotion · 2021-01-19T10:33:20Z

src/basekernels/rationalquad.jl

+For inputs ``x, x' \\in \\mathbb{R}^d``, the rational-quadratic kernel with shape parameter
+``\\alpha > 0`` is defined as
+```math
+k(x, x'; \\alpha) = \\bigg(1 + \\frac{\\|x - x'\\|_2^2}{2\\alpha}\\bigg)^{-\\alpha}.


Currently, docstring and documentation are not consistent - the documentation is missing a factor of 2 in the denominator. Since the docstring and code were consistent, I only fixed the format of the docstring. IIRC you changed this a while back @willtebbutt?

src/basekernels/rationalquad.jl

devmotion · 2021-01-19T10:47:16Z

src/basekernels/rationalquad.jl

-function kappa(κ::GammaRationalQuadraticKernel, d²::Real)
-    return (one(d²) + d²^(first(κ.γ) / 2) / first(κ.α))^(-first(κ.α))
+function kappa(κ::GammaRationalQuadraticKernel, d::Real)
+    return (one(d) + d^first(κ.γ) / (2 * first(κ.α)))^(-first(κ.α))


Is there a reason for using the SqEuclidean distance and dividing gamma by 2 instead of this approach, @willtebbutt?

devmotion · 2021-01-19T11:12:59Z

test/basekernels/exponential.jl

@@ -44,7 +44,7 @@
        @test repr(k) == "Gamma Exponential Kernel (γ = $(γ))"
        @test KernelFunctions.iskroncompatible(k) == true

-        test_ADs(γ -> GammaExponentialKernel(; gamma=first(γ)), [γ])
+        test_ADs(γ -> GammaExponentialKernel(; gamma=first(γ)), [1 + 0.5 * rand()])


Ideally, one should use a reparameterization R -> (0,2] here, e.g. f(x) = 2 * logistic(x). With the current boundary point gamma = 2 finite differencing errors since it steps outside the domain.

willtebbutt

This is really fantastic work. Just a few things to fix.

docs/src/transform.md

docs/src/kernels.md

src/kernels/scaledkernel.jl

src/kernels/transformedkernel.jl

src/kernels/scaledkernel.jl

docs/src/kernels.md

docs/src/transform.md

Co-authored-by: willtebbutt <wct23@cam.ac.uk>

willtebbutt

Thanks for addressing my points. Now LGTM.

st-- · 2021-01-20T11:22:47Z

The downside of this is that while the maths may look nice on the rendered docs online, the REPL help is now rather less helpful compared to the previous use of unicode maths in the docstrings, e.g.:

  MaternKernel(; ν::Real=1.5)

  Matérn kernel of order ν.

  Definition
  ≡≡≡≡≡≡≡≡≡≡≡≡

  For inputs x, x' \in \mathbb{R}^d, the Matérn kernel of order \nu > 0 is defined as

  k(x,x';\nu) = \frac{2^{1-\nu}}{\Gamma(\nu)}\big(\sqrt{2\nu}\|x-x'\|_2\big) K_\nu\big(\sqrt{2\nu}\|x-x'\|_2\big),

  where \Gamma is the Gamma function and K_{\nu} is the modified Bessel function of the second kind of order \nu.

  A Gaussian process with a Matérn kernel is \lceil \nu \rceil - 1-times differentiable in the mean-square sense.

How about keeping the unification (:+1:) but using unicode for maths as in the docstrings before?

devmotion · 2021-01-20T11:33:56Z

I tried this recently in some other package but unfortunately unicode math does not work reliably with Documenter (or the javascript libraries). Greek letters are displayed in a different non-math font, brackets can not be adjusted properly, unicode supports only a limited number of sub- and superscripts etc. Other things such as \frac are just not possible to write with unicode characters only. So in any case one would end up with a mix of both LaTeX and unicode characters, which makes e.g. the differences of the fonts even more prominent and does not solve the problem of LaTeX syntax in docstrings.

devmotion · 2021-01-20T11:38:25Z

Possibly also due to these reasons, LaTeX syntax in docstrings is common practice in other packages such as Distributions (see e.g. https://github.com/JuliaStats/Distributions.jl/blob/master/src/univariate/discrete/betabinomial.jl) and standard libraries (see e.g. https://github.com/JuliaLang/julia/blob/d6f99481b1d5cf399165be2eb3893c5bdbe90dcf/stdlib/LinearAlgebra/src/qr.jl#L19)

theogf · 2021-01-20T13:04:31Z

I chime in definitely too late, but that was the reason for two versions of the docs (docstring vs docs.md), unicode readable for docstring and latex for documenter.

devmotion · 2021-01-20T13:59:23Z

I definitely agree that it would be nice to have some more readable output in the REPL and I tried to use unicode but it does not work. Only the simplest formulas can be written with unicode only, so one basically always has to write them at least partly with LaTeX syntax. And even the simplest unicode formulas look terrible on the webpage (e.g. even subscripts and operators such as exp).

devmotion · 2021-01-20T14:03:15Z

On a different note, it might also be problematic if the docstring contains characters that are only supported by very few fonts.

Use only docstrings in documentation + fix them

4ebdecf

devmotion commented Jan 19, 2021

View reviewed changes

docs/src/kernels.md Outdated Show resolved Hide resolved

devmotion and others added 6 commits January 19, 2021 03:17

Update docs/src/kernels.md

d59f5ab

Fix typo

662e252

Fix LaTeX

f18c8b1

Add missing @ref

f7d3621

Fix tests

8db1c3d

Fix format

d17e753

devmotion commented Jan 19, 2021

View reviewed changes

src/basekernels/rationalquad.jl Outdated Show resolved Hide resolved

devmotion commented Jan 19, 2021

View reviewed changes

Revert reparameterization and extend docstring

120c957

devmotion commented Jan 19, 2021

View reviewed changes

devmotion requested a review from willtebbutt January 19, 2021 11:22

Update docstring of Wiener kernel

fd8fdbc

willtebbutt requested changes Jan 19, 2021

View reviewed changes

devmotion mentioned this pull request Jan 19, 2021

Inconsistency in rational kernels #238

Closed

devmotion commented Jan 19, 2021

View reviewed changes

src/kernels/transformedkernel.jl Outdated Show resolved Hide resolved

devmotion commented Jan 19, 2021

View reviewed changes

src/kernels/transformedkernel.jl Outdated Show resolved Hide resolved

devmotion commented Jan 19, 2021

View reviewed changes

src/kernels/transformedkernel.jl Show resolved Hide resolved

devmotion commented Jan 19, 2021

View reviewed changes

src/kernels/scaledkernel.jl Outdated Show resolved Hide resolved

devmotion commented Jan 19, 2021

View reviewed changes

docs/src/kernels.md Show resolved Hide resolved

devmotion commented Jan 19, 2021

View reviewed changes

docs/src/transform.md Outdated Show resolved Hide resolved

devmotion and others added 3 commits January 19, 2021 15:48

Apply suggestions from code review

4a2a844

Co-authored-by: willtebbutt <wct23@cam.ac.uk>

Try to reformat kernel functions

dd2223e

Fix links

befebd9

devmotion added 3 commits January 19, 2021 17:08

Update transform.md

93ac89e

Fix typo

4bbac93

Update according to @willtebbut's suggestion

44e5e27

willtebbutt approved these changes Jan 19, 2021

View reviewed changes

devmotion added 2 commits January 19, 2021 17:45

Fix some remaining inconsistencies

d2317b7

Fix two links

f0f7b70

devmotion merged commit 45226b0 into master Jan 19, 2021

devmotion deleted the dw/docs branch January 19, 2021 18:27

This was referenced Jan 19, 2021

Consolidate mathematical description of kernels between docs and docstrings #218

Closed

Split API docs into separate pages, remove redundancy with kernels/transforms #216

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use only docstrings in documentation + fix them #236

Use only docstrings in documentation + fix them #236

devmotion commented Jan 19, 2021

devmotion Jan 19, 2021

devmotion Jan 19, 2021

devmotion Jan 19, 2021

devmotion Jan 19, 2021

devmotion Jan 19, 2021

devmotion Jan 19, 2021 •

edited

Loading

devmotion Jan 19, 2021 •

edited

Loading

willtebbutt left a comment

willtebbutt left a comment

st-- commented Jan 20, 2021

devmotion commented Jan 20, 2021

devmotion commented Jan 20, 2021

theogf commented Jan 20, 2021

devmotion commented Jan 20, 2021

devmotion commented Jan 20, 2021

Use only docstrings in documentation + fix them #236

Use only docstrings in documentation + fix them #236

Conversation

devmotion commented Jan 19, 2021

devmotion Jan 19, 2021

Choose a reason for hiding this comment

devmotion Jan 19, 2021

Choose a reason for hiding this comment

devmotion Jan 19, 2021

Choose a reason for hiding this comment

devmotion Jan 19, 2021

Choose a reason for hiding this comment

devmotion Jan 19, 2021

Choose a reason for hiding this comment

devmotion Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

devmotion Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

willtebbutt left a comment

Choose a reason for hiding this comment

willtebbutt left a comment

Choose a reason for hiding this comment

st-- commented Jan 20, 2021

devmotion commented Jan 20, 2021

devmotion commented Jan 20, 2021

theogf commented Jan 20, 2021

devmotion commented Jan 20, 2021

devmotion commented Jan 20, 2021

devmotion Jan 19, 2021 •

edited

Loading

devmotion Jan 19, 2021 •

edited

Loading