move SparseArrays to stdlib #25249

fredrikekre · 2017-12-22T14:33:19Z

Work in progress, but at least I got this locally:

Test Summary:           |     Pass  Fail  Error  Broken     Total
  Overall               | 26574757     2      2  328326  26903087

so its not too bad!

TODOs:

Chase down the 2 last test failures
Activate deprecated sparse methods that are moved from base/deprecated.jl to stdlib/SparseArrays/src/deprecated.jl
Add deprecations for all the moved methods
Make doc building work.

Sacha0 · 2018-01-06T23:37:20Z

Resurrect once #25364 clears, and then on to LinAlg? :)

fredrikekre · 2018-01-08T07:16:25Z

Down to one:

Test Summary:           |     Pass  Error  Broken     Total
  Overall               | 26597574      1  328323  26925898

fredrikekre · 2018-01-09T13:05:52Z

Test Summary: |     Pass  Broken     Total
  Overall     | 26598707  328325  26927032
    SUCCESS

fredrikekre · 2018-01-09T13:15:17Z

base/asyncmap.jl

@@ -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...)


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.

fredrikekre · 2018-01-09T13:24:42Z

base/deprecated.jl

+@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


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.

fredrikekre · 2018-01-09T13:25:50Z

base/deprecated.jl

@@ -505,6 +503,7 @@ function OverflowError()
 end

 # PR #22703
+import .LinAlg: Bidiagonal


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?

Curious :).

fredrikekre · 2018-01-09T13:26:51Z

base/linalg/lapack.jl

@@ -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 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.

Perhaps !iszero? :)

fredrikekre · 2018-01-09T13:31:45Z

stdlib/SparseArrays/src/sparsevector.jl

@@ -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}


I assume this is meant to extend Base.reinterpret? The import Base: reinterpret was removed in #23750. Or should this method have been removed as part of the last commit in that PR @Keno ?

I think this can be deleted.

fredrikekre · 2018-01-09T13:33:42Z

stdlib/SparseArrays/test/sparse.jl

@@ -1048,110 +1048,112 @@ end
    @test iA === iS === nothing
 end

-# findmin/findmax/minumum/maximum
+@testset "findmin/findmax/minumum/maximum" begin


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

Good catch!

fredrikekre · 2018-01-09T13:34:31Z

test/ambiguous.jl

-        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},)))


I don't know what this test does, but it passes with those two lines removed :)

fredrikekre · 2018-01-09T13:35:39Z

This is no longer WIP, all tests, and doc building, passed for me locally, so feel free to review!

fredrikekre · 2018-01-09T14:10:12Z

base/deprecated.jl

-                    "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)


SparseMatrixCSC{T}(I, m, n) -> SparseMatrixCSC{T}(I, size(S)...)

Good catch!

KristofferC · 2018-01-10T13:01:41Z

This looks good to me. Great work.

Sacha0 · 2018-01-11T22:29:46Z

NEWS.md

@@ -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]).


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"?

Sacha0 · 2018-01-11T22:38:27Z

base/deprecated.jl

+## 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


Tangentially, adding the oc to blockdiag might be nice...

Sacha0 · 2018-01-11T22:39:18Z

base/deprecated.jl

+@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


"that were exported"? :)

Sacha0

Absolutely superlative! Thanks so much for your persistence and patience in seeing this work through! :)

Sacha0 · 2018-01-12T02:55:42Z

(The AV i686 failure seems unrelated. FreeBSD timed out without apparent issue prior.)

Sacha0 · 2018-01-12T02:57:51Z

Assuming CI continues to approve and absent objections or requests for time, I plan to merge these changes tomorrow midday PT or later. Best!

Sacha0 · 2018-01-12T20:26:29Z

ararslan · 2018-01-12T21:33:43Z

Thanks so much for taking this on @fredrikekre! You're a true hero!

rfourquet · 2018-01-14T10:17:50Z

When running julia stdlib/SparseArrays/test/runtests.jl, I get a bunch of errors, with show methods (show removes the SparseArrays. module prefix). Is this issue tracked somewhere ?

KristofferC · 2018-01-14T10:32:00Z

What happens when you run make test-SparseArrays?

fredrikekre · 2018-01-14T10:33:19Z

show removes the prefix if the module is Main or something, I tried to modify those tests to incorporate that, but didn't bother in the end. It is rather annoying that the sparse show testset does not pass if copy-pasted into the REPL though.

rfourquet · 2018-01-14T10:50:49Z

What happens when you run make test-SparseArrays?

This works! But indeed, running the runtest.jl file directly or copy-pasting a @testset into the REPL would be nice. I guess this problem will show-up also with other stdlib packages.

KristofferC · 2018-01-14T10:55:13Z

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.

Sacha0 · 2018-01-14T16:48:55Z

Perhaps opening a dedicated issue would be worthwhile? Being able to copy-paste and/or include test files in REPL sessions is useful. Best! :)

rfourquet · 2018-01-15T11:05:02Z

In the "test/offsetarray.jl", there is const OAs_name = join(fullname(OAs), ".") near to the top of the file, which seems to address the same problem.

fredrikekre added excision Removal of code from Base or the repository sparse Sparse arrays stdlib Julia's standard library labels Dec 22, 2017

