Skip to content

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

Open
ViralBShah wants to merge 1 commit into
mainfrom
vs/fix-findprev-import
Open

Fix findnext/findprev for sparse arrays to extend Base, making hash fast#769
ViralBShah wants to merge 1 commit into
mainfrom
vs/fix-findprev-import

Conversation

@ViralBShah

@ViralBShah ViralBShah commented Sep 8, 2026

Copy link
Copy Markdown
Member

Fixes #570

Follow-up to #766 and #768, addressing the last relative of isequal that still visited every element: hash.

Root cause. The sparse findnext/findprev methods in src/abstractsparse.jl have been dead code since the JuliaLang/julia import cleanup (JuliaLang/julia#42894) dropped findnext, findprev from the import Base: list. They defined a local SparseArrays.findprev instead of extending Base.findprev, so every call fell through to the generic elementwise method. The existing tests compared against dense results and so never noticed.

Why this matters for hash. Base.hash on large arrays (_hash_fib) skips runs of equal values with findprev(!isequal(elt), A, i). On a sparse array that call is where all the time goes. With the import restored it jumps straight to the previous stored entry.

Call Before After
hash of a length 10^9 sparse vector, one stored entry ~1 s 2 µs
hash of a 10^4 x 10^4 sparse matrix, one stored entry 81 ms 4 µs

Hash values are unchanged: this PR does not touch hash itself, so a sparse array still hashes identically to its dense counterpart, and explicitly stored zeros do not affect the value. Verified with randomized checks including NaN and signed zeros, and now covered by tests.

Changes:

  • Add findnext, findprev back to the import Base: list.
  • Test that the sparse methods actually extend Base, and that findnext/findprev with !isequal(x) predicates agree with dense for NaN and signed zeros, including a timing guard on a 10^9 vector.
  • Test that hash of sparse vectors and matrices matches dense (with and without a seed, with stored zeros, NaN and -0.0), plus a timing guard.

Based on #768 because both touch the same test files; it will rebase cleanly onto main once that merges. Full test suite passes on nightly and Test.detect_ambiguities(SparseArrays) remains empty.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LgBHUw9Hp7YW5ub29B4R5y

@ViralBShah

Copy link
Copy Markdown
Member Author

This should close #570.

@fredrikekre Can I ask you for a review?

Base automatically changed from vs/isequal-adjoint-sparse to main September 8, 2026 09:46
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LgBHUw9Hp7YW5ub29B4R5y
@ViralBShah
ViralBShah force-pushed the vs/fix-findprev-import branch from 5e913ce to 2ae3698 Compare September 8, 2026 09:48
@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.58%. Comparing base (53690b3) to head (2ae3698).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #769      +/-   ##
==========================================
+ Coverage   84.19%   84.58%   +0.39%     
==========================================
  Files          13       13              
  Lines        9403     9401       -2     
==========================================
+ Hits         7917     7952      +35     
+ Misses       1486     1449      -37     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ViralBShah added a commit that referenced this pull request Sep 8, 2026
Follow-up to #766 and #768 (and item 6 of the test-time review).

The `@elapsed(...) < 0.1` assertions guarding the stored-entries-only
`isequal`/`==` paths are wall-clock thresholds, which are the main flake
risk on loaded CI runners. The matrix variants also build `10^5 x 10^5`
matrices (an 800 KB column-pointer array each) for seven wrapper
combinations.

They are replaced with a deterministic check. `sparsematrix_ops.jl`
already has a `Counting` eltype whose `==` increments a global counter;
this PR gives it `isequal` too and asserts that comparing two sparse
arrays performs at most `nnz(A) + nnz(B)` element comparisons. The
generic `AbstractArray` fallback would perform `length(A)` of them, so a
regression to it fails the test regardless of machine speed. The check
covers vectors, matrices, and all adjoint/transpose combinations, for
both `==` and `isequal`.

The existing NaN, signed-zero, stored-zero and dimension-mismatch
semantics tests are unchanged apart from running at small sizes.

#769 adds similar timing guards for `findprev` and `hash`; I'll convert
those the same way once it merges.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01LgBHUw9Hp7YW5ub29B4R5y

Co-authored-by: Viral B. Shah <ViralBShah@users.noreply.github.com>
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hash(SparseVector) very slow

1 participant