eye see deprecated methods #24438

Sacha0 · 2017-11-02T01:30:08Z

This pull request deprecates eye in favor of I and Matrix constructors (e.g. ... == I rather than ... == eye(...) and Matrix{T}(I, n) rather than eye(T, n)). Regarding the eye call rewrites in test/, the rewrites are for the most part conservative rather than potentially ideal (e.g. ... == eye(T, 3, 4) -> ... == Matrix(I, 3, 4) rather than ... == I); see JuliaLang/LinearAlgebra.jl#454. Addresses JuliaLang/LinearAlgebra.jl#454. Best!

(I expect a cascade of CI failures and subsequent iteration on this pull request, hence the WIP.)

fredrikekre · 2017-11-02T07:07:09Z

base/deprecated.jl


-@deprecate eye(::Type{Diagonal{T}}, n::Int) where {T} Diagonal{T}(I, n)
+## goodbeye, eye!
+export eye


In theory, I guess we also need

@eval Base.LinAlg import Base.eye @eval Base.SparseArrays import Base.eye

if someone uses LinAlg.eye(...) or SparseArrays.eye() in their codes. I don't think we have done that before, but perhaps we should, since changes like this should not break code :)

fredrikekre · 2017-11-02T07:10:01Z

base/linalg/dense.jl

 # Examples
 ```jldoctest
-julia> A = 2.7182818 * eye(2)
+julia> A = Matrix(2.7182818I, 2)


The I looked like a 1 at first sight and I was confused... Perhaps add an explicit *? Matrix(2.7182818*I, 2)

Revised accordingly on push. Thanks!

fredrikekre · 2017-11-02T07:13:46Z

base/linalg/qr.jl


  - The subdiagonal part contains the reflectors ``v_i`` stored in a packed format where
-    ``v_i`` is the ``i``th column of the matrix `V = eye(m,n) + tril(F.factors,-1)`.
+    ``v_i`` is the ``i``th column of the matrix `V = I + tril(F.factors, -1)`.


Sizes m and n not needed? Same on the changes below.

Omitting the explicit m and n in this documentation seems preferable on this end, particularly if we relax addition with UniformScalings to behave generally as it does with sparse matrices (as discussed on triage).

fredrikekre · 2017-11-02T07:25:30Z

test/sparse/cholmod.jl

    ## Low level interface
    @test CHOLMOD.nnz(A1Sparse) == nnz(A1)
-    @test CHOLMOD.speye(5, 5, elty) == eye(elty, 5, 5)
+    @test CHOLMOD.speye(5, 5, elty) == Matrix(I, 5)


Probably does not matter, but perhaps Matrix{elty}(I, 5)

In ==, the Matrix(I, ...) on the right-hand-side lifts to the eltype of the left-hand-side, making the eltype specification on the right unnecessary :).

fredrikekre · 2017-11-02T07:28:06Z

base/linalg/dense.jl

    ilo, ihi, scale = LAPACK.gebal!('B', A)    # modifies A
    nA   = norm(A, 1)
-    I    = eye(T,n)
+    Inn    = Matrix{T}(I, n)


Great example why exporting single-character names is a bad idea :)

On the other hand, in all recent work related to I I have found only two such collisions IIRC, indicating that in practice this issue is not so great :).

JeffBezanson · 2017-11-02T18:26:43Z

base/deprecated.jl

+        "constructors. For a direct replacement, consider `Matrix(1.0I, m)` or ",
+        "`Matrix{Float64}(I, m)`. If `Float64` element type is not necessary, ",
+        "consider the shorter `Matrix(I, m)` (with default `eltype(I)` `Bool`)."), :eye)
+    return Matrix{Float64}(I, m)


I think this should be Matrix{Float64}(I, m, m).

Sacha0 · 2017-11-02T19:10:47Z

Plan per triage: Remove the single-integer Matrix(I, ...) methods and merge. Best!

Sacha0 · 2017-11-05T19:53:03Z

Rebased and revised per triage, removing all single-integer Matrix constructor calls.

Removing those calls strongly reaffirmed what I conveyed weakly in triage: Constructing a non-square identity matrix is the rare exception; eye(n)/speye(n) is far and away the most common invocation mode. Consequently, having to use the two-integer constructor form feels like a meaningful usability regression, results in noticeably more mistakes, and makes expressions like Matrix(1.0I, size(Q.factors, 1)) much less wieldy (as Matrix(1.0I, size(Q.factors, 1), size(Q.factors, 1)) or (sQf1 = size(Q.factors, 1); Matrix(1.0I, sQf1, sQf1)) where possible). In other words, having only the two-integer constructor form feels in practice like burdensome purism. We should consider again whether that real cost is worth avoiding the perceived risk. Best!

kshyatt · 2017-11-06T22:35:33Z

Why is this not titled "eye speye deprecated methods"?

Anyway looks great but could you rebase?

Sacha0 · 2017-11-06T22:52:35Z

Why is this not titled "eye speye deprecated methods"?

That's brilliant! #24356 now has a vastly superior title.

(Also https://media.giphy.com/media/DvtsYOKrqPZg4/giphy.gif :).)

Sacha0 · 2017-11-18T22:27:21Z

Rebased and (hopefully) fixed the test/sparse/sparse.jl, test/linalg/tridiag.jl, and test/linalg/uniformscaling.jl failures. Assuming CI approves and absent objections or requests for time, I plan to merge this pull request tomorrow. Best!

Sacha0 · 2017-11-20T02:49:44Z

Thanks all! :)

Sacha0 added arrays [a, r, r, a, y, s] deprecation This change introduces or involves a deprecation linear algebra Linear algebra needs news A NEWS entry is required for this change triage This should be discussed on a triage call labels Nov 2, 2017

Sacha0 force-pushed the goodbeye branch from 24e0c94 to 896be52 Compare November 2, 2017 01:32

Sacha0 removed the needs news A NEWS entry is required for this change label Nov 2, 2017

Sacha0 force-pushed the goodbeye branch 2 times, most recently from 8af3e9a to 02d2d38 Compare November 2, 2017 04:31

fredrikekre reviewed Nov 2, 2017

View reviewed changes

JeffBezanson reviewed Nov 2, 2017

View reviewed changes

StefanKarpinski removed the triage This should be discussed on a triage call label Nov 2, 2017

Sacha0 added this to the 1.0 milestone Nov 2, 2017

Sacha0 force-pushed the goodbeye branch 2 times, most recently from c6e46ad to f48bf94 Compare November 5, 2017 19:32

Sacha0 force-pushed the goodbeye branch 2 times, most recently from 6fe1522 to a36c13d Compare November 5, 2017 20:21

Sacha0 force-pushed the goodbeye branch 2 times, most recently from 7d86a5b to 220061d Compare November 18, 2017 22:26

Sacha0 changed the title ~~[WIP] eye see deprecated methods~~ eye see deprecated methods Nov 18, 2017

Sacha0 force-pushed the goodbeye branch from 220061d to 6a763af Compare November 19, 2017 16:56

Sacha0 force-pushed the goodbeye branch from 6a763af to 0fb2be5 Compare November 19, 2017 19:14

Sacha0 added 3 commits November 19, 2017 16:08

Eliminate all eye calls from tests.

39dbf4d

Eliminate all eye calls from base.

6013448

Deprecate eye.

40cd410

Sacha0 force-pushed the goodbeye branch from 0fb2be5 to 40cd410 Compare November 20, 2017 00:11

Sacha0 merged commit 06a6afb into JuliaLang:master Nov 20, 2017

Sacha0 deleted the goodbeye branch November 20, 2017 02:49

Sacha0 mentioned this pull request Nov 20, 2017

make I(n) behave as Diagonal / eye? JuliaLang/LinearAlgebra.jl#454

Closed

cormullion mentioned this pull request Mar 18, 2018

NEWS.md is getting a bit untidy #26508

Closed

dlfivefifty mentioned this pull request Jun 13, 2018

add ones and eyehere JuliaArrays/FillArrays.jl#22

Closed

fredrikekre mentioned this pull request Dec 7, 2018

Overload UniformScaling to make I(n) create a Diagonal matrix. #30298

Merged

Uh oh!

eye see deprecated methods #24438

eye see deprecated methods #24438

Uh oh!

Conversation

Sacha0 commented Nov 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sacha0 commented Nov 2, 2017

Uh oh!

Sacha0 commented Nov 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kshyatt commented Nov 6, 2017

Uh oh!

Sacha0 commented Nov 6, 2017

Uh oh!

Sacha0 commented Nov 18, 2017

Uh oh!

Sacha0 commented Nov 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Sacha0 commented Nov 5, 2017 •

edited

Loading