fredrikekre requested review from andreasnoack and Sacha0 December 22, 2017 14:33

fredrikekre force-pushed the fe/stdlib-sparsearrays branch 2 times, most recently from 67dc16a to 17f8a9a Compare December 29, 2017 02:00

fredrikekre force-pushed the fe/stdlib-sparsearrays branch 2 times, most recently from 6c63ce9 to d4fa4c5 Compare January 5, 2018 22:09

fredrikekre force-pushed the fe/stdlib-sparsearrays branch from d4fa4c5 to b32e398 Compare January 8, 2018 00:16

fredrikekre force-pushed the fe/stdlib-sparsearrays branch from b32e398 to 77edba7 Compare January 8, 2018 07:32

fredrikekre mentioned this pull request Jan 8, 2018

move Random to stdlib #24874

Merged

5 tasks

fredrikekre force-pushed the fe/stdlib-sparsearrays branch 2 times, most recently from 10763d5 to 6acf733 Compare January 9, 2018 09:36

fredrikekre force-pushed the fe/stdlib-sparsearrays branch 2 times, most recently from 11e778b to 7d3e87c Compare January 9, 2018 13:06

fredrikekre changed the title ~~[WIP] SparseArrays to stdlib~~ move SparseArrays to stdlib Jan 9, 2018

fredrikekre commented Jan 9, 2018

View reviewed changes

fredrikekre force-pushed the fe/stdlib-sparsearrays branch from 7d3e87c to b4ff1bf Compare January 9, 2018 14:33

fredrikekre force-pushed the fe/stdlib-sparsearrays branch 3 times, most recently from 492514d to 9c218a1 Compare January 10, 2018 23:36

Sacha0 reviewed Jan 11, 2018

View reviewed changes

Sacha0 approved these changes Jan 11, 2018

View reviewed changes

move SparseArrays to stdlib

04b3569

fredrikekre force-pushed the fe/stdlib-sparsearrays branch from 9c218a1 to 04b3569 Compare January 11, 2018 23:33

Sacha0 merged commit f8bc37b into JuliaLang:master Jan 12, 2018

fredrikekre deleted the fe/stdlib-sparsearrays branch January 12, 2018 21:16

blegat mentioned this pull request Jan 21, 2018

Clean up sdp2 JuliaOpt/MathOptInterfaceTests.jl#46

Merged

cormullion mentioned this pull request Mar 18, 2018

NEWS.md is getting a bit untidy #26508

Closed

mcognetta mentioned this pull request Jul 29, 2018

bi/tri/symdiagonal matrix multiplication should return sparse matrices #28343

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move SparseArrays to stdlib #25249

move SparseArrays to stdlib #25249

fredrikekre commented Dec 22, 2017 •

edited

Loading

Sacha0 commented Jan 6, 2018

fredrikekre commented Jan 8, 2018

fredrikekre commented Jan 9, 2018

fredrikekre Jan 9, 2018

fredrikekre Jan 9, 2018

fredrikekre Jan 9, 2018

Sacha0 Jan 11, 2018

fredrikekre Jan 9, 2018

Sacha0 Jan 11, 2018

fredrikekre Jan 9, 2018 •

edited

Loading

KristofferC Jan 10, 2018

fredrikekre Jan 9, 2018

Sacha0 Jan 11, 2018

fredrikekre Jan 9, 2018

fredrikekre commented Jan 9, 2018

fredrikekre Jan 9, 2018

Sacha0 Jan 11, 2018

KristofferC commented Jan 10, 2018

Sacha0 Jan 11, 2018

Sacha0 Jan 11, 2018

Sacha0 Jan 11, 2018

Sacha0 left a comment

Sacha0 commented Jan 12, 2018

Sacha0 commented Jan 12, 2018

Sacha0 commented Jan 12, 2018

ararslan commented Jan 12, 2018

rfourquet commented Jan 14, 2018

KristofferC commented Jan 14, 2018

fredrikekre commented Jan 14, 2018

rfourquet commented Jan 14, 2018

KristofferC commented Jan 14, 2018

Sacha0 commented Jan 14, 2018

rfourquet commented Jan 15, 2018

move SparseArrays to stdlib #25249

move SparseArrays to stdlib #25249

Conversation

fredrikekre commented Dec 22, 2017 • edited Loading

Sacha0 commented Jan 6, 2018

fredrikekre commented Jan 8, 2018

fredrikekre commented Jan 9, 2018

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

fredrikekre Jan 9, 2018 • 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

fredrikekre commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KristofferC commented Jan 10, 2018

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 Jan 12, 2018

Sacha0 commented Jan 12, 2018

Sacha0 commented Jan 12, 2018

ararslan commented Jan 12, 2018

rfourquet commented Jan 14, 2018

KristofferC commented Jan 14, 2018

fredrikekre commented Jan 14, 2018

rfourquet commented Jan 14, 2018

KristofferC commented Jan 14, 2018

Sacha0 commented Jan 14, 2018

rfourquet commented Jan 15, 2018

fredrikekre commented Dec 22, 2017 •

edited

Loading

fredrikekre Jan 9, 2018 •

edited

Loading