-
-
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
move SparseArrays to stdlib #25249
move SparseArrays to stdlib #25249
Conversation
67dc16a
to
17f8a9a
Compare
6c63ce9
to
d4fa4c5
Compare
Resurrect once #25364 clears, and then on to |
d4fa4c5
to
b32e398
Compare
Down to one:
|
b32e398
to
77edba7
Compare
10763d5
to
6acf733
Compare
|
11e778b
to
7d3e87c
Compare
@@ -260,13 +260,6 @@ function asyncmap(f, b::BitArray; kwargs...) | |||
return b2 | |||
end | |||
|
|||
# TODO: Optimize for sparse arrays | |||
# For now process as regular arrays and convert back | |||
function asyncmap(f, s::AbstractSparseArray...; kwargs...) |
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 deprecated this in stdlib/SparseArrays/src/deprecated.jl
since it is better to be transparent to the user that we don't have an optimized method for this anyway.
base/deprecated.jl
Outdated
@deprecate_moved droptol! "SparseArrays" false true | ||
## deprecated functions that are moved to stdlib/SparseArrays/src/deprecated.jl | ||
@deprecate_moved spones "SparseArrays" true true | ||
@deprecate_moved speye "SparseArrays" true true |
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.
Calling spones
and speye
now will result in users first getting redirected to SparseArrays
, and then get a message that those methods are deprecated. But perhaps no big deal.
@@ -505,6 +503,7 @@ function OverflowError() | |||
end | |||
|
|||
# PR #22703 | |||
import .LinAlg: Bidiagonal |
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.
Had to add this (and similarly below) to be able to build the sysimg, otherwise I get
LoadError("sysimg.jl", 461, LoadError("deprecated.jl", 507, TypeError(:Union, "parameter", Type, Bidiagonal)))
I wonder why those are not needed to compile master?
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.
Curious :).
base/linalg/lapack.jl
Outdated
@@ -3759,7 +3759,7 @@ for (stev, stebz, stegr, stein, elty) in | |||
chklapackerror(info[]) | |||
if any(ifail .!= 0) | |||
# TODO: better error message / type | |||
error("failed to converge eigenvectors:\n$(nonzeros(ifail))") | |||
error("failed to converge eigenvectors:\n$(find(x->x != 0, ifail))") |
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.
There is no nonzeros
method for Vector
on master, but with this excision we get
WARNING: importing deprecated binding Base.nonzeros into LAPACK.
here, so updated.
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.
Perhaps !iszero
? :)
@@ -896,7 +896,7 @@ vec(x::AbstractSparseVector) = x | |||
copy(x::AbstractSparseVector) = | |||
SparseVector(length(x), copy(nonzeroinds(x)), copy(nonzeros(x))) | |||
|
|||
function reinterpret(::Type{T}, x::AbstractSparseVector{Tv}) where {T,Tv} | |||
function Base.reinterpret(::Type{T}, x::AbstractSparseVector{Tv}) where {T,Tv} |
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.
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 this can be deleted.
@@ -1048,110 +1048,112 @@ end | |||
@test iA === iS === nothing | |||
end | |||
|
|||
# findmin/findmax/minumum/maximum | |||
@testset "findmin/findmax/minumum/maximum" begin |
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 just adds a testset to fix:
Deprecated syntax `implicit assignment to global variable `A``
see https://github.com/JuliaLang/julia/pull/25249/files?w=1#diff-6b5c2f417a1aa03f6bfdfb283f4bd01c
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!
test/ambiguous.jl
Outdated
pop!(need_to_handle_undef_sparam, which(Base.SparseArrays._absspvec_vcat, (AbstractSparseArray{Tv, Ti, 1} where {Tv, Ti},))) | ||
pop!(need_to_handle_undef_sparam, which(Base.SparseArrays._absspvec_hcat, (AbstractSparseArray{Tv, Ti, 1} where {Tv, Ti},))) | ||
pop!(need_to_handle_undef_sparam, which(Base.cat, (Any, Base.SparseArrays._TypedDenseConcatGroup{T} where T))) | ||
# pop!(need_to_handle_undef_sparam, which(SparseArrays._absspvec_vcat, (AbstractSparseArray{Tv, Ti, 1} where {Tv, Ti},))) |
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 don't know what this test does, but it passes with those two lines removed :)
This is no longer WIP, all tests, and doc building, passed for me locally, so feel free to review! |
"If preserving element type `T` is not necessary, consider the shorter ", | ||
"`sparse(I, size(S)...)` or `SparseMatrixCSC(I, size(S))` (with default ", | ||
"`eltype(I)` of `Bool`)."), :speye) | ||
return SparseMatrixCSC{T}(I, m, n) |
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.
SparseMatrixCSC{T}(I, m, n)
-> SparseMatrixCSC{T}(I, size(S)...)
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!
7d3e87c
to
b4ff1bf
Compare
This looks good to me. Great work. |
492514d
to
9c218a1
Compare
NEWS.md
Outdated
@@ -881,6 +881,8 @@ Deprecated or removed | |||
* The functions `eigs` and `svds` have been moved to the `IterativeEigensolvers` standard | |||
library module ([#24714]). | |||
|
|||
* Sparse array functionality has moved to the `SparseArrays` standard library ([#25249]). |
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.
Perhaps "has moved to the SparseArrays
(package|module) in the standard library", "has moved to the SparseArrays
standard library (package|module)", or "has moved to SparseArrays
in the standard library"?
## functions that were re-exported from Base | ||
@deprecate_moved nonzeros "SparseArrays" true true | ||
@deprecate_moved permute "SparseArrays" true true | ||
@deprecate_moved blkdiag "SparseArrays" true true |
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.
Tangentially, adding the oc
to blockdiag
might be nice...
base/deprecated.jl
Outdated
@deprecate_moved rowvals "SparseArrays" true true | ||
@deprecate_moved nzrange "SparseArrays" true true | ||
@deprecate_moved nnz "SparseArrays" true true | ||
## functions that where exported from Base.SparseArrays but not from Base |
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 were exported"? :)
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.
Absolutely superlative! Thanks so much for your persistence and patience in seeing this work through! :)
9c218a1
to
04b3569
Compare
(The AV i686 failure seems unrelated. FreeBSD timed out without apparent issue prior.) |
Assuming CI continues to approve and absent objections or requests for time, I plan to merge these changes tomorrow midday PT or later. Best! |
Thanks so much for taking this on @fredrikekre! You're a true hero! |
When running |
What happens when you run |
|
This works! But indeed, running the |
Yeah, I think @fredrikekre identified the issue then. Not sure what the best way to solve it is. Probably change the show tests to look if the module is loaded in Main. |
Perhaps opening a dedicated issue would be worthwhile? Being able to copy-paste and/or include test files in REPL sessions is useful. Best! :) |
In the "test/offsetarray.jl", there is |
Work in progress, but at least I got this locally:
so its not too bad!
TODOs:
base/deprecated.jl
tostdlib/SparseArrays/src/deprecated.jl