-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Towards #12251 and #12153, reimplement all full(X) method defs. as convert(Array, X) and add convert(AbstractArray, X) methods for Factorizations #17066
Conversation
full(A::LQ) = convert(Array, A) | ||
|
||
function convert{T}(::Type{Matrix}, A::LQPackedQ{T}; thin::Bool = true) | ||
#= We construct the full eye here, even though it seems ineffecient, because |
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.
inefficient
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.
Fixed. Thanks!
Cool. Could you add tests for the new |
Thanks for the review!
Apologies for not clearly explaining this pull request's purpose above. Deprecating
Rationale for this partitioning: Completing (1) upstream of (2) upstream of (3): significantly eases (2); minimizes commit-to-commit discontinuities in functionality and testing, helping to avoid introducing regressions in existing functionality; and hopefully simplifies review by separating logically disjoint transformations.
If migrating the |
Adding untested features isn't generally a good idea. I guess you could copy the existing tests for |
Agreed. The added Edit: Thought this through while migrating the tests in #17079. All
would not impact coverage, only increase code churn unfortunately. As such keeping test migration in #17079 alongside the other |
ip = invperm(F[:p]) | ||
return (F[:L] * F[:U])[ip,ip] | ||
end | ||
convert(::Type{Matrix}, C::Cholesky) = C.uplo == 'U' ? C[:U]'C[:U] : C[:L]*C[:L]' |
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 issue here is that the result might not be a Matrix
since Cholesky
is parametric on the array type.
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 catch! What would you suggest? I could wrap the result in another convert(Array, )
call?
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.
For each array-parametic type that originally had a full
definition not necessarily returning an Array
(e.g. that you highlighted, and the array-parametric factorization types generally), I've added a wrapping convert(Array, )
to ensure the return will now be an Array
.
The pattern for LDLt
convert(::Type{SymTridiagonal}, F::LDLt) = ...
convert(::Type{AbstractMatrix}, F::LDLt) = convert(SymTridiagonal, F)
convert(::Type{AbstractArray}, F::LDLt) = convert(AbstractMatrix, F)
convert(::Type{Matrix}, F::LDLt) = convert(Array, convert(AbstractArray, F))
convert(::Type{Array}, F::LDLt) = convert(Matrix, F)
full(F::LDLt) = convert(Array, F)
is where the methods for these cases should ultimately go I think, solving #12153. Another PR though. Thanks!
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.
I think something like this is the right approach. Basically, the convert(::Type{AbstractMatrix}, F::<:Factorization)
should be what full(F::Factorization)
is now. The version for Matrix
could then probably be defined once with
convert(::Type{Matrix}, F::Factorization) = convert(Matrix, convert(AbstractMatrix, F))
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.
Cheers, we're on the same page then. Are you advocating for additionally making those changes (i.e. attacking #12153 simultaneously) in this PR, rather than in a later PR? Thanks!
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.
I've reworked the PR to simultaneously address #12153 as discussed just above. Thanks! I hope JuliaCon is going well!
convert(::Type{Array}, C::Cholesky) = convert(Matrix, C) | ||
full(C::Cholesky) = convert(Array, C) | ||
|
||
convert(::Type{Matrix}, F::CholeskyPivoted) = (ip = invperm(F[:p]); convert(Array, (F[:L] * F[:U])[ip,ip])) |
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.
I liked this better in the multi-line form
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.
That line is simplified / shortened in the most recent version. Do you still prefer the multiline form? If so I'll break it down. Thanks!
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's still multiple statements, assigning a temporary variable then using it - worth doing those on separate lines IMO
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.
Multiline it is, and done! :)
""" | ||
full(S) | ||
|
||
Convert a sparse matrix or vector `S` into a dense matrix or vector. | ||
""" | ||
full | ||
|
||
function full{Tv}(S::SparseMatrixCSC{Tv}) | ||
# Handle cases where zero(Tv) is not defined but the array is dense. | ||
# (Should we really worry about this?) |
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.
this is kind of useful to keep
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 call. Reintroduced. Thanks!
…trixCSC) chain, and reimplement full(SparseMatrixCSC) as a synonymous child of convert(Array, SparseMatrixCSC).
…tractSparseVector) chain, and reimplement full(AbstractSparseVector) as a synonymous child of convert(Array, AbstractSparseVector).
…hain, and reimplement full(Bidiagonal) as a synonymous child of convert(Array, Bidiagonal).
…convert(Matrix, X) <- convert(Array, X) chains for Cholesky and CholeskyPivoted types, and reimplement full(X) for both types as synonymous children of convert(Array, X).
…, and reimplement full(Diagonal) as a synonymous child of convert(Array, Diagonal).
…gen) <- convert(Matrix, Eigen) <- convert(Array, Eigen) chain, and reimplement full(Eigen) as synonymous child of convert(Array, Eigen).
…y, Hessenberg) <- convert(Matrix, Hessenberg) <- convert(Array, Hessenberg) and convert(Matrix, HessenbergQ) <- convert(Array, HessenbergQ )chains, and reimplement full(X) for both types as synonmous children of convert(Array, X).
…Lt) <- convert(AbstractArray, LDLt) <- convert(Matrix, LDLt) <- convert(Array, LDLt) chain, and reimplement full(LDLt) as a synonymous child of convert(Array, LDLt).
…- convert(Matrix, LQ) <- convert(Array, LQ) and convert(Matrix, LQPackedQ) <- convert(Array, LQPackedQ) chains, and reimplement full(X) for both types as a synonymous child of convert(Array, X).
…onvert(Matrix, X) <- convert(Array, X) chains for LU and LU{T,Tridiagonal{T}} types, and reimplement full(X) for both types as synonymous children of convert(Array, X).
…stractMatrix, Union{QR,QRCompactWY}) <- convert(Matrix, Union{QR,QRCompactWY}) <- convert(Array, Union{QR,QRCompactWY}) and convert(Matrix, Union{QRPackedQ,QRCompactWYQ}) <- convert(Array, Union{QRPackedQ,QRCompactWYQ}) chains, and reimplement full(Union{QR,QRCompactWY}) and full(Union{QRPackedQ,QRCompactWYQ}) as synonymous children of convert(Array, Union{QR,QRCompactWY}) and convert(Array, Union{QRPackedQ,QRCompactWYQ}) respectively.
… Implements convert(Array, Union{Symmetric,Hermitian}) as a short child of the two preceding methods, and reimplements full(Union{Symmetric,Hermitian}) likewise.
…vert(Matrix, AbstractTriangular), and reimplements full(AbstractTriangular) as a short child of convert(Array, AbstractTriangular).
…nal) as synonymous children of convert(Matrix, Tridiagonal) and convert(Matrix, SymTridiagonal) respectively, and reimplements full(Tridiagonal) and full(SymTridiagonal) as short children of convert(Array, Tridiagonal) and convert(Array, SymTridiagonal) respectively.
just being paranoid - @nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
I suspect those two might be noise... |
Concur. Is this in good shape now, or have I missed anything? Thanks and best! |
Unfortunately, I've just realized that we still need to come up with a good solution for what used to be e.g. parenttype{T,S}(A::Hermitian{T,S}) = S
convert{T,S}(S,A::Hermitian{T,S}) = ...blablabla... |
It's not really the parent, it's the wrapped type. |
Can you elaborate? Don't you think that |
I guess it's the underlying array that the "view" is being taken of, though I think with triangular or symmetric matrices it would be more common (than with SubArray) to have situations where the internal array is never referenced as its own separate object. |
I see. This made me this a little more about this and I don't think the A = randn(4,4)
B = view(A,1:3,1:3)
C = Symmetric(B) Right now, |
@Sacha0 What are your current thoughts here? |
My thoughts echo yours. Something along the lines of julia> parenttype(A) = typeof(A) == typeof(parent(A)) ? typeof(A) : parenttype(parent(A))
parenttype (generic function with 1 method)
julia> foo = rand(3,3)
3×3 Array{Float64,2}:
0.645174 0.500884 0.85511
0.0901531 0.753874 0.346687
0.975905 0.920539 0.797743
julia> parenttype(Hermitian(foo))
Array{Float64,2}
julia> bar = view(foo, 1:2, 1:2)
2×2 SubArray{Float64,2,Array{Float64,2},Tuple{UnitRange{Int64},UnitRange{Int64}},false}:
0.645174 0.500884
0.0901531 0.753874
julia> parenttype(Hermitian(bar))
Array{Float64,2}
julia> baz = reshape(bar, 2, 2)
2×2 Base.ReshapedArray{Float64,2,SubArray{Float64,2,Array{Float64,2},Tuple{UnitRange{Int64},UnitRange{Int64}},false},Tuple{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64}}}:
0.645174 0.500884
0.0901531 0.753874
julia> parenttype(Hermitian(baz))
Array{Float64,2}
julia> qux = reshape(UpperTriangular(view(Hermitian(baz),1:2,1:2)),2,2)
2×2 Base.ReshapedArray{Float64,2,UpperTriangular{Float64,SubArray{Float64,2,Hermitian{Float64,Base.ReshapedArray{Float64,2,SubArray{Float64,2,Array{Float64,2},Tuple{UnitRange{Int64},UnitRange{Int64}},false},Tuple{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64}}}},Tuple{UnitRange{Int64},UnitRange{Int64}},false}},Tuple{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64}}}:
0.645174 0.500884
0.0 0.753874
julia> parenttype(qux)
Array{Float64,2} Edit: Maybe |
Also, thanks for the review and merge! Best! |
…7848) This commit will allow a user to call: convert(Vector, my_abstractvector_of_eltype_Foo) convert(Matrix, my_abstractmatrix_of_eltype_Foo) which is useful because - for the case that the variable is a subarray of the same dimensionality - for consistency with PR #17066 (cherry picked from commit bffeedb)
…nvert(Array, X) and add convert(AbstractArray, X) methods for Factorizations (JuliaLang#17066) * Implement convert(Matrix, SparseMatrixCSC) <- convert(Array, SparseMatrixCSC) chain, and reimplement full(SparseMatrixCSC) as a synonymous child of convert(Array, SparseMatrixCSC). * Implement convert(Vector, AbstractSparseVector) <- convert(Array, AbstractSparseVector) chain, and reimplement full(AbstractSparseVector) as a synonymous child of convert(Array, AbstractSparseVector). * Implement convert(Matrix, Bidiagonal) <- convert(Array, Bidiagonal) chain, and reimplement full(Bidiagonal) as a synonymous child of convert(Array, Bidiagonal). * Implement convert(AbstractMatrix, X) <- convert(AbstractArray, X) <- convert(Matrix, X) <- convert(Array, X) chains for Cholesky and CholeskyPivoted types, and reimplement full(X) for both types as synonymous children of convert(Array, X). * Implement convert(Matrix, Diagonal) <- convert(Array, Diagonal) chain, and reimplement full(Diagonal) as a synonymous child of convert(Array, Diagonal). * Implement convert(AbstractMatrix, Eigen) <- convert(AbstractArray, Eigen) <- convert(Matrix, Eigen) <- convert(Array, Eigen) chain, and reimplement full(Eigen) as synonymous child of convert(Array, Eigen). * Implement convert(AbstractMatrix, Hessenberg) <- convert(AbstractArray, Hessenberg) <- convert(Matrix, Hessenberg) <- convert(Array, Hessenberg) and convert(Matrix, HessenbergQ) <- convert(Array, HessenbergQ )chains, and reimplement full(X) for both types as synonmous children of convert(Array, X). * Implement convert(SymTridiagonal, LDLt) <- convert(AbstractMatrix, LDLt) <- convert(AbstractArray, LDLt) <- convert(Matrix, LDLt) <- convert(Array, LDLt) chain, and reimplement full(LDLt) as a synonymous child of convert(Array, LDLt). * Implement convert(AbstractMatrix, LQ) <- convert(AbstractArray, LQ) <- convert(Matrix, LQ) <- convert(Array, LQ) and convert(Matrix, LQPackedQ) <- convert(Array, LQPackedQ) chains, and reimplement full(X) for both types as a synonymous child of convert(Array, X). * Implement convert(AbstractMatrix, X) < convert(AbstractArray, X) <- convert(Matrix, X) <- convert(Array, X) chains for LU and LU{T,Tridiagonal{T}} types, and reimplement full(X) for both types as synonymous children of convert(Array, X). * Implement convert(AbstractArray, Union{QR,QRCompactWY}) <- convert(AbstractMatrix, Union{QR,QRCompactWY}) <- convert(Matrix, Union{QR,QRCompactWY}) <- convert(Array, Union{QR,QRCompactWY}) and convert(Matrix, Union{QRPackedQ,QRCompactWYQ}) <- convert(Array, Union{QRPackedQ,QRCompactWYQ}) chains, and reimplement full(Union{QR,QRCompactWY}) and full(Union{QRPackedQ,QRCompactWYQ}) as synonymous children of convert(Array, Union{QR,QRCompactWY}) and convert(Array, Union{QRPackedQ,QRCompactWYQ}) respectively. * Implement convert(AbstractMatrix, Schur) <- convert(AbstractArray, Schur) <- convert(Matrix, Schur) <- convert(Array, Schur) chain, and reimplement full(Schur) as a synonymous child of convert(Array, Schur). * Implement convert(AbstractMatrix, SVD) <- convert(AbstractArray, SVD) <- convert(Matrix, SVD) <- convert(Array, SVD) chain, and reimplement full(SVD) as a synonymous child of convert(Array, SVD). * Implements convert(Matrix, Symmetric) and convert(Matrix, Hermitian). Implements convert(Array, Union{Symmetric,Hermitian}) as a short child of the two preceding methods, and reimplements full(Union{Symmetric,Hermitian}) likewise. * Implements convert(Array, AbstractTriangular) as a short child of convert(Matrix, AbstractTriangular), and reimplements full(AbstractTriangular) as a short child of convert(Array, AbstractTriangular). * Implements convert(Array, Tridiagonal) and convert(Array, SymTridiagonal) as synonymous children of convert(Matrix, Tridiagonal) and convert(Matrix, SymTridiagonal) respectively, and reimplements full(Tridiagonal) and full(SymTridiagonal) as short children of convert(Array, Tridiagonal) and convert(Array, SymTridiagonal) respectively.
…liaLang#17848) This commit will allow a user to call: convert(Vector, my_abstractvector_of_eltype_Foo) convert(Matrix, my_abstractmatrix_of_eltype_Foo) which is useful because - for the case that the variable is a subarray of the same dimensionality - for consistency with PR JuliaLang#17066
First step towards JuliaLang/LinearAlgebra.jl#231 (edit: and now JuliaLang/LinearAlgebra.jl#229 as well) (edit: explanation and plan now described in #17066 (comment)). This pull request reduces every
full(X)
method definition toconvert(Array, X)
(apart from theAbstractArray
no-op fallback and theapprox_full
methods intest.jl
):This change should enable drop-in replacement of
full(X)
calls withconvert(Array, X)
throughoutbase/
, the next step towards JuliaLang/LinearAlgebra.jl#231. Some notes:For non-
Factorization
types, eachfull(X)
method is now part of a block likeor
such that generic methods may call
convert(Array, foo)
for arbitrary presently-full
-ablefoo
, and hence don't need to know whetherfoo
naturally converts to aMatrix
,Vector
, etc. In some cases parametricconvert{T}(::Type{Matrix{T}}, X)
methods already existed; this pull request preserves those methods and implements theconvert(::Type{Matrix}, X)
andconvert(::Type{Array}, X)
methods as children thereof. But it does not introduce such parametric definitions where they did not previously exist.Edit: For
Factorization
types, as a simultaneous attack on JuliaLang/LinearAlgebra.jl#229, eachfull(X)
method is now part of a block likewhere
convert(::Type{AbstractMatrix}, F::Cholesky)
recovers the original matrix, not necessarily as anArray
.For
Symmetric
/Hermitian
types, the formerfull
semantics also differed from the norm:which does not necessarily return an
Array
. This pull request effectively changes the behavior toThis change leaves a void where the (potentially useful) former behavior lived. Perhaps this behavior needs a separate name, and perhaps unification with the conceptually similar
full!
methods forXTriangular
types, all of which return a copy of the underlying data modified such that it matches (in theAbstractArray
comparison sense) theSymmetric
/Hermitian
/XTriangular
-wrapped object. Thoughts?Best!