Skip to content
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.

Sign up for GitHub

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

Jump to bottom

eye speye deprecated methods #24356

Merged
merged 1 commit into from
Nov 19, 2017
Merged

eye speye deprecated methods #24356

merged 1 commit into from
Nov 19, 2017

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Oct 27, 2017 •
edited
Loading

Towards #23156 (comment), this pull request explores deprecating speye. Please see observations from sketching this pull request in #23156 (comment). Best!

@Sacha0 Sacha0 added domain:arrays [a, r, r, a, y, s] domain:linear algebra Linear algebra labels Oct 27, 2017
@Sacha0 Sacha0 mentioned this pull request Oct 27, 2017
Closed
ararslan
ararslan reviewed Oct 27, 2017
View reviewed changes
base/deprecated.jl Outdated
# deprecate speye
@deprecate speye(n::Integer) sparse(1.0I, n)
@deprecate speye(m::Integer, n::Integer) sparse(1.0I, m, n)
@deprecate speye(::Type{T}, n::Integer) where {T} = sparse(UniformScaling{T}(one(T)), n)
Copy link
Member

Choose a reason for hiding this comment

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

Misplaced =?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Fixed. Thanks Alex! :)

@ararslan ararslan added kind:deprecation This change introduces or involves a deprecation domain:arrays:sparse Sparse arrays labels Oct 27, 2017
@Sacha0 Sacha0 added the needs news A NEWS entry is required for this change label Oct 27, 2017
@Sacha0
Copy link
Member Author

Sacha0 commented Oct 27, 2017

As @fredrikekre prompted in #23156 (comment) / #23156 (comment) , we might consider simultaneously deprecating sparse(I, m, n) (as opposed to sparse(I, n)). Best!

@Sacha0 Sacha0 force-pushed the nixspeye branch 2 times, most recently from 2ea21f4 to 5005854 Compare October 27, 2017 21:38
@Sacha0 Sacha0 removed the needs news A NEWS entry is required for this change label Oct 27, 2017
@Sacha0 Sacha0 changed the title [WIP] deprecate speye deprecate speye Oct 27, 2017
@Sacha0
Copy link
Member Author

Sacha0 commented Oct 27, 2017

Fixed a few mistakes, added news, and removed the WIP label. Downstream actions include introducing a SparseMatrixCSC(I, m, n) constructor and potentially deprecating sparse(I, m, n), though perhaps there's no need. Best!

@ViralBShah
Copy link
Member

I think sparse(I, m, n) is useful. I can't recollect my use cases though.

@Sacha0 Sacha0 added the status:triage This should be discussed on a triage call label Oct 28, 2017
@Sacha0 Sacha0 force-pushed the nixspeye branch 2 times, most recently from 70d29c6 to db88a73 Compare October 28, 2017 21:34
@Sacha0 Sacha0 changed the title deprecate speye [WIP] deprecate speye Oct 30, 2017
@Sacha0
Copy link
Member Author

Sacha0 commented Oct 31, 2017

Rebased and revised with #24372 and #24396 in. Pending CI, this pull request should be in shape. Best!

@Sacha0 Sacha0 changed the title [WIP] deprecate speye deprecate speye Oct 31, 2017
fredrikekre
fredrikekre reviewed Oct 31, 2017
View reviewed changes
base/deprecated.jl
@@ -2062,6 +2062,57 @@ end
# deprecate bits to bitstring (#24263, #24281)
@deprecate bits bitstring

# deprecate speye
Copy link
Member

Choose a reason for hiding this comment

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

Should export speye to avoid #24325 again :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! One day I will remember :). Also fixed. Thanks! :)

JeffBezanson
JeffBezanson reviewed Nov 2, 2017
View reviewed changes
base/deprecated.jl Outdated
"`sparse(1.0I, n)`, `SparseMatrixCSC(1.0I, n)`, or `SparseMatrixCSC{Float64}(I, n)`. ",
"If `Float64` element type is not necessary, consider the shorter `sparse(I, n)` ",
"or `SparseMatrixCSC(I, n)` (with default `eltype(I)` of `Bool`)."), :speye)
return speye(1.0I, n)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

sparse(1.0I, n)) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Fixed on push. Thanks!

