Fix findnext/findprev for sparse arrays to extend Base, making hash fast

src/SparseArrays.jl

@@ -22,7 +22,7 @@ import LinearAlgebra: mul!, ldiv!, rdiv!, cholesky, adjoint!, diag, eigen, dot,
     matop_dest, copytrito!, nonzeroinds
 
 import Base: adjoint, argmin, argmax, Array, broadcast, circshift!, complex, Complex,
-    conj, conj!, convert, copy, copy!, copyto!, count, diff, findall, findmax, findmin,
+    conj, conj!, convert, copy, copy!, copyto!, count, diff, findall, findmax, findmin, findnext, findprev,
     float, getindex, imag, inv, kron, kron!, length, map, maximum, minimum, permute!, real,
     rot180, rotl90, rotr90, setindex!, show, similar, size, sum, transpose,
     vcat, hcat, hvcat, cat, vec, reverse, reverse!

test/sparsematrix_constructors_indexing.jl

@@ -1290,6 +1290,27 @@ end
         @test findprev(!iszero, z_sp, T(4)) isa keytype(z_sp)
         @test findprev(!iszero, z_sp, T(5)) isa keytype(z_sp)
     end
+
+    # The sparse methods must actually extend `Base.findnext`/`Base.findprev` and skip
+    # implicit zeros for predicates other than `!iszero`, e.g. the `!isequal(elt)` that
+    # `Base.hash` uses to skip runs of equal values.
+    @test SparseArrays.findnext === Base.findnext && SparseArrays.findprev === Base.findprev
+    n = 10^9
+    big = spzeros(n); big[1] = 1; big[n ÷ 2] = -0.0
+    @test findprev(!isequal(0.0), big, n) == n ÷ 2
+    @test findprev(!isequal(-0.0), big, n ÷ 2) == n ÷ 2 - 1   # implicit 0.0 is not isequal(-0.0)
+    @test findnext(!isequal(0.0), big, 2) == n ÷ 2
+    @test findnext(!isequal(0.0), big, n ÷ 2 + 1) === nothing
+    # the predicate is evaluated once on the implicit zero and then on stored entries only
+    calls = Ref(0)
+    counted = x -> (calls[] += 1; !isequal(x, 0.0))
+    @test findprev(counted, big, n) == n ÷ 2 && calls[] <= nnz(big) + 1
+    calls[] = 0
+    @test findnext(counted, big, 2) == n ÷ 2 && calls[] <= nnz(big) + 1
+    for i in keys(y), f in (!isequal(0.0), !isequal(-0.0), !isequal(7.0), !isequal(NaN))
+        @test findnext(f, y, i) == findnext(f, y_sp, i)
+        @test findprev(f, y, i) == findprev(f, y_sp, i)
+    end
 end
 
 _length_or_count_or_five(::Colon) = 5

test/sparsematrix_ops.jl

@@ -75,6 +75,20 @@ end
     end
 end
 
+@testset "hash matches dense" begin
+    # The stored-entries-only complexity guarantee is checked with an operation-counting
+    # eltype below ("hash walks stored entries only").
+    n = 1000
+    A = spzeros(n, n); A[1, 1] = 1
+    B = copy(A); B[2, 2] = 0.0   # explicitly stored zero must not change the hash
+    @test hash(B) == hash(A) && isequal(B, A)
+    for m in (2, 10, 200), X in (sprand(m, m, 0.1), sprandn(m, m, 0.3), spzeros(m, m))
+        k = min(3, nnz(X)); nonzeros(X)[1:k] .= [NaN, -0.0, 0.0][1:k]
+        @test hash(X) == hash(Matrix(X))
+        @test hash(X, UInt(7)) == hash(Matrix(X), UInt(7))
+    end
+end
+
 @testset "isequal for adjoint/transpose of sparse matrices" begin
     n = 100
     A = spzeros(n, n); A[1, 1] = 1
@@ -663,6 +677,23 @@ Base.isequal(x::Counting, y::Counting) = (stepcounter(); isequal(x.elt, y.elt))
     end
 end
 
+# `Base.hash` on a large array skips runs of equal values with `findprev(!isequal(elt), A, i)`.
+# Each such call on a sparse array costs at most nnz(A) + 1 element comparisons and `hash`
+# makes only a handful of them, whereas the generic `findprev` performs up to length(A).
+@testset "hash walks stored entries only (issue #570)" begin
+    n = 10^5
+    v = sparsevec([1, n ÷ 2], Counting.([1.0, 2.0]), n)
+    w = sparsevec([1, n ÷ 2, n], Counting.([1.0, 0.0, 3.0]), n)
+    A = sparse([1, n ÷ 2], [1, n], Counting.([1.0, 2.0]), n, n)
+    B = sparse([1, n ÷ 2, 7], [1, n, 7], Counting.([1.0, 2.0, 0.0]), n, n)
+    for x in (v, w, A, B)
+        resetcounter()
+        hash(x)
+        @test getcounter() <= 8 * (nnz(x) + 1)
+    end
+    @test hash(v) == hash(Vector(v)) && hash(w) == hash(Vector(w))
+end
+
 @testset "Comparisons to adjoints are efficient" for
     A in Any[sparse(1*I(10000)), sprandn(10000, 10000, 0.00001), sprandn(ComplexF64, 100, 100, 0.9)],
     B in Any[sparse(1*I(10000)), sprandn(10000, 10000, 0.00001), sprandn(ComplexF64, 100, 100, 0.9)]

test/sparsevector.jl

@@ -383,6 +383,20 @@ end
     @test !isequal(spzeros(3), spzeros(4))
 end
 
+@testset "hash matches dense" begin
+    # The stored-entries-only complexity guarantee is checked with an operation-counting
+    # eltype in sparsematrix_ops.jl ("hash walks stored entries only").
+    n = 10^5
+    v = spzeros(n); v[1] = 1
+    w = copy(v); w[2] = 0.0   # explicitly stored zero must not change the hash
+    @test hash(w) == hash(v) && isequal(w, v)
+    for len in (5, 100, 40000), x in (sprand(len, 0.1), sprandn(len, 0.3), spzeros(len))
+        k = min(3, nnz(x)); nonzeros(x)[1:k] .= [NaN, -0.0, 0.0][1:k]
+        @test hash(x) == hash(Vector(x))
+        @test hash(x, UInt(7)) == hash(Vector(x), UInt(7))
+    end
+end
+
 @testset "findall and findnz" begin
     @test findall(!iszero, spv_x1) == findall(!iszero, x1_full)
     @test findall(spv_x1 .> 1) == findall(x1_full .> 1)