-
Notifications
You must be signed in to change notification settings - Fork 55
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
Support generic arrays #706
Support generic arrays #706
Conversation
Co-authored-by: Dominique <dominique.orban@gmail.com>
Co-authored-by: Dominique <dominique.orban@gmail.com>
Co-authored-by: Dominique <dominique.orban@gmail.com>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? you just introduced kaxes
, didn't you?
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.
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
.
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 |
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.
A few more comments, and then looks good to me, thanks!
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 |
a346c40
to
aff18f8
Compare
Co-authored-by: tmigot <tangi.migot@gmail.com>
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