-
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
Add LazyKernelMatrix and lazykernelmatrix #515
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #515 +/- ##
==========================================
- Coverage 94.16% 87.64% -6.52%
==========================================
Files 52 53 +1
Lines 1387 1433 +46
==========================================
- Hits 1306 1256 -50
- Misses 81 177 +96
☔ View full report in Codecov by Sentry. |
Yes, I imagine this would be useful also in the context of #93 (comment). Though there maybe often the non-lazy version might be sufficient (or even preferable). |
@@ -1,6 +1,7 @@ | |||
module KernelFunctions | |||
|
|||
export kernelmatrix, kernelmatrix!, kernelmatrix_diag, kernelmatrix_diag! | |||
export LazyKernelMatrix, lazykernelmatrix |
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.
Do we have to export both? Is lazykernelmatrix
sufficient maybe?
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.
Probably.
It looks really nice already! Only thing which is a bit uneasy is the output type... I am not sure there is a strong guarantee that the first evaluated type would correspond to the rest of the matrix. But I also don't see how it could be solved easily... |
The result is a matrix with the same entries as [`kernelmatrix(κ, x)`](@ref) but where the | ||
entries are not computed until they are needed. | ||
""" | ||
lazykernelmatrix(κ::Kernel, x) = lazykernelmatrix(κ, x, x) |
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.
It would be good to optimize this for the symmetric case, IMO, similar to kernelmatrix
(which IIRC often does not use such a fallback but more optimized methods).
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 optimization for the symmetric case is only calculating half the matrix lets say the top half and then redirecting all queries from the bottom half to the top half. (Actually distances simply copies the top half into the bottom half).
Since this is lazy, there is probably no point in this optimization because you do not do the calculation from the start. And when you call getindex it does not matter whether you calculate the element in the top or bottom half.
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.
At least with other lazy iterators in Base it's a common pattern to collect
results at some point (e.g., after filtering, mapping, etc.). In this case it seems beneficial to know that the lazy matrix is symmetric.
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.
Good point, I'll see if there are ops we can optimize without too much extra code complexity.
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 an interface for that? I mean you could use LinearAlgebra.Symmetric since that just wraps the original matrix afaik, but it also simply redirects queries so collect would still cause two calculations since you do a calculation per query.
I mean you could just specialize collect I guess
Relevant PartFor this a lazy ProductArray would also be a neat abstraction: julia> v = rand(3)
3-element Vector{Float64}:
0.417571623820013
0.39972694171008405
0.9970727095536318
julia> productArray(v,v)
3×3 ProductArrays.ProductArray{Tuple{Vector{Float64}, Vector{Float64}}, Tuple{Float64, Float64}, 2}:
(0.417572, 0.417572) (0.417572, 0.399727) (0.417572, 0.997073)
(0.399727, 0.417572) (0.399727, 0.399727) (0.399727, 0.997073)
(0.997073, 0.417572) (0.997073, 0.399727) (0.997073, 0.997073) now the lazy kernelmatrix is simply a lazy mappedarray of this product array. As mappedarray has a field for its mapping: struct ReadonlyMappedArray{T,N,A<:AbstractArray,F} <: AbstractMappedArray{T,N}
f::F
data::A
end you can write custom code for when Synergy tangentThis is neat, because the productArray abstraction is also helpful as a multioutput abstraction: julia> vec(productArray(v,1:2))
6-element reshape(::ProductArrays.ProductArray{Tuple{Vector{Float64}, UnitRange{Int64}}, Tuple{Float64, Int64}, 2}, 6) with eltype Tuple{Float64, Int64}:
(0.417571623820013, 1)
(0.39972694171008405, 1)
(0.9970727095536318, 1)
(0.417571623820013, 2)
(0.39972694171008405, 2)
(0.9970727095536318, 2) cf. https://github.com/lazyLibraries/ProductArrays.jl (not yet a package JuliaRegistries/General#84683) |
I have the same feeling as @sethaxen:
A separate dedicated type provides more information about the context and allows us to define dispatches, special operations and optimizations that might be less relevant or not well defined in the general case. |
I edited my reply to explain how you can still do special dispatches |
I actually found this pull request because I wanted to ask: does it ever make sense for kernelmatrix to be eager? I mean memory access is expensive and you take |
I wonder of it's safe to ask Julia to infer the return type of |
mappedarrays does eltype inference 🤷 |
A separate option would be to be a little less ambitious with this initial implementation, and not implement a new matrix type, and just provide an interface for the operation that we want. Specifically, add the following function to the interface: function kernel_matrix_vector_product(k::Kernel, x::AbstractVector, y::AbstractVector, v::AbstractVector{<:Real})
return kernelmatrix(k, x, y) * v
end
kernel_matrix_vector_product(k::Kernel, x::AbstractVector, v::AbstractVector{<:Real}) = kernelmatrix(k, x) * v where the above methods are the default implementations and specify the semantics. The nice thing about doing things this way is that we avoid having to e.g. guess at the output type of Does this cover our needs? I guess really I'm just asking whether we actually need to go to the trouble of implementing a new matrix type, and can instead just implement a single function that can be overloaded. If there is a range of functionality that we require, then maybe a matrix type is needed, but if we really only have one operation in mind, maybe it's not? edit: or we could add an additional argument to the functioon that says how things should be computed when you're doing block-wise operations. e.g. |
ProductArrays v1.0.0 is now online, so you could do using MappedArrays: mappedarray, ReadonlyMappedArray
using ProductArrays: productArray, ProductArray
struct Splat{T}
func::T
end
(s::Splat)(x) = s.func(x...)
lazykernelmatrix(k::Kernel, x, y) = mappedarray(Splat(k), productArray(x,y))
const LazyKernelMatrix{K<:Kernel, T<:Real, P<:ProductArray} = ReadonlyMappedArray{T, 2, P, Splat{K}} and then implement additional functionality for julia> a = mappedarray(Splat(k), productArray(x,x))
3×3 mappedarray(Splat{SqExponentialKernel{Distances.Euclidean}}(Squared Exponential Kernel (metric = Distances.Euclidean(0.0))), ::ProductArray{Tuple{Vector{Float64}, Vector{Float64}}, Tuple{Float64, Float64}, 2}) with eltype Float64:
1.0 0.879512 0.998656
0.879512 1.0 0.901716
0.998656 0.901716 1.0
julia> a isa LazyKernelMatrix
true
julia> eltype(a)
Float64
julia> a isa AbstractArray
true
julia> dump(a) # very readable structure
ReadonlyMappedArray{Float64, 2, ProductArray{Tuple{Vector{Float64}, Vector{Float64}}, Tuple{Float64, Float64}, 2}, Splat{SqExponentialKernel{Distances.Euclidean}}}
f: Splat{SqExponentialKernel{Distances.Euclidean}}
func: SqExponentialKernel{Distances.Euclidean}
metric: Distances.Euclidean
thresh: Float64 0.0
data: ProductArray{Tuple{Vector{Float64}, Vector{Float64}}, Tuple{Float64, Float64}, 2}
prodIt: Base.Iterators.ProductIterator{Tuple{Vector{Float64}, Vector{Float64}}}
iterators: Tuple{Vector{Float64}, Vector{Float64}}
1: Array{Float64}((3,)) [0.6995475228324576, 0.19281640447854786, 0.6476909300916908]
2: Array{Float64}((3,)) [0.6995475228324576, 0.19281640447854786, 0.6476909300916908] if you want I can also write a convenience method to skip |
Summary
This PR introduces functionality for lazily representing kernel matrices, which is necessary when the matrix might be too large to store in memory. Fixes #514
Proposed changes
lazykernelmatrix
: supports similar semantics askernelmatrix
but constructs a lazy representationAbstractMatrix
subtypeLazyKernelMatrix
, constructed for thelazykernelmatrix
default.What alternatives have you considered?
lazykernelmatrix
, but this could allow even more structured matrix types to be defined for specific kernel types and returned in the future.LazyKernelMatrix
should also storeobsdim1
andobsdim2
? Currently we require the user has passed a vector e.g.RowVecs
orColVecs
.y
being anothing
to define a symmetric kernel matrix? This would allow a couple checks to be done at compile time. In particular we could support+(::LazyKernelMatrix{T,Tk,Tx,Nothing}, ::Diagonal) -> LazyKernelMatrix
.To-Do