Fix findnext/findprev for sparse arrays to extend Base, making hash fast - #769
Open
ViralBShah wants to merge 1 commit into
Open
Fix findnext/findprev for sparse arrays to extend Base, making hash fast#769ViralBShah wants to merge 1 commit into
findnext/findprev for sparse arrays to extend Base, making hash fast#769ViralBShah wants to merge 1 commit into
Conversation
Member
Author
|
This should close #570. @fredrikekre Can I ask you for a review? |
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
force-pushed
the
vs/fix-findprev-import
branch
from
September 8, 2026 09:48
5e913ce to
2ae3698
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #570
Follow-up to #766 and #768, addressing the last relative of
isequalthat still visited every element:hash.Root cause. The sparse
findnext/findprevmethods insrc/abstractsparse.jlhave been dead code since the JuliaLang/julia import cleanup (JuliaLang/julia#42894) droppedfindnext, findprevfrom theimport Base:list. They defined a localSparseArrays.findprevinstead of extendingBase.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.hashon large arrays (_hash_fib) skips runs of equal values withfindprev(!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.hashof a length10^9sparse vector, one stored entryhashof a10^4 x 10^4sparse matrix, one stored entryHash values are unchanged: this PR does not touch
hashitself, 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:
findnext, findprevback to theimport Base:list.Base, and thatfindnext/findprevwith!isequal(x)predicates agree with dense for NaN and signed zeros, including a timing guard on a10^9vector.hashof 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
mainonce that merges. Full test suite passes on nightly andTest.detect_ambiguities(SparseArrays)remains empty.🤖 Generated with Claude Code
https://claude.ai/code/session_01LgBHUw9Hp7YW5ub29B4R5y