Deprecate use of Mahalanobis distance #225
Conversation
|
Maybe it would be worth noting somewhere in the docs how to create a "MahalonobisKernel" (and similarly use the Maha.. distance for PieceWise..), there might be people looking for this exactly without realizing that one can use |
|
I guess I should set up JuliaFormatter support for my editors but I don't want to mess with all the other projects that don't use it... Maybe calling |
| return 1 + | ||
| (j + 3) * r + | ||
| (6 * j^2 + 36j + 45) / 15 * r .^ 2 + | ||
| (j^3 + 9 * j^2 + 23j + 15) / 15 * r .^ 3 | ||
| (j + 3) * r + | ||
| (6 * j^2 + 36j + 45) / 15 * r ^ 2 + | ||
| (j^3 + 9 * j^2 + 23j + 15) / 15 * r ^ 3 |
There was a problem hiding this comment.
JuliaFormatter wants this to be formatted
return 1 +
(j + 3) * r +
(6 * j^2 + 36j + 45) / 15 * r ^ 2 +
(j^3 + 9 * j^2 + 23j + 15) / 15 * r ^ 3This feels weird since usually we just increase the indentation level but do not align operators or lines. Maybe I missed it but I couldn't find any information about similar cases in BlueStyle - is this a bug in JuliaFormatter?
| struct PiecewisePolynomialKernel{D,C<:Tuple} <: SimpleKernel | ||
| alpha::Int | ||
| coeffs::C | ||
|
|
||
| function PiecewisePolynomialKernel{D}(dim::Int) where {D} | ||
| dim > 0 || error("number of dimensions has to be positive") | ||
| j = div(dim, 2) + D + 1 | ||
| alpha = j + D | ||
| coeffs = piecewise_polynomial_coefficients(Val(D), j) | ||
| return new{D,typeof(coeffs)}(alpha, coeffs) | ||
| end | ||
| end | ||
|
|
||
| function PiecewisePolynomialKernel(; v::Integer=0, maha::AbstractMatrix{<:Real}) | ||
| return PiecewisePolynomialKernel{v}(maha) | ||
| end | ||
| # TODO: remove `maha` keyword argument in next breaking release | ||
| function PiecewisePolynomialKernel(; v::Int=-1, degree::Int=v, maha=nothing, dim::Int=-1) | ||
| if v != -1 | ||
| Base.depwarn( | ||
| "keyword argument `v` is deprecated, use `degree` instead", | ||
| :PiecewisePolynomialKernel, | ||
| ) | ||
| end | ||
|
|
||
| # Have to reconstruct the type parameter | ||
| # See also https://github.com/FluxML/Functors.jl/issues/3#issuecomment-626747663 | ||
| function Functors.functor(::Type{<:PiecewisePolynomialKernel{V}}, x) where {V} | ||
| function reconstruct_kernel(xs) | ||
| return PiecewisePolynomialKernel{V}(xs.maha) | ||
| if maha !== nothing | ||
| Base.depwarn( | ||
| "keyword argument `maha` is deprecated, use a `LinearTransform` instead", | ||
| :PiecewisePolynomialKernel, | ||
| ) | ||
| dim = size(maha, 1) | ||
| return transform( | ||
| PiecewisePolynomialKernel{degree}(dim), LinearTransform(cholesky(maha).U) | ||
| ) | ||
| else | ||
| return PiecewisePolynomialKernel{degree}(dim) | ||
| end | ||
| return (maha=x.maha,), reconstruct_kernel | ||
| end | ||
|
|
||
| _f(κ::PiecewisePolynomialKernel{0}, r, j) = 1 | ||
| _f(κ::PiecewisePolynomialKernel{1}, r, j) = 1 + (j + 1) * r | ||
| _f(κ::PiecewisePolynomialKernel{2}, r, j) = 1 + (j + 2) * r + (j^2 + 4 * j + 3) / 3 * r .^ 2 | ||
| function _f(κ::PiecewisePolynomialKernel{3}, r, j) | ||
| return 1 + | ||
| (j + 3) * r + | ||
| (6 * j^2 + 36j + 45) / 15 * r .^ 2 + | ||
| (j^3 + 9 * j^2 + 23j + 15) / 15 * r .^ 3 | ||
| piecewise_polynomial_coefficients(::Val{0}, ::Int) = (1,) | ||
| piecewise_polynomial_coefficients(::Val{1}, j::Int) = (1, j + 1) | ||
| piecewise_polynomial_coefficients(::Val{2}, j::Int) = (1, j + 2, (j^2 + 4 * j)//3 + 1) | ||
| function piecewise_polynomial_coefficients(::Val{3}, j::Int) | ||
| return (1, j + 3, (2 * j^2 + 12 * j)//5 + 3, (j^3 + 9 * j^2 + 23 * j)//15 + 1) | ||
| end | ||
| function piecewise_polynomial_coefficients(::Val{D}, ::Int) where {D} | ||
| return error("invalid degree $D, only 0, 1, 2, or 3 are supported") | ||
| end | ||
|
|
||
| function kappa(κ::PiecewisePolynomialKernel{V}, r) where {V} | ||
| return max(1 - r, 0)^(κ.j + V) * _f(κ, r, κ.j) | ||
| # `evalpoly` is not available on Julia < 1.4 | ||
| @static if VERSION < v"1.4" | ||
| @generated function _evalpoly(r, coeffs) | ||
| N = length(coeffs.parameters) | ||
| return quote | ||
| return @evalpoly(r, $((:(coeffs[$i]) for i in 1:N)...)) | ||
| end | ||
| end | ||
| else | ||
| _evalpoly(r, coeffs) = evalpoly(r, coeffs) | ||
| end | ||
|
|
||
| metric(κ::PiecewisePolynomialKernel) = Mahalanobis(κ.maha) | ||
| kappa(κ::PiecewisePolynomialKernel, r) = max(1 - r, 0)^κ.alpha * _evalpoly(r, κ.coeffs) |
There was a problem hiding this comment.
What was your motivation behind changing all of this from explicit functions to storing coefficients inside the type struct? (I found the previous version of the code much easier to understand and read.)
There was a problem hiding this comment.
It seems wasteful to re-compute the fixed coefficients every time the kernel function is evaluated. Moreover, evalpoly uses Horner's method and hence the evaluation of the polynomials is more stable and more efficient than the naive implementation before.
|
@willtebbutt Do you have any comments or objections to this PR? Since everything should be deprecated properly, it is not breaking. |
| @@ -17,7 +17,7 @@ Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40" | |||
| ZygoteRules = "700de1a5-db45-46bc-99cf-38207098b444" | |||
|
|
|||
| [compat] | |||
| Compat = "2.2, 3" | |||
| Compat = "3.7" | |||
There was a problem hiding this comment.
I forgot that we already depend on Compat which contains an implementation of evalpoly for Julia < 1.4. It was introduced in Compat 3.7. Is it OK to drop support for older versions?
There was a problem hiding this comment.
Latest version of Compat is 3.25.
Fixes #177 and corrects the documentation of PiecewisePolynomialKernel.