@Sacha0
Copy link
Member Author

Sacha0 commented Nov 2, 2017

Plan per triage: Remove the single-integer versions of sparse(I, ...) and SparseMatrixCSC(I, ...) and merge. Best!

@Sacha0 Sacha0 added this to the 1.0 milestone Nov 2, 2017
@Sacha0 Sacha0 removed the status:triage This should be discussed on a triage call label Nov 2, 2017
@Sacha0
Copy link
Member Author

Sacha0 commented Nov 4, 2017 •
edited
Loading

Revised in accord with triage, removing the single-integer versions of sparse(I, ...) and SparseMatrixCSC(I, ...). Best!

@Sacha0
Copy link
Member Author

Sacha0 commented Nov 5, 2017

Ref. #24438 (comment). Best!

@Sacha0 Sacha0 changed the title deprecate speye eye speye deprecated methods Nov 6, 2017
@Sacha0 Sacha0 mentioned this pull request Nov 6, 2017
Merged
@Sacha0
Copy link
Member Author

Sacha0 commented Nov 18, 2017

Rebased. Absent objections or requests for time, I plan to merge this pull request tomorrow. Best!

@Sacha0 Sacha0 merged commit a05a249 into JuliaLang:master Nov 19, 2017
@Sacha0 Sacha0 deleted the nixspeye branch November 19, 2017 23:43
@Sacha0
Copy link
Member Author

Sacha0 commented Nov 19, 2017

Thanks all! :)

martinholters
martinholters reviewed Nov 20, 2017
View reviewed changes
base/deprecated.jl
"if `T` is either concrete or abstract. If element type `T` is not necessary, ",
"consider the shorter `sparse(I, n, n)` or `SparseMatrixCSC(I, n, n)` ",
"(with default `eltype(I)` of `Bool`)."), :speye)
return SparseMatrixCSC{T}(I, n)
Copy link
Member

@martinholters martinholters Nov 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

SparseMatrixCSC{T}(I, n, n)

Currently,

julia> speye(Float64, 4)
WARNING: `speye(T, n::Integer)` has been deprecated in favor of `I`, `sparse`, and `SparseMatrixCSC` constructor methods. For a direct replacement, consider `sparse(T(1)I, n, n)` if `T` is concrete or `SparseMatrixCSC{T}(I, n, n)` if `T` is either concrete or abstract. If element type `T` is not necessary, consider the shorter `sparse(I, n, n)` or `SparseMatrixCSC(I, n, n)` (with default `eltype(I)` of `Bool`).
Stacktrace:
 [1] depwarn(::String, ::Symbol) at ./deprecated.jl:68
 [2] speye(::Type{Float64}, ::Int64) at ./deprecated.jl:2145
 [3] top-level scope
 [4] eval(::Module, ::Expr) at ./repl/REPL.jl:3
 [5] eval_user_input(::Any, ::Base.REPL.REPLBackend) at ./repl/REPL.jl:69
 [6] macro expansion at ./repl/REPL.jl:100 [inlined]
 [7] (::getfield(Base.REPL, Symbol("##1#2")){Base.REPL.REPLBackend})() at ./event.jl:95
in expression starting at no file:0
ERROR: MethodError: no method matching SparseMatrixCSC{Float64,Ti} where Ti<:Integer(::UniformScaling{Bool}, ::Int64)
Stacktrace:
 [1] speye(::Type{Float64}, ::Int64) at ./deprecated.jl:2151
 [2] top-level scope

EDIT: Fix at #24663.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @martinholters! :)

@cormullion cormullion mentioned this pull request Mar 18, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JeffBezanson JeffBezanson JeffBezanson left review comments

@ararslan ararslan ararslan left review comments

@fredrikekre fredrikekre fredrikekre left review comments

@martinholters martinholters martinholters left review comments

Assignees
No one assigned
Labels
domain:arrays:sparse Sparse arrays domain:arrays [a, r, r, a, y, s] domain:linear algebra Linear algebra kind:deprecation This change introduces or involves a deprecation
Projects
None yet
Milestone
0.7
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Sacha0 @ViralBShah @JeffBezanson @ararslan @fredrikekre @martinholters