-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use only docstrings in documentation + fix them #236
Conversation
@check_args(GammaExponentialKernel, γ, γ >= zero(T), "γ > 0") | ||
return new{T}([γ]) | ||
function GammaExponentialKernel(; gamma::Real=2.0, γ::Real=gamma) | ||
@check_args(GammaExponentialKernel, γ, zero(γ) < γ ≤ 2, "γ ∈ (0, 2]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
@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]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here h=0
is uninteresting.
|
||
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current documentation is incorrect, it is missing the minus sign inside the exponential.
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, the current docs are missing the minus sign.
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}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
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(κ.α)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for using the SqEuclidean
distance and dividing gamma by 2 instead of this approach, @willtebbutt?
@@ -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()]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really fantastic work. Just a few things to fix.
Co-authored-by: willtebbutt <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my points. Now LGTM.
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.:
How about keeping the unification (:+1:) but using unicode for maths as in the docstrings before? |
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 |
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) |
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. |
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 |
On a different note, it might also be problematic if the docstring contains characters that are only supported by very few fonts. |
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.