From 72415540d1e929a69b445c1f0279729468809bd6 Mon Sep 17 00:00:00 2001 From: Sacha Verweij Date: Fri, 8 Jul 2016 19:09:45 -0700 Subject: [PATCH 01/10] Rewrite setindex!(A::SparseMatrixCSC, v, i, j) such that it no longer expunges stored entries on zero assignment, throws a more explicit BoundsError when needed, and also for simplicity and clarity. Also add tests. --- base/sparse/sparsematrix.jl | 41 +++++++++++++++---------------------- test/sparsedir/sparse.jl | 9 ++++++++ 2 files changed, 26 insertions(+), 24 deletions(-) diff --git a/base/sparse/sparsematrix.jl b/base/sparse/sparsematrix.jl index 62d50096a880e..900388f2e45dc 100644 --- a/base/sparse/sparsematrix.jl +++ b/base/sparse/sparsematrix.jl @@ -2489,32 +2489,25 @@ getindex{T<:Integer}(A::SparseMatrixCSC, I::AbstractVector{Bool}, J::AbstractVec function setindex!{Tv,Ti}(A::SparseMatrixCSC{Tv,Ti}, v, i::Integer, j::Integer) setindex!(A, convert(Tv, v), convert(Ti, i), convert(Ti, j)) end -function setindex!{Tv,Ti<:Integer}(A::SparseMatrixCSC{Tv,Ti}, v::Tv, i0::Ti, i1::Ti) - if !(1 <= i0 <= A.m && 1 <= i1 <= A.n); throw(BoundsError()); end - r1 = Int(A.colptr[i1]) - r2 = Int(A.colptr[i1+1]-1) - if v == 0 #either do nothing or delete entry if it exists - if r1 <= r2 - r1 = searchsortedfirst(A.rowval, i0, r1, r2, Forward) - if (r1 <= r2) && (A.rowval[r1] == i0) - deleteat!(A.rowval, r1) - deleteat!(A.nzval, r1) - @simd for j = (i1+1):(A.n+1) - @inbounds A.colptr[j] -= 1 - end - end - end +function setindex!{Tv,Ti<:Integer}(A::SparseMatrixCSC{Tv,Ti}, v::Tv, i::Ti, j::Ti) + if !((1 <= i <= A.m) & (1 <= j <= A.n)) + throw(BoundsError(A, (i,j))) + end + coljfirstk = Int(A.colptr[j]) + coljlastk = Int(A.colptr[j+1] - 1) + searchk = searchsortedfirst(A.rowval, i, coljfirstk, coljlastk, Base.Order.Forward) + if searchk <= coljlastk && A.rowval[searchk] == i + # Column j contains entry A[i,j]. Update and return + A.nzval[searchk] = v return A end - i = (r1 > r2) ? r1 : searchsortedfirst(A.rowval, i0, r1, r2, Forward) - - if (i <= r2) && (A.rowval[i] == i0) - A.nzval[i] = v - else - insert!(A.rowval, i, i0) - insert!(A.nzval, i, v) - @simd for j = (i1+1):(A.n+1) - @inbounds A.colptr[j] += 1 + # Column j does not contain entry A[i,j]. If v is nonzero, insert entry A[i,j] = v + # and return. If to the contrary v is zero, then simply return. + if v != 0 + insert!(A.rowval, searchk, i) + insert!(A.nzval, searchk, v) + @simd for m in (j + 1):(A.n + 1) + @inbounds A.colptr[m] += 1 end end return A diff --git a/test/sparsedir/sparse.jl b/test/sparsedir/sparse.jl index 88835d4f274fa..47cccac054ccd 100644 --- a/test/sparsedir/sparse.jl +++ b/test/sparsedir/sparse.jl @@ -578,6 +578,14 @@ let a = spzeros(Int, 10, 10) @test countnz(a) == 19 @test a[:,2] == 2*sparse(ones(Int,10)) + # Zero-assignment behavior of setindex!(A, v, i, j) + a[1,3] = 0 + @test nnz(a) == 19 + @test countnz(a) == 18 + a[2,1] = 0 + @test nnz(a) == 19 + @test countnz(a) == 18 + a[1,:] = 1:10 @test a[1,:] == sparse([1:10;]) a[:,2] = 1:10 @@ -1397,6 +1405,7 @@ let x = UpperTriangular(A)*ones(n) @test UpperTriangular(A)\x ≈ ones(n) A[2,2] = 0 + dropzeros!(A) @test_throws LinAlg.SingularException LowerTriangular(A)\ones(n) @test_throws LinAlg.SingularException UpperTriangular(A)\ones(n) end From 49712aae41f2f67e80aef7621a0cf2bba08ef11e Mon Sep 17 00:00:00 2001 From: Sacha Verweij Date: Tue, 12 Jul 2016 18:42:15 -0700 Subject: [PATCH 02/10] Rework setindex!(A::SparseMatrixCSC, v, I, J) such that it no longer expunges stored entries on zero assignment. Also add tests. --- base/sparse/sparsematrix.jl | 129 +++++++++++++++--------------------- test/sparsedir/sparse.jl | 19 ++++++ 2 files changed, 74 insertions(+), 74 deletions(-) diff --git a/base/sparse/sparsematrix.jl b/base/sparse/sparsematrix.jl index 900388f2e45dc..f3b0b3a84ff45 100644 --- a/base/sparse/sparsematrix.jl +++ b/base/sparse/sparsematrix.jl @@ -2525,23 +2525,64 @@ setindex!(A::SparseMatrixCSC, x, ::Colon, ::Colon) = setindex!(A, x, 1:size(A, 1 setindex!(A::SparseMatrixCSC, x, ::Colon, j::Union{Integer, AbstractVector}) = setindex!(A, x, 1:size(A, 1), j) setindex!(A::SparseMatrixCSC, x, i::Union{Integer, AbstractVector}, ::Colon) = setindex!(A, x, i, 1:size(A, 2)) -setindex!{Tv,T<:Integer}(A::SparseMatrixCSC{Tv}, x::Number, I::AbstractVector{T}, J::AbstractVector{T}) = - (0 == x) ? spdelete!(A, I, J) : spset!(A, convert(Tv,x), I, J) - -function spset!{Tv,Ti<:Integer}(A::SparseMatrixCSC{Tv}, x::Tv, I::AbstractVector{Ti}, J::AbstractVector{Ti}) - !issorted(I) && (I = sort(I)) - !issorted(J) && (J = sort(J)) - - m, n = size(A) - lenI = length(I) - - if (!isempty(I) && (I[1] < 1 || I[end] > m)) || (!isempty(J) && (J[1] < 1 || J[end] > n)) +function setindex!{TvA,TiI<:Integer,TiJ<:Integer}(A::SparseMatrixCSC{TvA}, x::Number, + I::AbstractVector{TiI}, J::AbstractVector{TiJ}) + if isempty(I) || isempty(J); return A; end + if !issorted(I); I = sort(I); end + if !issorted(J); J = sort(J); end + if (I[1] < 1 || I[end] > A.m) || (J[1] < 1 || J[end] > A.n) throw(BoundsError(A, (I, J))) end - - if isempty(I) || isempty(J) - return A + if x == 0 + _spsetz_setindex!(A, I, J) + else + _spsetnz_setindex!(A, convert(TvA, x), I, J) + end +end +""" +Helper method for immediately preceding setindex! method. For all (i,j) such that i in I and +j in J, assigns zero to A[i,j] if A[i,j] is a presently-stored entry, and otherwise does nothing. +""" +function _spsetz_setindex!{TvA,TiI<:Integer,TiJ<:Integer}(A::SparseMatrixCSC{TvA}, + I::AbstractVector{TiI}, J::AbstractVector{TiJ}) + lengthI = length(I) + for j in J + coljAfirstk = A.colptr[j] + coljAlastk = A.colptr[j+1] - 1 + coljAfirstk > coljAlastk && continue + kA = coljAfirstk + kI = 1 + entrykArow = A.rowval[kA] + entrykIrow = I[kI] + while true + if entrykArow < entrykIrow + kA += 1 + kA > coljAlastk && break + entrykArow = A.rowval[kA] + elseif entrykArow > entrykIrow + kI += 1 + kI > lengthI && break + entrykIrow = I[kI] + else # entrykArow == entrykIrow + A.nzval[kA] = 0 + kA += 1 + kI += 1 + (kA > coljAlastk || kI > lengthI) && break + entrykArow = A.rowval[kA] + entrykIrow = I[kI] + end + end end +end +""" +Helper method for immediately preceding setindex! method. For all (i,j) such that i in I +and j in J, assigns x to A[i,j] if A[i,j] is a presently-stored entry, and allocates and +assigns x to A[i,j] if A[i,j] is not presently stored. +""" +function _spsetnz_setindex!{Tv,TiI<:Integer,TiJ<:Integer}(A::SparseMatrixCSC{Tv}, x::Tv, + I::AbstractVector{TiI}, J::AbstractVector{TiJ}) + m, n = size(A) + lenI = length(I) nnzA = nnz(A) + lenI * length(J) @@ -2644,66 +2685,6 @@ function spset!{Tv,Ti<:Integer}(A::SparseMatrixCSC{Tv}, x::Tv, I::AbstractVector return A end -function spdelete!{Tv,Ti<:Integer}(A::SparseMatrixCSC{Tv}, I::AbstractVector{Ti}, J::AbstractVector{Ti}) - m, n = size(A) - nnzA = nnz(A) - (nnzA == 0) && (return A) - - !issorted(I) && (I = sort(I)) - !issorted(J) && (J = sort(J)) - - if (!isempty(I) && (I[1] < 1 || I[end] > m)) || (!isempty(J) && (J[1] < 1 || J[end] > n)) - throw(BoundsError(A, (I, J))) - end - - if isempty(I) || isempty(J) - return A - end - - rowval = rowvalA = A.rowval - nzval = nzvalA = A.nzval - rowidx = 1 - ndel = 0 - @inbounds for col in 1:n - rrange = nzrange(A, col) - if ndel > 0 - A.colptr[col] = A.colptr[col] - ndel - end - - if isempty(rrange) || !(col in J) - nincl = length(rrange) - if(ndel > 0) && !isempty(rrange) - copy!(rowvalA, rowidx, rowval, rrange[1], nincl) - copy!(nzvalA, rowidx, nzval, rrange[1], nincl) - end - rowidx += nincl - else - for ridx in rrange - if rowval[ridx] in I - if ndel == 0 - rowval = copy(rowvalA) - nzval = copy(nzvalA) - end - ndel += 1 - else - if ndel > 0 - rowvalA[rowidx] = rowval[ridx] - nzvalA[rowidx] = nzval[ridx] - end - rowidx += 1 - end - end - end - end - - if ndel > 0 - A.colptr[n+1] = rowidx - deleteat!(rowvalA, rowidx:nnzA) - deleteat!(nzvalA, rowidx:nnzA) - end - return A -end - setindex!{Tv,Ti,T<:Integer}(A::SparseMatrixCSC{Tv,Ti}, S::Matrix, I::AbstractVector{T}, J::AbstractVector{T}) = setindex!(A, convert(SparseMatrixCSC{Tv,Ti}, S), I, J) diff --git a/test/sparsedir/sparse.jl b/test/sparsedir/sparse.jl index 47cccac054ccd..e186deafa197e 100644 --- a/test/sparsedir/sparse.jl +++ b/test/sparsedir/sparse.jl @@ -577,6 +577,7 @@ let a = spzeros(Int, 10, 10) a[:,2] = 2 @test countnz(a) == 19 @test a[:,2] == 2*sparse(ones(Int,10)) + b = copy(a) # Zero-assignment behavior of setindex!(A, v, i, j) a[1,3] = 0 @@ -586,6 +587,24 @@ let a = spzeros(Int, 10, 10) @test nnz(a) == 19 @test countnz(a) == 18 + # Zero-assignment behavior of setindex!(A, v, I, J) + a[1,:] = 0 + @test nnz(a) == 19 + @test countnz(a) == 9 + a[2,:] = 0 + @test nnz(a) == 19 + @test countnz(a) == 8 + a[:,1] = 0 + @test nnz(a) == 19 + @test countnz(a) == 8 + a[:,2] = 0 + @test nnz(a) == 19 + @test countnz(a) == 0 + a = copy(b) + a[:,:] = 0 + @test nnz(a) == 19 + @test countnz(a) == 0 + a[1,:] = 1:10 @test a[1,:] == sparse([1:10;]) a[:,2] = 1:10 From a4643458c9fc168aae9629fb2a25774f1282f39f Mon Sep 17 00:00:00 2001 From: Sacha Verweij Date: Tue, 12 Jul 2016 21:18:19 -0700 Subject: [PATCH 03/10] Revise setindex!(A::SparseMatrixCSC, B::SparseMatrixCSC, I, J) such that, when an assigning zero to entry A[i,j], if A[i,j] is not presently stored, the method does not introduce storage for A[i,j]. Also add tests. --- base/sparse/sparsematrix.jl | 32 ++++++++++++++++---------------- test/sparsedir/sparse.jl | 16 ++++++++++++++++ 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/base/sparse/sparsematrix.jl b/base/sparse/sparsematrix.jl index f3b0b3a84ff45..efc9a97da13c7 100644 --- a/base/sparse/sparsematrix.jl +++ b/base/sparse/sparsematrix.jl @@ -2772,16 +2772,16 @@ function setindex!{Tv,Ti,T<:Integer}(A::SparseMatrixCSC{Tv,Ti}, B::SparseMatrixC rowA = rowvalS[ptrA] rowB = I[rowvalB[ptrB]] if rowA < rowB - if ~I_asgn[rowA] - rowvalA[ptrS] = rowA - nzvalA[ptrS] = nzvalS[ptrA] - ptrS += 1 - end + rowvalA[ptrS] = rowA + nzvalA[ptrS] = I_asgn[rowA] ? zero(Tv) : nzvalS[ptrA] + ptrS += 1 ptrA += 1 elseif rowB < rowA - rowvalA[ptrS] = rowB - nzvalA[ptrS] = nzvalB[ptrB] - ptrS += 1 + if nzvalB[ptrB] != zero(Tv) + rowvalA[ptrS] = rowB + nzvalA[ptrS] = nzvalB[ptrB] + ptrS += 1 + end ptrB += 1 else rowvalA[ptrS] = rowB @@ -2794,19 +2794,19 @@ function setindex!{Tv,Ti,T<:Integer}(A::SparseMatrixCSC{Tv,Ti}, B::SparseMatrixC while ptrA < stopA rowA = rowvalS[ptrA] - if ~I_asgn[rowA] - rowvalA[ptrS] = rowA - nzvalA[ptrS] = nzvalS[ptrA] - ptrS += 1 - end + rowvalA[ptrS] = rowA + nzvalA[ptrS] = I_asgn[rowA] ? zero(Tv) : nzvalS[ptrA] + ptrS += 1 ptrA += 1 end while ptrB < stopB rowB = I[rowvalB[ptrB]] - rowvalA[ptrS] = rowB - nzvalA[ptrS] = nzvalB[ptrB] - ptrS += 1 + if nzvalB[ptrB] != zero(Tv) + rowvalA[ptrS] = rowB + nzvalA[ptrS] = nzvalB[ptrB] + ptrS += 1 + end ptrB += 1 end diff --git a/test/sparsedir/sparse.jl b/test/sparsedir/sparse.jl index e186deafa197e..cbafc7929379b 100644 --- a/test/sparsedir/sparse.jl +++ b/test/sparsedir/sparse.jl @@ -605,6 +605,22 @@ let a = spzeros(Int, 10, 10) @test nnz(a) == 19 @test countnz(a) == 0 + # Zero-assignment behavior of setindex!(A, B::SparseMatrixCSC, I, J) + a = copy(b) + a[1:2,:] = spzeros(2, 10) + @test nnz(a) == 19 + @test countnz(a) == 8 + a[1:2,1:3] = sparse([1 0 1; 0 0 1]) + @test nnz(a) == 20 + @test countnz(a) == 11 + a = copy(b) + a[1:2,:] = let c = sparse(ones(2,10)); fill!(c.nzval, 0); c; end + @test nnz(a) == 19 + @test countnz(a) == 8 + a[1:2,1:3] = let c = sparse(ones(2,3)); c[1,2] = c[2,1] = c[2,2] = 0; c; end + @test nnz(a) == 20 + @test countnz(a) == 11 + a[1,:] = 1:10 @test a[1,:] == sparse([1:10;]) a[:,2] = 1:10 From 606a8e44206c416b9565fc887190739bef40e8d0 Mon Sep 17 00:00:00 2001 From: Kristoffer Carlsson Date: Wed, 13 Jul 2016 13:23:03 -0700 Subject: [PATCH 04/10] Revise setindex!(A::SparseMatrixCSC, x, I::AbstractMatrix{Bool}) such that it no longer expunges stored entries on zero assignment. Also add tests. --- base/sparse/sparsematrix.jl | 23 ++++++++++++----------- test/sparsedir/sparse.jl | 8 +++++++- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/base/sparse/sparsematrix.jl b/base/sparse/sparsematrix.jl index efc9a97da13c7..5efa4ba386205 100644 --- a/base/sparse/sparsematrix.jl +++ b/base/sparse/sparsematrix.jl @@ -2842,7 +2842,7 @@ function setindex!{Tv,Ti}(A::SparseMatrixCSC{Tv,Ti}, x, I::AbstractMatrix{Bool}) colptrA = A.colptr; rowvalA = A.rowval; nzvalA = A.nzval colptrB = colptrA; rowvalB = rowvalA; nzvalB = nzvalA - nadd = ndel = 0 + nadd = 0 bidx = xidx = 1 r1 = r2 = 0 @@ -2858,7 +2858,7 @@ function setindex!{Tv,Ti}(A::SparseMatrixCSC{Tv,Ti}, x, I::AbstractMatrix{Bool}) if r1 <= r2 copylen = searchsortedfirst(rowvalA, row, r1, r2, Forward) - r1 if (copylen > 0) - if (nadd > 0) || (ndel > 0) + if (nadd > 0) copy!(rowvalB, bidx, rowvalA, r1, copylen) copy!(nzvalB, bidx, nzvalA, r1, copylen) end @@ -2867,13 +2867,17 @@ function setindex!{Tv,Ti}(A::SparseMatrixCSC{Tv,Ti}, x, I::AbstractMatrix{Bool}) end end - # 0: no change, 1: update, 2: delete, 3: add new - mode = ((r1 <= r2) && (rowvalA[r1] == row)) ? ((v == 0) ? 2 : 1) : ((v == 0) ? 0 : 3) + # 0: no change, 1: update, 2: add new + mode = ((r1 <= r2) && (rowvalA[r1] == row)) ? 1 : ((v == 0) ? 0 : 2) - if (mode > 1) && (nadd == 0) && (ndel == 0) + if (mode > 1) && (nadd == 0) # copy storage to take changes colptrA = copy(colptrB) memreq = (x == 0) ? 0 : n + # this x == 0 check and approach doesn't jive with use of v above + # and may not make sense generally, as scalar x == 0 probably + # means this section should never be called. also may not be generic. + # TODO: clean this up, maybe separate scalar and array X cases rowvalA = copy(rowvalB) nzvalA = copy(nzvalB) resize!(rowvalB, length(rowvalA)+memreq) @@ -2885,9 +2889,6 @@ function setindex!{Tv,Ti}(A::SparseMatrixCSC{Tv,Ti}, x, I::AbstractMatrix{Bool}) bidx += 1 r1 += 1 elseif mode == 2 - r1 += 1 - ndel += 1 - elseif mode == 3 rowvalB[bidx] = row nzvalB[bidx] = v bidx += 1 @@ -2897,7 +2898,7 @@ function setindex!{Tv,Ti}(A::SparseMatrixCSC{Tv,Ti}, x, I::AbstractMatrix{Bool}) end # if I[row, col] end # for row in 1:A.m - if ((nadd != 0) || (ndel != 0)) + if (nadd != 0) l = r2-r1+1 if l > 0 copy!(rowvalB, bidx, rowvalA, r1, l) @@ -2907,7 +2908,7 @@ function setindex!{Tv,Ti}(A::SparseMatrixCSC{Tv,Ti}, x, I::AbstractMatrix{Bool}) colptrB[col+1] = bidx if (xidx > n) && (length(colptrB) > (col+1)) - diff = nadd - ndel + diff = nadd colptrB[(col+2):end] = colptrA[(col+2):end] .+ diff r1 = colptrA[col+1] r2 = colptrA[end]-1 @@ -2924,7 +2925,7 @@ function setindex!{Tv,Ti}(A::SparseMatrixCSC{Tv,Ti}, x, I::AbstractMatrix{Bool}) (xidx > n) && break end # for col in 1:A.n - if (nadd != 0) || (ndel != 0) + if (nadd != 0) n = length(nzvalB) if n > (bidx-1) deleteat!(nzvalB, bidx:n) diff --git a/test/sparsedir/sparse.jl b/test/sparsedir/sparse.jl index cbafc7929379b..33a00dbb1c10a 100644 --- a/test/sparsedir/sparse.jl +++ b/test/sparsedir/sparse.jl @@ -709,15 +709,21 @@ let S = sprand(50, 30, 0.5, x->round(Int,rand(x)*100)), I = sprand(Bool, 50, 30, sumS1 = sum(S) sumFI = sum(S[FI]) + nnzS1 = nnz(S) S[FI] = 0 - @test sum(S[FI]) == 0 sumS2 = sum(S) + cnzS2 = countnz(S) + @test sum(S[FI]) == 0 + @test nnz(S) == nnzS1 @test (sum(S) + sumFI) == sumS1 S[FI] = 10 + nnzS3 = nnz(S) @test sum(S) == sumS2 + 10*sum(FI) S[FI] = 0 @test sum(S) == sumS2 + @test nnz(S) == nnzS3 + @test countnz(S) == cnzS2 S[FI] = [1:sum(FI);] @test sum(S) == sumS2 + sum(1:sum(FI)) From 6d3e198c2d63f4d163b8c4ba2c05b5c1de82abc1 Mon Sep 17 00:00:00 2001 From: Kristoffer Carlsson Date: Wed, 13 Jul 2016 13:38:45 -0700 Subject: [PATCH 05/10] Revise setindex!(A::SparseMatrixCSC, x, I::AbstractVector) such that it no longer expunges stored entries on zero assignment. Also add tests. --- base/sparse/sparsematrix.jl | 23 ++++++++++------------- test/sparsedir/sparse.jl | 14 ++++++++++++++ 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/base/sparse/sparsematrix.jl b/base/sparse/sparsematrix.jl index 5efa4ba386205..84b9550119b7e 100644 --- a/base/sparse/sparsematrix.jl +++ b/base/sparse/sparsematrix.jl @@ -2942,7 +2942,7 @@ function setindex!{Tv,Ti,T<:Real}(A::SparseMatrixCSC{Tv,Ti}, x, I::AbstractVecto colptrA = A.colptr; rowvalA = A.rowval; nzvalA = A.nzval; szA = size(A) colptrB = colptrA; rowvalB = rowvalA; nzvalB = nzvalA - nadd = ndel = 0 + nadd = 0 bidx = aidx = 1 S = issorted(I) ? (1:n) : sortperm(I) @@ -2968,8 +2968,8 @@ function setindex!{Tv,Ti,T<:Real}(A::SparseMatrixCSC{Tv,Ti}, x, I::AbstractVecto r2 = Int(colptrA[col+1] - 1) # copy from last position till current column - if (nadd > 0) || (ndel > 0) - colptrB[(lastcol+1):col] = colptrA[(lastcol+1):col] .+ (nadd - ndel) + if (nadd > 0) + colptrB[(lastcol+1):col] = colptrA[(lastcol+1):col] .+ nadd copylen = r1 - aidx if copylen > 0 copy!(rowvalB, bidx, rowvalA, aidx, copylen) @@ -2986,7 +2986,7 @@ function setindex!{Tv,Ti,T<:Real}(A::SparseMatrixCSC{Tv,Ti}, x, I::AbstractVecto if r1 <= r2 copylen = searchsortedfirst(rowvalA, row, r1, r2, Forward) - r1 if (copylen > 0) - if (nadd > 0) || (ndel > 0) + if (nadd > 0) copy!(rowvalB, bidx, rowvalA, r1, copylen) copy!(nzvalB, bidx, nzvalA, r1, copylen) end @@ -2996,13 +2996,14 @@ function setindex!{Tv,Ti,T<:Real}(A::SparseMatrixCSC{Tv,Ti}, x, I::AbstractVecto end end - # 0: no change, 1: update, 2: delete, 3: add new - mode = ((r1 <= r2) && (rowvalA[r1] == row)) ? ((v == 0) ? 2 : 1) : ((v == 0) ? 0 : 3) + # 0: no change, 1: update, 2: add new + mode = ((r1 <= r2) && (rowvalA[r1] == row)) ? 1 : ((v == 0) ? 0 : 2) - if (mode > 1) && (nadd == 0) && (ndel == 0) + if (mode > 1) && (nadd == 0) # copy storage to take changes colptrA = copy(colptrB) memreq = (x == 0) ? 0 : n + # see comment/TODO for same statement in preceding logical setindex! method rowvalA = copy(rowvalB) nzvalA = copy(nzvalB) resize!(rowvalB, length(rowvalA)+memreq) @@ -3015,10 +3016,6 @@ function setindex!{Tv,Ti,T<:Real}(A::SparseMatrixCSC{Tv,Ti}, x, I::AbstractVecto aidx += 1 r1 += 1 elseif mode == 2 - r1 += 1 - aidx += 1 - ndel += 1 - elseif mode == 3 rowvalB[bidx] = row nzvalB[bidx] = v bidx += 1 @@ -3027,8 +3024,8 @@ function setindex!{Tv,Ti,T<:Real}(A::SparseMatrixCSC{Tv,Ti}, x, I::AbstractVecto end # copy the rest - @inbounds if (nadd > 0) || (ndel > 0) - colptrB[(lastcol+1):end] = colptrA[(lastcol+1):end] .+ (nadd - ndel) + @inbounds if (nadd > 0) + colptrB[(lastcol+1):end] = colptrA[(lastcol+1):end] .+ nadd r1 = colptrA[end]-1 copylen = r1 - aidx + 1 if copylen > 0 diff --git a/test/sparsedir/sparse.jl b/test/sparsedir/sparse.jl index 33a00dbb1c10a..47e94324ba2da 100644 --- a/test/sparsedir/sparse.jl +++ b/test/sparsedir/sparse.jl @@ -699,6 +699,20 @@ let A = speye(Int, 5), I=1:10, X=reshape([trues(10); falses(15)],5,5) @test A[I] == A[X] == [1,0,0,0,0,0,1,0,0,0] A[I] = [1:10;] @test A[I] == A[X] == collect(1:10) + A[I] = zeros(Int, 10) + @test nnz(A) == 13 + @test countnz(A) == 3 + @test A[I] == A[X] == zeros(Int, 10) + c = collect(11:20); c[1] = c[3] = 0 + A[I] = c + @test nnz(A) == 13 + @test countnz(A) == 11 + @test A[I] == A[X] == c + A = speye(Int, 5) + A[I] = c + @test nnz(A) == 12 + @test countnz(A) == 11 + @test A[I] == A[X] == c end let S = sprand(50, 30, 0.5, x->round(Int,rand(x)*100)), I = sprand(Bool, 50, 30, 0.2) From de379692587e25558cecbbf9b0fff90524545822 Mon Sep 17 00:00:00 2001 From: Sacha Verweij Date: Fri, 15 Jul 2016 12:32:46 -0700 Subject: [PATCH 06/10] Introduce rudimentary dropstored! methods for dropping stored entries from SparseMatrixCSCs. --- base/sparse/sparsematrix.jl | 103 ++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/base/sparse/sparsematrix.jl b/base/sparse/sparsematrix.jl index 84b9550119b7e..666e3fd0277bb 100644 --- a/base/sparse/sparsematrix.jl +++ b/base/sparse/sparsematrix.jl @@ -3044,6 +3044,109 @@ function setindex!{Tv,Ti,T<:Real}(A::SparseMatrixCSC{Tv,Ti}, x, I::AbstractVecto A end +## dropstored! methods +""" + dropstored!(A::SparseMatrixCSC, i::Integer, j::Integer) + +Drop entry `A[i,j]` from `A` if `A[i,j]` is stored, and otherwise do nothing. +""" +function dropstored!(A::SparseMatrixCSC, i::Integer, j::Integer) + if !((1 <= i <= A.m) & (1 <= j <= A.n)) + throw(BoundsError(A, (i,j))) + end + coljfirstk = Int(A.colptr[j]) + coljlastk = Int(A.colptr[j+1] - 1) + searchk = searchsortedfirst(A.rowval, i, coljfirstk, coljlastk, Base.Order.Forward) + if searchk <= coljlastk && A.rowval[searchk] == i + # Entry A[i,j] is stored. Drop and return. + deleteat!(A.rowval, searchk) + deleteat!(A.nzval, searchk) + @simd for m in j:(A.n + 1) + @inbounds A.colptr[m] -= 1 + end + end + return A +end +""" + dropstored!(A::SparseMatrix, I::AbstractVector{<:Integer}, J::AbstractVector{<:Integer}) + +For each `(i,j)` where `i in I` and `j in J`, drop entry `A[i,j]` from `A` if `A[i,j]` is +stored and otherwise do nothing. Derivative forms: + + dropstored!(A::SparseMatrixCSC, i::Integer, J::AbstractVector{<:Integer}) + dropstored!(A::SparseMatrixCSC, I::AbstractVector{<:Integer}, j::Integer) +""" +function dropstored!{TiI<:Integer,TiJ<:Integer}(A::SparseMatrixCSC, + I::AbstractVector{TiI}, J::AbstractVector{TiJ}) + m, n = size(A) + nnzA = nnz(A) + (nnzA == 0) && (return A) + + !issorted(I) && (I = sort(I)) + !issorted(J) && (J = sort(J)) + + if (!isempty(I) && (I[1] < 1 || I[end] > m)) || (!isempty(J) && (J[1] < 1 || J[end] > n)) + throw(BoundsError(A, (I, J))) + end + + if isempty(I) || isempty(J) + return A + end + + rowval = rowvalA = A.rowval + nzval = nzvalA = A.nzval + rowidx = 1 + ndel = 0 + @inbounds for col in 1:n + rrange = nzrange(A, col) + if ndel > 0 + A.colptr[col] = A.colptr[col] - ndel + end + + if isempty(rrange) || !(col in J) + nincl = length(rrange) + if(ndel > 0) && !isempty(rrange) + copy!(rowvalA, rowidx, rowval, rrange[1], nincl) + copy!(nzvalA, rowidx, nzval, rrange[1], nincl) + end + rowidx += nincl + else + for ridx in rrange + if rowval[ridx] in I + if ndel == 0 + rowval = copy(rowvalA) + nzval = copy(nzvalA) + end + ndel += 1 + else + if ndel > 0 + rowvalA[rowidx] = rowval[ridx] + nzvalA[rowidx] = nzval[ridx] + end + rowidx += 1 + end + end + end + end + + if ndel > 0 + A.colptr[n+1] = rowidx + deleteat!(rowvalA, rowidx:nnzA) + deleteat!(nzvalA, rowidx:nnzA) + end + return A +end +dropstored!{T<:Integer}(A::SparseMatrixCSC, i::Integer, J::AbstractVector{T}) = dropstored!(A, [i], J) +dropstored!{T<:Integer}(A::SparseMatrixCSC, I::AbstractVector{T}, j::Integer) = dropstored!(A, I, [j]) +dropstored!(A::SparseMatrixCSC, ::Colon, j::Union{Integer,AbstractVector}) = dropstored!(A, 1:size(A,1), j) +dropstored!(A::SparseMatrixCSC, i::Union{Integer,AbstractVector}, ::Colon) = dropstored!(A, i, 1:size(A,2)) +dropstored!(A::SparseMatrixCSC, ::Colon, ::Colon) = dropstored!(A, 1:size(A,1), 1:size(A,2)) +dropstored!(A::SparseMatrixCSC, ::Colon) = dropstored!(A, :, :) +# TODO: Several of the preceding methods are optimization candidates. +# TODO: Implement linear indexing methods for dropstored! ? +# TODO: Implement logical indexing methods for dropstored! ? + + # Sparse concatenation function vcat(X::SparseMatrixCSC...) From ffc5be2ec14fb79e7ad74540d145a1755a3a0e27 Mon Sep 17 00:00:00 2001 From: Sacha Verweij Date: Fri, 15 Jul 2016 13:04:29 -0700 Subject: [PATCH 07/10] Introduce tests for dropstored!(A, i, j) and dropstored!(A, I, J). --- test/sparsedir/sparse.jl | 54 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/test/sparsedir/sparse.jl b/test/sparsedir/sparse.jl index 47e94324ba2da..3666e96e9bc89 100644 --- a/test/sparsedir/sparse.jl +++ b/test/sparsedir/sparse.jl @@ -755,6 +755,60 @@ let S = sprand(50, 30, 0.5, x->round(Int,rand(x)*100)) @test sum(S) == (sumS1 - sumS2 + sum(J)) end + +## dropstored! tests +let A = spzeros(Int, 10, 10) + # Introduce nonzeros in row and column two + A[1,:] = 1 + A[:,2] = 2 + @test nnz(A) == 19 + + # Test argument bounds checking for dropstored!(A, i, j) + @test_throws BoundsError Base.SparseArrays.dropstored!(A, 0, 1) + @test_throws BoundsError Base.SparseArrays.dropstored!(A, 1, 0) + @test_throws BoundsError Base.SparseArrays.dropstored!(A, 1, 11) + @test_throws BoundsError Base.SparseArrays.dropstored!(A, 11, 1) + + # Test argument bounds checking for dropstored!(A, I, J) + @test_throws BoundsError Base.SparseArrays.dropstored!(A, 0:1, 1:1) + @test_throws BoundsError Base.SparseArrays.dropstored!(A, 1:1, 0:1) + @test_throws BoundsError Base.SparseArrays.dropstored!(A, 10:11, 1:1) + @test_throws BoundsError Base.SparseArrays.dropstored!(A, 1:1, 10:11) + + # Test behavior of dropstored!(A, i, j) + # --> Test dropping a single stored entry + Base.SparseArrays.dropstored!(A, 1, 2) + @test nnz(A) == 18 + # --> Test dropping a single nonstored entry + Base.SparseArrays.dropstored!(A, 2, 1) + @test nnz(A) == 18 + + # Test behavior of dropstored!(A, I, J) and derivs. + # --> Test dropping a single row including stored and nonstored entries + Base.SparseArrays.dropstored!(A, 1, :) + @test nnz(A) == 9 + # --> Test dropping a single column including stored and nonstored entries + Base.SparseArrays.dropstored!(A, :, 2) + @test nnz(A) == 0 + # --> Introduce nonzeros in rows one and two and columns two and three + A[1:2,:] = 1 + A[:,2:3] = 2 + @test nnz(A) == 36 + # --> Test dropping multiple rows containing stored and nonstored entries + Base.SparseArrays.dropstored!(A, 1:3, :) + @test nnz(A) == 14 + # --> Test dropping multiple columns containing stored and nonstored entries + Base.SparseArrays.dropstored!(A, :, 2:4) + @test nnz(A) == 0 + # --> Introduce nonzeros in every other row + A[1:2:9, :] = 1 + @test nnz(A) == 50 + # --> Test dropping a block of the matrix towards the upper left + Base.SparseArrays.dropstored!(A, 2:5, 2:5) + @test nnz(A) == 42 +end + + #Issue 7507 @test (i7507=sparsevec(Dict{Int64, Float64}(), 10))==spzeros(10) From f98987fce78c593bc4c5a9717586516c32edebf9 Mon Sep 17 00:00:00 2001 From: Sacha Verweij Date: Fri, 15 Jul 2016 13:53:32 -0700 Subject: [PATCH 08/10] Revise setindex!(x::SparseVector, v, i) such that it no longer purges stored entries on zero assignment. Modify tests accordingly. --- base/sparse/sparsevector.jl | 7 +------ test/sparsedir/sparsevector.jl | 9 ++++++--- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/base/sparse/sparsevector.jl b/base/sparse/sparsevector.jl index 4c303ff911cfa..b28c8ab2fe822 100644 --- a/base/sparse/sparsevector.jl +++ b/base/sparse/sparsevector.jl @@ -197,12 +197,7 @@ function setindex!{Tv,Ti<:Integer}(x::SparseVector{Tv,Ti}, v::Tv, i::Ti) m = length(nzind) k = searchsortedfirst(nzind, i) if 1 <= k <= m && nzind[k] == i # i found - if v == 0 - deleteat!(nzind, k) - deleteat!(nzval, k) - else - nzval[k] = v - end + nzval[k] = v else # i not found if v != 0 insert!(nzind, k, i) diff --git a/test/sparsedir/sparsevector.jl b/test/sparsedir/sparsevector.jl index ca94ed7974832..46919c2dd62dd 100644 --- a/test/sparsedir/sparsevector.jl +++ b/test/sparsedir/sparsevector.jl @@ -201,13 +201,16 @@ end let xc = copy(spv_x1) xc[5] = 0.0 - @test exact_equal(xc, SparseVector(8, [2, 6], [1.25, 3.5])) + @test exact_equal(xc, SparseVector(8, [2, 5, 6], [1.25, 0.0, 3.5])) xc[6] = 0.0 - @test exact_equal(xc, SparseVector(8, [2], [1.25])) + @test exact_equal(xc, SparseVector(8, [2, 5, 6], [1.25, 0.0, 0.0])) xc[2] = 0.0 - @test exact_equal(xc, SparseVector(8, Int[], Float64[])) + @test exact_equal(xc, SparseVector(8, [2, 5, 6], [0.0, 0.0, 0.0])) + + xc[1] = 0.0 + @test exact_equal(xc, SparseVector(8, [2, 5, 6], [0.0, 0.0, 0.0])) end From 16971a45736bcae26822dad8448a1a598742d45e Mon Sep 17 00:00:00 2001 From: Sacha Verweij Date: Fri, 15 Jul 2016 14:19:33 -0700 Subject: [PATCH 09/10] Introduce rudimentary dropstored! method for dropping entries from SparseVectors. Also introduce associated tests. --- base/sparse/sparsevector.jl | 22 ++++++++++++++++++++++ test/sparsedir/sparsevector.jl | 12 ++++++++++++ 2 files changed, 34 insertions(+) diff --git a/base/sparse/sparsevector.jl b/base/sparse/sparsevector.jl index b28c8ab2fe822..b07360a707c93 100644 --- a/base/sparse/sparsevector.jl +++ b/base/sparse/sparsevector.jl @@ -211,6 +211,28 @@ setindex!{Tv, Ti<:Integer}(x::SparseVector{Tv,Ti}, v, i::Integer) = setindex!(x, convert(Tv, v), convert(Ti, i)) +### dropstored! +""" + dropstored!(x::SparseVector, i::Integer) + +Drop entry `x[i]` from `x` if `x[i]` is stored and otherwise do nothing. +""" +function dropstored!(x::SparseVector, i::Integer) + if !(1 <= i <= x.n) + throw(BoundsError(x, i)) + end + searchk = searchsortedfirst(x.nzind, i) + if searchk <= length(x.nzind) && x.nzind[searchk] == i + # Entry x[i] is stored. Drop and return. + deleteat!(x.nzind, searchk) + deleteat!(x.nzval, searchk) + end + return x +end +# TODO: Implement linear collection indexing methods for dropstored! ? +# TODO: Implement logical indexing methods for dropstored! ? + + ### Conversion # convert SparseMatrixCSC to SparseVector diff --git a/test/sparsedir/sparsevector.jl b/test/sparsedir/sparsevector.jl index 46919c2dd62dd..62a7649cc1e4d 100644 --- a/test/sparsedir/sparsevector.jl +++ b/test/sparsedir/sparsevector.jl @@ -213,6 +213,18 @@ let xc = copy(spv_x1) @test exact_equal(xc, SparseVector(8, [2, 5, 6], [0.0, 0.0, 0.0])) end +## dropstored! tests +let x = SparseVector(10, [2, 7, 9], [2.0, 7.0, 9.0]) + # Test argument bounds checking for dropstored!(x, i) + @test_throws BoundsError Base.SparseArrays.dropstored!(x, 0) + @test_throws BoundsError Base.SparseArrays.dropstored!(x, 11) + # Test behavior of dropstored!(x, i) + # --> Test dropping a single stored entry + @test Base.SparseArrays.dropstored!(x, 2) == SparseVector(10, [7, 9], [7.0, 9.0]) + # --> Test dropping a single nonstored entry + @test Base.SparseArrays.dropstored!(x, 5) == SparseVector(10, [7, 9], [7.0, 9.0]) +end + ### Array manipulation From 59d3d7cf9617248dfdf9b864cb80a577b64867ad Mon Sep 17 00:00:00 2001 From: Sacha Verweij Date: Thu, 14 Jul 2016 14:24:04 -0700 Subject: [PATCH 10/10] Note in NEWS.md that setindex! for sparse matrices and vectors no longer purges allocated entries on zero assignment and that dropstored! now exists to drop stored entries. --- NEWS.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/NEWS.md b/NEWS.md index b96a6f1f6568d..df92f944d367d 100644 --- a/NEWS.md +++ b/NEWS.md @@ -196,6 +196,10 @@ Library improvements * All `sparse` methods now retain provided numerical zeros as structural nonzeros; to drop numerical zeros, use `dropzeros!` ([#14798], [#15242]). + * `setindex!` methods for sparse matrices and vectors no longer purge allocated entries + on zero assignment. To drop stored entries from sparse matrices and vectors, use + `Base.SparseArrays.dropstored!` ([#17404]). + * New `foreach` function for calling a function on every element of a collection when the results are not needed ([#13774]). @@ -343,3 +347,4 @@ Deprecated or removed [#17323]: https://github.com/JuliaLang/julia/issues/17323 [#17374]: https://github.com/JuliaLang/julia/issues/17374 [#17402]: https://github.com/JuliaLang/julia/issues/17402 +[#17404]: https://github.com/JuliaLang/julia/issues/17404