Make isequal on sparse vectors and matrices walk stored entries only - #766
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #766 +/- ##
==========================================
+ Coverage 84.16% 84.19% +0.02%
==========================================
Files 13 13
Lines 9393 9400 +7
==========================================
+ Hits 7906 7914 +8
+ Misses 1487 1486 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`isequal` on `SparseVector`/`SparseMatrixCSC` fell back to the generic `AbstractArray` method, which visits every element, so comparing two length-10^9 vectors with a single stored entry took ~1.5s. Refactor the existing sparse `==` implementations into an `_iseq(eq, A, B)` helper parameterized on the elementwise predicate, and define `isequal` on top of it. Unmatched stored entries are compared with the predicate against `zero(x)`, so `isequal` keeps its dense semantics for `NaN` and signed zeros (`isequal(-0.0, 0.0) == false`). Fixes #561 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XzN6CtuVBJWDVc88ShNYso
ViralBShah
force-pushed
the
vs/isequal-sparse
branch
from
September 7, 2026 23:58
94ea840 to
1d61cc1
Compare
This was referenced Sep 8, 2026
ViralBShah
added a commit
that referenced
this pull request
Sep 8, 2026
Follow-up to #766, which added stored-entry-only `isequal` for `SparseMatrixCSC` and for the `Adjoint`/`Transpose` wrappers of sparse *vectors*, but not for the wrappers of sparse *matrices*. Those still fell back to the generic elementwise `AbstractArray` method even though `==` has dedicated methods for them. On two `10^5 x 10^5` matrices with one stored entry each: | Call | Before | After | |---|---|---| | `isequal(A', B')` | 14.1 s | 0.2 ms | | `isequal(A, B')` | 14.1 s | 0.1 ms | Changes: - Parameterize `nzeq` on the elementwise predicate and route the CSC-vs-wrapper `==` through the same `_iseq(eq, A, B)` helper introduced in #766. - Add `isequal` methods for `Adjoint`/`Transpose` of sparse matrices mirroring the existing `==` ones. - Loosen the indexed argument of `nzeq` to `AbstractMatrix`. This also fixes a pre-existing `MethodError` in `A' == transpose(B)` for **complex** `A`: the adjoint of a transpose does not collapse for complex eltypes, so it yields a nested `Adjoint{<:Any,<:Transpose}` that the old one-level signature rejected. (Real eltypes were unaffected since `adjoint(::Transpose{<:Real})` returns the parent.) - Tests cover all wrapper combinations, a timing guard, and cross-check `isequal`/`==` against `Matrix` for NaN, signed zeros, stored zeros, and conjugation. `Test.detect_ambiguities(SparseArrays)` remains empty. 🤖 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>
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>
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
1 participant
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.
isequalonSparseVectorandSparseMatrixCSCfell back to the genericAbstractArraymethod, which visits every element. The example from #561 (two length-10^9 vectors with one stored entry each) took ~1.5s; it now takes microseconds.Changes:
==for compressed vectors and CSC matrices into an_iseq(eq, A, B)helper parameterized on the elementwise predicate.isequalforAbstractCompressedVector,AbstractSparseMatrixCSC, and theAdjoint/Transposevector wrappers (mirroring the existing==methods) on top of that helper.zero(x), soisequalkeeps its dense-array semantics:isequal(NaN, NaN)is true andisequal(-0.0, 0.0)is false, including a stored-0.0against an implicit zero. Usingzero(x)rather thanzero(eltype(A))keeps non-numeric element types such asAny/Numberworking.isequaland==against the dense results for NaN, signed zeros, stored zeros, and mixed element types, plus a timing guard on the issue's example.Test.detect_ambiguities(SparseArrays)remains empty.Fixes #561
🤖 Generated with Claude Code
https://claude.ai/code/session_01XzN6CtuVBJWDVc88ShNYso