From 2ae3698e20fdf3f2254f48d37f66a60f1bc98ddd Mon Sep 17 00:00:00 2001 From: "Viral B. Shah" Date: Tue, 8 Sep 2026 05:39:53 -0400 Subject: [PATCH] Fix `findnext`/`findprev` for sparse arrays to actually extend `Base` The sparse `findnext`/`findprev` methods in `abstractsparse.jl` defined a local `SparseArrays.findnext`/`findprev` rather than extending `Base`, ever since the import was dropped in the JuliaLang/julia import cleanup (#42894). Every call therefore hit the generic elementwise `Base` methods, and the existing tests did not notice because they only compared results against dense arrays. Restoring the import makes `Base.hash` on large sparse arrays walk stored entries only: `_hash_fib` skips runs of equal values via `findprev(!isequal(elt), A, i)`, which now jumps over implicit zeros. `hash` of a length-10^9 sparse vector with one stored entry drops from ~1s to microseconds, and a 10^4 x 10^4 sparse matrix from 81ms to 4us, with values unchanged (still identical to hashing the dense array). Add tests that the methods extend `Base`, that `findnext`/`findprev` with `!isequal(x)` predicates match dense for NaN and signed zeros, and that `hash` matches dense (including stored zeros) with a timing guard. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01LgBHUw9Hp7YW5ub29B4R5y --- src/SparseArrays.jl | 2 +- test/sparsematrix_constructors_indexing.jl | 16 ++++++++++++++++ test/sparsematrix_ops.jl | 16 +++++++++++++++- test/sparsevector.jl | 14 ++++++++++++++ 4 files changed, 46 insertions(+), 2 deletions(-) diff --git a/src/SparseArrays.jl b/src/SparseArrays.jl index a3ef1767..54fa1d84 100644 --- a/src/SparseArrays.jl +++ b/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! diff --git a/test/sparsematrix_constructors_indexing.jl b/test/sparsematrix_constructors_indexing.jl index a220478a..9490d0c1 100644 --- a/test/sparsematrix_constructors_indexing.jl +++ b/test/sparsematrix_constructors_indexing.jl @@ -1290,6 +1290,22 @@ 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 # also warms up before timing + @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 + @test @elapsed(findprev(!isequal(0.0), big, n)) < 0.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 diff --git a/test/sparsematrix_ops.jl b/test/sparsematrix_ops.jl index a0d941a3..c521fa1e 100644 --- a/test/sparsematrix_ops.jl +++ b/test/sparsematrix_ops.jl @@ -74,13 +74,27 @@ end end end +@testset "hash walks stored entries only" begin + n = 10^5 + A = spzeros(n, n); A[1, 1] = 1 + hash(A) # warm up + @test @elapsed(hash(A)) < 0.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 = 10^5 A = spzeros(n, n); A[1, 1] = 1 B = copy(A) for (L, R) in ((A', B'), (transpose(A), transpose(B)), (A, B'), (A', B), (A, transpose(B)), (transpose(A), B), (A', transpose(B))) - @test isequal(L, R) + @test isequal(L, R) # also warms up before timing @test @elapsed(isequal(L, R)) < 0.1 end A[1, 2] = 1; B[2, 1] = 1 diff --git a/test/sparsevector.jl b/test/sparsevector.jl index 735bc253..46479dee 100644 --- a/test/sparsevector.jl +++ b/test/sparsevector.jl @@ -382,6 +382,20 @@ end @test !isequal(spzeros(3), spzeros(4)) end +@testset "hash walks stored entries only" begin + n = 10^9 + v = spzeros(n); v[1] = 1 + hash(v) # warm up + @test @elapsed(hash(v)) < 0.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)