Deprecate getindex(::Factorization, ::Symbol) in favor of dot overloading #25184

andreasnoack · 2017-12-19T12:40:57Z

Now that we have dot overloading, it is possible to use that instead of the special getindex methods for Factorizations. Furthermore, with a little care in the getproperty methods, it is possible to make the constant propagation work such that qr and lu are now inferred even though they extract the factors from the Factorization.

julia> A = randn(3,3);

julia> F = lufact(A);

julia> F[:L]
3×3 Array{Float64,2}:
  1.0        0.0       0.0
 -0.368965   1.0       0.0
  0.414762  -0.531993  1.0

becomes

julia> F.L
3×3 Array{Float64,2}:
  1.0        0.0       0.0
 -0.368965   1.0       0.0
  0.414762  -0.531993  1.0

Fixes #25159

StefanKarpinski

Gorgeous! Why are all the getproperty methods marked as @inline?

StefanKarpinski · 2017-12-19T15:19:11Z

base/linalg/bunchkaufman.jl

 3×3 Array{Float64,2}:
 0.0 0.0 0.0
 0.0 0.0 0.0
 0.0 0.0 0.0
 ```
 """
-function getindex(B::BunchKaufman{T}, d::Symbol) where {T<:BlasFloat}
+@inline function getproperty(B::BunchKaufman{T}, d::Symbol) where {T<:BlasFloat}


this seems like a lot to inline

I think inlining is necessary to get the constant propagation working sufficiently well to make the factors inferred.

Hm. Maybe that wasn't the issue. It seems to work now. I'll try to remove the annotations and see what happens.

StefanKarpinski · 2017-12-19T15:19:30Z

base/linalg/bunchkaufman.jl

 3×3 Array{Float64,2}:
 0.0 0.0 0.0
 0.0 0.0 0.0
 0.0 0.0 0.0

 julia> F = bkfact(Symmetric(A));

-julia> F[:U]*F[:D]*F[:U]' - F[:P]*A*F[:P]'
+julia> F.U*F.D*F.U' - F.P*A*F.P'


oh boy, that is soooo much prettier

StefanKarpinski · 2017-12-19T15:20:39Z

base/linalg/cholesky.jl

- d == :L && return LowerTriangular(Symbol(C.uplo) == d ? C.factors : C.factors')
- d == :p && return C.piv
- if d == :P
+@inline function getproperty(C::Cholesky, d::Symbol)


again, a lot to inline

Sacha0

Beautiful! :) I will try to work in a proper review this evening.

c42f · 2017-12-19T22:19:36Z

This is really nice. Previously I wrapped SVD for StaticArrays and it wasn't really possible to have performance while sharing the Base interface.

Sacha0 · 2017-12-20T01:37:09Z

base/deprecated.jl

@@ -3423,6 +3423,12 @@ workspace() = error("workspace() is discontinued, check out Revise.jl for an alt
 # PR #25113
 @deprecate_binding CartesianRange CartesianIndices

+# Use getproperty instead of getindex for Factorizations
+function getindex(F::Factorization, s::Symbol)
+ depwarn("F[:$s] is deprecated, use F.$s instead.", :getindex)


Given that this method's F may have any name in the caller's code, F[:...] might be a bit obscure. Perhaps expand this depwarn a bit? A sketch:

Extracting factorization components via getindex(F::Factorization, s::Symbol) methods, usually written F[:s] where F is the <:Factorization and :s identifies the factorization component (for example aQRfact[:Q]), has been deprecated in favor of F.s.

Missed or disagree? :)

Decided to think a little longer and then forgot. I've decided that I prefer the current shorter version. If users later report that it is unclear we can adjust but I wouldn't expect that.

Sacha0 · 2017-12-20T01:39:40Z

base/linalg/bunchkaufman.jl

-size(B::BunchKaufman) = size(B.LD)
-size(B::BunchKaufman, d::Integer) = size(B.LD, d)
+size(B::BunchKaufman) = size(getfield(B, :LD))
+size(B::BunchKaufman, d::Integer) = size(getfield(B, :LD), d)


Why switch to explicitly calling getfield here?

To avoid an infinite cycle between size and getproperty. The reason is that getproperty always calls size.

Cheers, and thanks for the explanation! :)

Sacha0 · 2017-12-20T03:22:07Z

base/linalg/cholesky.jl

+ Cfactors = getfield(C, :factors)
+ Cuplo = getfield(C, :uplo)
+ if d == :U
+ return UpperTriangular(Symbol(Cuplo) == d ? Cfactors : Cfactors')


Assuming you would like an eager adjoint of Cfactors, Cfactors' -> adjoint(Cfactors)?

I didn't consider the possibility. I'm not sure. Should it really be eager when it could be lazy?

If we want this type stable it needs to be eager, right?

True. I thought it would be known at compile time but that is of course not the case for Cuplo. I'll change it.

Sacha0 · 2017-12-20T03:22:28Z

base/linalg/cholesky.jl

+ if d == :U
+ return UpperTriangular(Symbol(Cuplo) == d ? Cfactors : Cfactors')
+ elseif d == :L
+ return LowerTriangular(Symbol(Cuplo) == d ? Cfactors : Cfactors')


Likewise here re. eager versus lazy adjoint :).

Sacha0 · 2017-12-20T03:24:01Z

base/linalg/cholesky.jl

+ Cfactors = getfield(C, :factors)
+ Cuplo = getfield(C, :uplo)
+ if d == :U
+ return UpperTriangular(Symbol(Cuplo) == d ? Cfactors : Cfactors')


And likewise here re. eager versus lazy adjoint :).

Sacha0 · 2017-12-20T03:24:09Z

base/linalg/cholesky.jl

+ if d == :U
+ return UpperTriangular(Symbol(Cuplo) == d ? Cfactors : Cfactors')
+ elseif d == :L
+ return LowerTriangular(Symbol(Cuplo) == d ? Cfactors : Cfactors')


And likewise here re. eager versus lazy adjoint :).

Sacha0 · 2017-12-20T03:27:13Z

base/linalg/eigen.jl

- d == :values && return A.values
- d == :vectors && return A.vectors
- throw(KeyError(d))
-end


Sacha0 · 2017-12-20T03:31:35Z

base/linalg/lq.jl

 end
 end

 getindex(A::LQPackedQ, i::Integer, j::Integer) =
 mul!(A, setindex!(zeros(eltype(A), size(A, 2)), 1, j))[i]

-getq(A::LQ) = LQPackedQ(A.factors, A.τ)


💯! Perhaps getq deserves a deprecation?

Sacha0 · 2017-12-20T03:35:15Z

base/linalg/qr.jl

 sQf1 = size(Q.factors, 1)
 return (!full ? Array(Q) : mul!(Q, Matrix{eltype(Q)}(I, sQf1, sQf1))), R
 end
 function _qr(A::Union{Number, AbstractMatrix}, ::Val{true}; full::Bool = false)
 F = qrfact(A, Val(true))
- Q, R, p = getq(F), F[:R]::Matrix{eltype(F)}, F[:p]::Vector{BlasInt}
+ Q, R, p = F.Q, F.R, F.p


Sacha0 · 2017-12-20T03:42:27Z

base/linalg/svd.jl

- return F.Vt'
+function getproperty(F::SVD, d::Symbol)
+ if d == :V
+ return getfield(F, :Vt)'


adjoint(getfield(F, :Vt)) for eager adjoint?

Sacha0 · 2017-12-20T03:48:54Z

stdlib/SuiteSparse/src/cholmod.jl

 finalizer(free!, A)
 return A
 end
 end
 Sparse(p::Ptr{C_Sparse{Tv}}) where {Tv<:VTypes} = Sparse{Tv}(p)

-Base.unsafe_convert(::Type{Ptr{Tv}}, A::Sparse{Tv}) where {Tv} = A.p
+Base.unsafe_convert(::Type{Ptr{Tv}}, A::Sparse{Tv}) where {Tv} = getfield(A, :ptr)


Switch necessary? (Edit: I'm guessing so given you perform the same rewrite in the unsafe_convert def below as well?)

I had to change the name of the field to ptr because F.p is now syntax for extracting a permutation vector.

Sacha0 · 2017-12-20T04:07:12Z

test/linalg/cholesky.jl

+ @test_throws DimensionMismatch LinAlg.lowrankupdate(F, ones(eltype($v), length($v)+1))
+ @test LinAlg.lowrankdowndate(G, $v).$uplo ≈ F.$uplo
+ @test_throws DimensionMismatch LinAlg.lowrankdowndate(G, ones(eltype($v), length($v)+1))
+ end


Perhaps explicit getproperty calls (if possible?) would be simpler than metafication?

The idea was to exercise the compile time version but that is probably not worth the metafication here.

Sacha0 · 2017-12-20T04:37:57Z

test/linalg/svd.jl

 @test convert(Array, usv) ≈ a
- @test usv[:Vt]' ≈ usv[:V]
- @test_throws KeyError usv[:Z]
+ @test usv.Vt' ≈ usv.V


Eager adjoint? :)

Sacha0

Wonderful. Simply wonderful :).

…ding

Sacha0 · 2017-12-21T22:41:12Z

Rebase and merge? :)

JeffBezanson · 2017-12-22T07:00:50Z

🎉
Amazing. This is one of those things we've been dreaming of for years :)

…ding (#25184)

andreasnoack added the domain:linear algebra Linear algebra label Dec 19, 2017

andreasnoack force-pushed the anj/getprops branch 2 times, most recently from badc334 to 7da8002 Compare December 19, 2017 13:03

StefanKarpinski reviewed Dec 19, 2017

View reviewed changes

andreasnoack mentioned this pull request Dec 19, 2017

WIP: Iterate Factorizations to allow deconstruction #25187

Closed

andreasnoack force-pushed the anj/getprops branch 2 times, most recently from 38d96dd to b474339 Compare December 19, 2017 20:55

Sacha0 reviewed Dec 19, 2017

View reviewed changes

Sacha0 reviewed Dec 20, 2017

View reviewed changes

base/linalg/eigen.jl

d == :values && return A.values

d == :vectors && return A.vectors

throw(KeyError(d))

end

Copy link

Member

Sacha0 Dec 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Sacha0 reviewed Dec 20, 2017

View reviewed changes

Sacha0 approved these changes Dec 20, 2017

View reviewed changes

andreasnoack force-pushed the anj/getprops branch 4 times, most recently from 4da58d2 to 894cb01 Compare December 21, 2017 08:14

andreasnoack added 2 commits December 21, 2017 22:12

Deprecate getindex(::Factorization, ::Symbol) in favor of dot overloa…

edf1f36

…ding

Update NEWS.md

2a84077

Some fixes after rebase

c4e0e30

andreasnoack force-pushed the anj/getprops branch from 894cb01 to c4e0e30 Compare December 21, 2017 22:53

StefanKarpinski merged commit 15c4673 into master Dec 22, 2017

andreasnoack deleted the anj/getprops branch December 22, 2017 07:38

cormullion mentioned this pull request Mar 18, 2018

NEWS.md is getting a bit untidy #26508

Closed

mauro3 mentioned this pull request May 16, 2018

Doc of getproperty polluted by BunchKaufman and QRSparse #27119

Closed

Keno pushed a commit that referenced this pull request Jun 5, 2024

Deprecate getindex(::Factorization, ::Symbol) in favor of dot overloa…

f508386

…ding (#25184)

Deprecate getindex(::Factorization, ::Symbol) in favor of dot overloading #25184

Deprecate getindex(::Factorization, ::Symbol) in favor of dot overloading #25184

Conversation

andreasnoack commented Dec 19, 2017 • edited Loading

StefanKarpinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 left a comment

Choose a reason for hiding this comment

c42f commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 left a comment

Choose a reason for hiding this comment

Sacha0 commented Dec 21, 2017

JeffBezanson commented Dec 22, 2017

andreasnoack commented Dec 19, 2017 •

edited

Loading

Sacha0 Dec 20, 2017 •

edited

Loading

Sacha0 Dec 20, 2017 •

edited

Loading