-
Notifications
You must be signed in to change notification settings - Fork 36
Some improvement and additions to MO kernels #354
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
Changes from 25 commits
8427ae5
1a5ac23
d79d24b
4f23240
a33c312
a729c01
e1f92a8
e7d7db9
ee69c6e
d1d8d45
d80453c
06cbb3d
d03b951
c8f8093
cd6dedc
8b7410a
1978e25
3ffd436
c42e43d
96bf55e
b649e65
fe81b0a
509dfd6
33f3e87
0f9ce64
51a3736
8459d0c
1675598
0672a1f
cf8e8f3
31620e2
9b27b58
d68f86e
59ff2c8
7c99633
286d5d5
cb37e0f
fee8eaf
3466daa
533f8fd
213b606
f2c6ae6
407ca80
6bcc83d
d5571df
13427a7
48db5a0
43a5ab9
f2ac5ed
dac5db7
e61a668
8cbb1eb
c72efd9
11e6d56
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,3 +25,35 @@ end | |
k(x,x') = ∏ᵢᴰ k(xᵢ,x'ᵢ) | ||
""" | ||
@inline iskroncompatible(κ::Kernel) = false # Default return for kernels | ||
|
||
@inline ismatrixkroncompatible(κ::MOKernel) = false # Default return for kernels | ||
@inline ismatrixkroncompatible(κ::IndependentMOKernel) = true | ||
@inline ismatrixkroncompatible(κ::IntrinsicCoregionMOKernel) = true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would this be separate from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question. I did this because I wanted to avoid confusion with the first two functions, because the MO kernel matrix construction is distinct from the kronecker construction that was there already. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
function _kroneckerkernelmatrix(Ktmp, B, ::MOInputIsotopicByFeatures) | ||
return Kronecker.kronecker(Ktmp, B) | ||
end | ||
|
||
function _kroneckerkernelmatrix(K, Ktmp, B, ::MOInputIsotopicByOutputs) | ||
Crown421 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return Kronecker.kronecker(B, Ktmp) | ||
end | ||
|
||
function kernelkronmat(k::IndependentMOKernel, x::MOI, y::MOI) where {MOI<:MOInputsUnion} | ||
@assert x.out_dim == y.out_dim | ||
Ktmp = kernelmatrix(k.kernel, x.x, y.x) | ||
mtype = eltype(Ktmp) | ||
return _kroneckerkernelmatrix(Ktmp, Eye{mtype}(x.out_dim), x) | ||
end | ||
|
||
function kernelkronmat( | ||
k::IntrinsicCoregionMOKernel, x::MOI, y::MOI | ||
) where {MOI<:MOInputsUnion} | ||
@assert x.out_dim == y.out_dim | ||
Ktmp = kernelmatrix(k.kernel, x.x, y.x) | ||
return _kroneckerkernelmatrix(Ktmp, k.B, x) | ||
end | ||
|
||
function kernelkronmat(k::MOK, x::MOI) where {MOI<:MOInputsUnion,MOK<:MOKernel} | ||
@assert iskroncompatible(κ) "The chosen kernel is not compatible for Kronecker matrices" | ||
return kernelkronmat(k, x, x) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is going to be (somewhat) less efficient than separate implementations that then only call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that is, do we want separate methods for the sake of optimal efficiency? @willtebbutt There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would probably be nice, but I'm not going to insist on it going in in this PR provided it gets sorted in a follow up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@Crown421 if you don't want to do it in this PR, please open a new issue for it instead then: ) |
||
end |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -23,18 +23,44 @@ struct IndependentMOKernel{Tkernel<:Kernel} <: MOKernel | |||||
kernel::Tkernel | ||||||
end | ||||||
|
||||||
# kernel function should be symmetric | ||||||
# would really like (κ::IndependentMOKernel)((x, px)::Tuple{T,Int}, (y, py)::Tuple{T,Int}) where T, but seems to cause autodiff problems | ||||||
Crown421 marked this conversation as resolved.
Show resolved
Hide resolved
Crown421 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
function (κ::IndependentMOKernel)((x, px)::Tuple{Any,Int}, (y, py)::Tuple{Any,Int}) | ||||||
if px == py | ||||||
return κ.kernel(x, y) | ||||||
else | ||||||
return 0.0 | ||||||
end | ||||||
return κ.kernel(x, y) * (px == py) | ||||||
Crown421 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
end | ||||||
|
||||||
function _kronkernelmatrix(Ktmp, B, ::MOInputIsotopicByFeatures) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's starting to get a bit confusing with _kronkernelmatrix, _kroneckerkernelmatrix, kernelkronmat... how about at least clarifying in the method name that this is multioutput-specific? e.g.
Suggested change
(moving the argument on which we actually dispatch to the front) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not quite happy with this being a different pattern from kernelkronmat when they should otherwise be the same, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @st-- regarding argument names in particular -- |
||||||
return kron(Ktmp, B) | ||||||
end | ||||||
|
||||||
function kernelmatrix(k::IndependentMOKernel, x::MOInput, y::MOInput) | ||||||
function _kronkernelmatrix(Ktmp, B, ::MOInputIsotopicByOutputs) | ||||||
return kron(B, Ktmp) | ||||||
end | ||||||
Crown421 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
function kernelmatrix(k::IndependentMOKernel, x::MOI, y::MOI) where {MOI<:MOInputsUnion} | ||||||
@assert x.out_dim == y.out_dim | ||||||
temp = k.kernel.(x.x, permutedims(y.x)) | ||||||
return cat((temp for _ in 1:(y.out_dim))...; dims=(1, 2)) | ||||||
Ktmp = kernelmatrix(k.kernel, x.x, y.x) | ||||||
mtype = eltype(Ktmp) | ||||||
return _kronkernelmatrix(Ktmp, Eye{mtype}(x.out_dim), x) | ||||||
end | ||||||
|
||||||
if VERSION >= v"1.6" | ||||||
function _kronkernelmatrix!(K, Ktmp, B, ::MOInputIsotopicByFeatures) | ||||||
return kron!(K, Ktmp, B) | ||||||
end | ||||||
Crown421 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
function _kronkernelmatrix!(K, Ktmp, B, ::MOInputIsotopicByOutputs) | ||||||
return kron!(K, B, Ktmp) | ||||||
end | ||||||
Crown421 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
function kernelmatrix!( | ||||||
K::AbstractMatrix, k::IndependentMOKernel, x::MOI, y::MOI | ||||||
) where {MOI<:MOInputsUnion} | ||||||
@assert x.out_dim == y.out_dim | ||||||
Ktmp = kernelmatrix(k.kernel, x.x, y.x) | ||||||
mtype = eltype(Ktmp) | ||||||
return _kronkernelmatrix!(K, Ktmp, Matrix{mtype}(I, x.out_dim, x.out_dim), x) | ||||||
end | ||||||
end | ||||||
|
||||||
function Base.show(io::IO, k::IndependentMOKernel) | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -34,10 +34,36 @@ function IntrinsicCoregionMOKernel(; kernel::Kernel, B::AbstractMatrix) | |||||
return IntrinsicCoregionMOKernel{typeof(kernel),typeof(B)}(kernel, B) | ||||||
end | ||||||
|
||||||
function IntrinsicCoregionMOKernel(kernel::Kernel, B::AbstractMatrix) | ||||||
return IntrinsicCoregionMOKernel{typeof(kernel),typeof(B)}(kernel, B) | ||||||
end | ||||||
st-- marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
function (k::IntrinsicCoregionMOKernel)((x, px)::Tuple{Any,Int}, (y, py)::Tuple{Any,Int}) | ||||||
return k.B[px, py] * k.kernel(x, y) | ||||||
end | ||||||
|
||||||
function matrixkernel(k::IntrinsicCoregionMOKernel, x, y) | ||||||
return matrixkernel(k, x, y; outputsize=size(k.B, 1)) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor suggestion:
Suggested change
doesn't make any difference as |
||||||
end | ||||||
|
||||||
function kernelmatrix( | ||||||
k::IntrinsicCoregionMOKernel, x::MOI, y::MOI | ||||||
) where {MOI<:MOInputsUnion} | ||||||
@assert x.out_dim == y.out_dim | ||||||
Ktmp = kernelmatrix(k.kernel, x.x, y.x) | ||||||
return _kronkernelmatrix(Ktmp, k.B, x) | ||||||
end | ||||||
|
||||||
if VERSION >= v"1.6" | ||||||
function kernelmatrix!( | ||||||
K::AbstractMatrix, k::IntrinsicCoregionMOKernel, x::MOI, y::MOI | ||||||
) where {MOI<:MOInputsUnion} | ||||||
@assert x.out_dim == y.out_dim | ||||||
Ktmp = kernelmatrix(k.kernel, x.x, y.x) | ||||||
return _kronkernelmatrix!(K, Ktmp, k.B, x) | ||||||
end | ||||||
end | ||||||
|
||||||
function Base.show(io::IO, k::IntrinsicCoregionMOKernel) | ||||||
return print( | ||||||
io, "Intrinsic Coregion Kernel: ", k.kernel, " with ", size(k.B, 1), " outputs" | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,8 @@ | ||
@doc raw""" | ||
LinearMixingModelKernel(g, e::MOKernel, A::AbstractMatrix) | ||
LinearMixingModelKernel(k::Kernel, H::AbstractMatrix) | ||
LinearMixingModelKernel(Tk::AbstractVector{<:Kernel},Th::AbstractMatrix) | ||
|
||
Kernel associated with the linear mixing model. | ||
Kernel associated with the linear mixing model, taking a vector of `m` kernels and a `m × p` matrix H for a function with `p` outputs. Also accepts a single kernel `k` for use across all `m` basis vectors. | ||
|
||
# Definition | ||
|
||
|
@@ -21,6 +22,7 @@ struct LinearMixingModelKernel{Tk<:AbstractVector{<:Kernel},Th<:AbstractMatrix} | |
K::Tk | ||
H::Th | ||
end | ||
# does it maybe make sense to check that length(K) and size(H,1) are equal? | ||
Crown421 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
function LinearMixingModelKernel(k::Kernel, H::AbstractMatrix) | ||
return LinearMixingModelKernel(Fill(k, size(H, 1)), H) | ||
|
@@ -32,6 +34,10 @@ function (κ::LinearMixingModelKernel)((x, px)::Tuple{Any,Int}, (y, py)::Tuple{A | |
return sum(κ.H[i, px] * κ.K[i](x, y) * κ.H[i, py] for i in 1:length(κ.K)) | ||
end | ||
|
||
function matrixkernel(k::LinearMixingModelKernel, x, y) | ||
Crown421 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return matrixkernel(k, x, y; outputsize=size(k.H, 2)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how does this not turn into an infinite loop ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the inner function has/ requires a kwarg There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, didn't realize dispatch depended on kwargs as well, I thought it was only on the positional args... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this seems to be a bug (JuliaLang/julia#9498) that is only not fixed yet because it would break backwards compatibility - I would prefer not to abuse implementation details that go counter to what the Julia docs say. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of the layers of indirection, how about matrixkernel(k::IndependentMOKernel, x, y) = k.kernel(x, y) * I
matrixkernel(k::IntrinsicCoregionMOKernel, x, y) = k.kernel(x, y) * k.B etc.? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting, I have been doing this for a while, good to know that it will go away at some point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Unfortunately, |
||
end | ||
|
||
function Base.show(io::IO, k::LinearMixingModelKernel) | ||
return print(io, "Linear Mixing Model Multi-Output Kernel") | ||
end | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -4,3 +4,16 @@ | |||||
Abstract type for kernels with multiple outpus. | ||||||
""" | ||||||
abstract type MOKernel <: Kernel end | ||||||
|
||||||
""" | ||||||
matrixkernel(k::MOK, x, y) | ||||||
matrixkernel(k::IndependentMOKernel, x, y(; outputsize)) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be nice to be consistent with the MOInput* types and call it |
||||||
|
||||||
Convenience function to compute the matrix kernel for two inputs `x` and `y`. The `outputsize` keyword is only required for the `IndependentMOKernel` to indicated the number of outputs. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: line length Additionally, it would be helpful to add a doctest here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have unfortunately never used documenter, will need to read up on this. Style should be fixed by formatter now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Also, how about instead just having matrixkernel(k::IndependentMOKernel, x, y) = error("You must provide the number of outputs, call `matrixkernel(k, x, y, out_dim)`")
matrixkernel(k::IndependentMOKernel, x, y, out_dim) = ... |
||||||
""" | ||||||
function matrixkernel(k::MOK, x, y; outputsize) where {MOK<:MOKernel} | ||||||
Crown421 marked this conversation as resolved.
Show resolved
Hide resolved
Crown421 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
@assert size(x) == size(y) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
xMO = MOInputIsotopicByFeatures([x], outputsize) | ||||||
yMO = MOInputIsotopicByFeatures([y], outputsize) | ||||||
return kernelmatrix(k, xMO, yMO) | ||||||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
# this script should be removed when the alternative MO kernelmatrix is accepted or rejected. | ||
|
||
using KernelFunctions, BenchmarkTools | ||
using LinearAlgebra | ||
|
||
mrank = 1 | ||
dims = (in=5, out=3) | ||
x = [rand(dims.in) for _ in 1:20] | ||
|
||
xMOF = KernelFunctions.MOInputIsotopicByFeatures(x, dims.out) | ||
xMOO = KernelFunctions.MOInputIsotopicByOutputs(x, dims.out) | ||
|
||
indk = IndependentMOKernel(GaussianKernel()) | ||
|
||
Kind1 = kernelmatrix(indk, xMOF, xMOF) | ||
Kind2 = kernelmatrix2(indk, xMOF, xMOF) | ||
|
||
Kind1 ≈ Kind2 | ||
willtebbutt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# true | ||
|
||
@benchmark kernelmatrix($indk, $xMOF, $xMOF) | ||
# BenchmarkTools.Trial: 756 samples with 1 evaluation. | ||
# Range (min … max): 6.186 ms … 11.470 ms ┊ GC (min … max): 0.00% … 35.30% | ||
# Time (median): 6.423 ms ┊ GC (median): 0.00% | ||
# Time (mean ± σ): 6.614 ms ± 806.792 μs ┊ GC (mean ± σ): 2.76% ± 7.77% | ||
|
||
# ▅▇█▇▃ | ||
# ▆██████▅▆▄▅▄▁▄▁▄▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▄▁▁▁▁▁▁▁▁▁▁▁▁▁▄▁▁▁▁▁▁▆██▇▇ ▇ | ||
# 6.19 ms Histogram: log(frequency) by time 10 ms < | ||
|
||
# Memory estimate: 2.34 MiB, allocs estimate: 60060. | ||
|
||
@benchmark kernelmatrix2($indk, $xMOF, $xMOF) | ||
# BenchmarkTools.Trial: 10000 samples with 5 evaluations. | ||
# Range (min … max): 6.162 μs … 341.226 μs ┊ GC (min … max): 0.00% … 96.29% | ||
# Time (median): 6.947 μs ┊ GC (median): 0.00% | ||
# Time (mean ± σ): 8.235 μs ± 16.802 μs ┊ GC (mean ± σ): 12.37% ± 5.92% | ||
|
||
# ▁▄▆▇██▇▆▅▄▃▂▁▁▁ ▂ | ||
# ▅██████████████████▇▇▆▇▅▆▆▆▅▅▆▅▅▃▄▅▅▅▁▁▃▁▃▅▅▆▆▆▇▇▇▇▇▇▇██▇▇▇ █ | ||
# 6.16 μs Histogram: log(frequency) by time 13.7 μs < | ||
|
||
# Memory estimate: 34.89 KiB, allocs estimate: 7. | ||
|
||
A = randn(dims.out, mrank) | ||
B = A * transpose(A) + Diagonal(rand(dims.out)) | ||
|
||
ickernel = IntrinsicCoregionMOKernel(GaussianKernel(), B) | ||
|
||
Kic1 = kernelmatrix(ickernel, xMOF, xMOF) | ||
Kic2 = kernelmatrix2(ickernel, xMOF, xMOF) | ||
|
||
Kic1 ≈ Kic2 | ||
#true | ||
|
||
@benchmark kernelmatrix($ickernel, $xMOF, $xMOF) | ||
# BenchmarkTools.Trial: 1874 samples with 1 evaluation. | ||
# Range (min … max): 2.522 ms … 5.424 ms ┊ GC (min … max): 0.00% … 51.13% | ||
# Time (median): 2.601 ms ┊ GC (median): 0.00% | ||
# Time (mean ± σ): 2.666 ms ± 369.030 μs ┊ GC (mean ± σ): 2.01% ± 7.04% | ||
|
||
# ▂█▆▁ ▁ | ||
# ███████▅▄▅▅▃▁▁▃▅▁▁▁▃▃▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▇▇ █ | ||
# 2.52 ms Histogram: log(frequency) by time 570 ms < | ||
|
||
# Memory estimate: 985.47 KiB, allocs estimate: 39639. | ||
|
||
@benchmark kernelmatrix2($ickernel, $xMOF, $xMOF) | ||
# BenchmarkTools.Trial: 10000 samples with 5 evaluations. | ||
# Range (min … max): 6.152 μs … 322.002 μs ┊ GC (min … max): 0.00% … 96.14% | ||
# Time (median): 6.676 μs ┊ GC (median): 0.00% | ||
# Time (mean ± σ): 8.100 μs ± 16.959 μs ┊ GC (mean ± σ): 12.53% ± 5.85% | ||
|
||
# ▄▇██▇▅▄▃▂▁▁ ▁▁▁▁ ▂ | ||
# ▆██████████████▇▇▇▆▆▇▇▇▇▆▇▆▄▅▄▃▆▁▅▄▅▆▆▅▅▄▅▄▆▇▇█▇▇▇▇██████▇▇ █ | ||
# 6.15 μs Histogram: log(frequency) by time 13.5 μs < | ||
|
||
# Memory estimate: 34.77 KiB, allocs estimate: 6. |
Uh oh!
There was an error while loading. Please reload this page.