Support generic arrays #706
Conversation
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
|
There was a problem hiding this comment.
Thanks @amontoison for the PR, that looks great!
Could you give us a bit more details about the PR?
If I understand correctly you:
- replace
S(undef, n)bysimilar(S, n) - remove some
axpy!(s, x, y)andkaxpy!(n, Complex{T}(s), x, dx, y, dy)(not sure why though?) - Add a specific constructor if the right-hand side is not a
DenseVector
It probably misses some unit tests showing/testing the new data types handled by these changes.
src/krylov_solvers.jl
Outdated
| m, n = size(A) | ||
| S = ktypeof(b) | ||
| MinresSolver(m, n, S, window=window) | ||
| if S <: DenseVector | ||
| ixm, ixn = m, n | ||
| else | ||
| ixm, ixn = axes(A) | ||
| end |
There was a problem hiding this comment.
Shouldn't we have a method and use a dispatch for this instead?
There was a problem hiding this comment.
Good idea, I added a function kaxes.
"""
ixm, ixn = kaxes(S, A)
Return the size of `A` if `S` is a `DenseVector`.
Otherwise, it returns the axes of `A`.
`axes(A)` could not be defined for some linear operators and we only want to call it for fancy arrays.
"""
function kaxes end
kaxes(::Type{S}, A) where S <: DenseVector = size(A)
kaxes(::Type{S}, A) where S <: AbstractVector = axes(A)Thanks to kaxes, the new version of Krylov.jl will be backward compatible with matrices / linear operators A that don't support axes(A).
It will insure that we don't break any code working with Krylov.jl v"9.0".
There was a problem hiding this comment.
We could define axes for linear operarors maybe?!
There was a problem hiding this comment.
It's already defined for LinearOperators.jl or LinearMaps.jl but if a user defined a custom structure to model a linear operator in the past, the new release will break its code if axes(A) was not defined.
src/krylov_utils.jl
Outdated
| kaxpy!(n :: Integer, s :: T, x :: AbstractVector{T}, dx :: Integer, y :: AbstractVector{T}, dy :: Integer) where T <: FloatOrComplex = (y .+= s .* x) # axpy!(s, x, y) | ||
| kaxpy!(n :: Integer, s :: T, x :: AbstractVector{Complex{T}}, dx :: Integer, y :: AbstractVector{Complex{T}}, dy :: Integer) where T <: AbstractFloat = (y .+= s .* x) # kaxpy!(n, Complex{T}(s), x, dx, y, dy) | ||
|
|
||
| kaxpby!(n :: Integer, s :: T, x :: Vector{T}, dx :: Integer, t :: T, y :: Vector{T}, dy :: Integer) where T <: BLAS.BlasFloat = BLAS.axpby!(n, s, x, dx, t, y, dy) | ||
| kaxpby!(n :: Integer, s :: T, x :: AbstractVector{T}, dx :: Integer, t :: T, y :: AbstractVector{T}, dy :: Integer) where T <: FloatOrComplex = axpby!(s, x, t, y) | ||
| kaxpby!(n :: Integer, s :: T, x :: AbstractVector{Complex{T}}, dx :: Integer, t :: Complex{T}, y :: AbstractVector{Complex{T}}, dy :: Integer) where T <: AbstractFloat = kaxpby!(n, Complex{T}(s), x, dx, t, y, dy) | ||
| kaxpby!(n :: Integer, s :: Complex{T}, x :: AbstractVector{Complex{T}}, dx :: Integer, t :: T, y :: AbstractVector{Complex{T}}, dy :: Integer) where T <: AbstractFloat = kaxpby!(n, s, x, dx, Complex{T}(t), y, dy) | ||
| kaxpby!(n :: Integer, s :: T, x :: AbstractVector{Complex{T}}, dx :: Integer, t :: T, y :: AbstractVector{Complex{T}}, dy :: Integer) where T <: AbstractFloat = kaxpby!(n, Complex{T}(s), x, dx, Complex{T}(t), y, dy) | ||
| kaxpby!(n :: Integer, s :: T, x :: AbstractVector{T}, dx :: Integer, t :: T, y :: AbstractVector{T}, dy :: Integer) where T <: FloatOrComplex = (y .= s .* x .+ t .* y) # axpby!(s, x, t, y) | ||
| kaxpby!(n :: Integer, s :: T, x :: AbstractVector{Complex{T}}, dx :: Integer, t :: Complex{T}, y :: AbstractVector{Complex{T}}, dy :: Integer) where T <: AbstractFloat = (y .= s .* x .+ t .* y) # kaxpby!(n, Complex{T}(s), x, dx, t, y, dy) | ||
| kaxpby!(n :: Integer, s :: Complex{T}, x :: AbstractVector{Complex{T}}, dx :: Integer, t :: T, y :: AbstractVector{Complex{T}}, dy :: Integer) where T <: AbstractFloat = (y .= s .* x .+ t .* y) # kaxpby!(n, s, x, dx, Complex{T}(t), y, dy) | ||
| kaxpby!(n :: Integer, s :: T, x :: AbstractVector{Complex{T}}, dx :: Integer, t :: T, y :: AbstractVector{Complex{T}}, dy :: Integer) where T <: AbstractFloat = (y .= s .* x .+ t .* y) # kaxpby!(n, Complex{T}(s), x, dx, Complex{T}(t), y, dy) |
There was a problem hiding this comment.
Yes, I found a neat solution.
Yes, I can give more details about the pull request. With this PR, I change how the vectors in the Krylov workspaces are initialized and allocated.
I will add a buildkite build that will test that the new data types are handled by these changes. |
Codecov ReportBase: 98.22% // Head: 98.20% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #706 +/- ##
==========================================
- Coverage 98.22% 98.20% -0.03%
==========================================
Files 38 38
Lines 6659 6695 +36
==========================================
+ Hits 6541 6575 +34
- Misses 118 120 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
src/krylov_utils.jl
Outdated
|
|
||
| Return the size of `A` if `S` is a subtype of `DenseVector`. | ||
| Otherwise, it returns the axes of `A`. | ||
| `axes(A)` could not be defined for some linear operators and we only want to call it for fancy arrays. |
There was a problem hiding this comment.
| `axes(A)` could not be defined for some linear operators and we only want to call it for fancy arrays. | |
| `axes(A)` may not be defined for some linear operators and we only want to call it for fancy arrays. |
I think it would be reasonable to require axes() to be implemented. It's part of the array API. You can recover the dimensions with length.(axes(A)), which works for dense arrays too.
There was a problem hiding this comment.
A can be any structure that models a matrix of size m x n. I'm not sure that it's always relevant to define axes.
I have an example in mind:
structure MyJacobian{T} where {T}
m::Int
n::Int
F::Function
MyJacobian is only defined such that mul!(y::Vector{T}, A::MyJacobian{T}, x::Vector{T}) can be overloaded and mul! performs Jacobian-vector products with automatic differentiation tools.
The user will probably never implement axes(A::MyJacobian).
There was a problem hiding this comment.
What I'm saying is that it's not for Krylov.jl to try to guess what the implementation of axes() should be for a user type. We could simply document that users must implement axes().
There was a problem hiding this comment.
We can but it's a breaking change. The next release of Krylov.jl must be v"10.0" if we use axes instead of kaxes.
There was a problem hiding this comment.
why? you just introduced kaxes, didn't you?
There was a problem hiding this comment.
Yes, I introduced kaxes to avoid that but you suggest to just use axes directly, am I wrong?
axes or kaxes are called when we create a KrylovSolver, which means every time that we solve a linear problem with a method that is not in-place such as cg(A, b).
Because only S <: DenseVector was supported before, I'm sure to not break any code if kaxes dispatches to size.
amontoison
force-pushed
the
support_generic_arrays
branch
from
1c79f02
to
f00e46b
Compare
|
Because 0 is an integer, similar(S, 0) can't be used for some storage types that are not a subtype of a DenseVector. I must allocate all optional vectors required for warm-start, regularization or preconditioners in the Krylov workspaces. Thanks for the details. Isn't that strange the empty vector is not defined even for sparse arrays ? In the worst-case could you do |
I don't understand your comment Tangi. 1 is also an integer like 0. The problem is not the value 0, I just want to create a vector that satisfy For the sparse arrays, I will add a check to insure that |
amontoison
force-pushed
the
support_generic_arrays
branch
from
a346c40
to
aff18f8
Compare
Co-authored-by: tmigot <[email protected]>
Ok, I didn't understand the problem at first. Do you have an example that would not work? |
Yes, I do. using BlockArrays, Krylov
T = Float64
n = 10
x = BlockVector(rand(T,n), [n-2,2])
S = Krylov.ktypeof(x)
similar(S, 0) |
Well... that won't really help but you could create an issue on this package and on the packages where the empty vector is not defined. To me, it looks like a very natural property for any vector type, |
close #605