Repository navigation
Keep a non-finite value times an unstored zero in the sparse outer product x .* y' - #1077
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1077 +/- ##
==========================================
- Coverage 94.76% 94.76% -0.01%
==========================================
Files 16 16
Lines 9947 9965 +18
==========================================
+ Hits 9426 9443 +17
- Misses 521 522 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ViralBShah
commented
Oct 13, 2026
ViralBShah
left a comment
Member
Author
There was a problem hiding this comment.
Reviewed by Codex at 92fed66.
No actionable findings in the non-finite outer-product fix. A sweep of 9,604 cases with finite values, infinities, NaNs, stored zeros and dropped zeros found no nonzero/NaN discrepancies against dense results. The guard correctly sends either operand’s non-finite zero products through general broadcasting.
ViralBShah
enabled auto-merge (squash)
October 13, 2026 05:43
… when a stored entry times a zero is not zero Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ViralBShah
force-pushed
the
vs/outer-nonfinite
branch
from
October 13, 2026 05:44
92fed66 to
74b08f0
Compare
40 of 42 tasks
dkarrasch
pushed a commit
that referenced
this pull request
Oct 13, 2026
…oduct `x .* y'` (#1077) `x .* y'` for sparse vectors drops a non-finite product with an unstored zero: `Inf * 0` and `NaN * 0` are `NaN` in the dense result and an unstored zero in the sparse one. `kron(x, y')` goes through the same broadcast and has the same defect. **Before** ```julia julia> x, y = sparsevec([Inf, 0.0]), sparsevec([0.0, 1.0]); julia> Array(x .* y') 2×2 Matrix{Float64}: 0.0 Inf 0.0 0.0 julia> Array(x) .* Array(y)' 2×2 Matrix{Float64}: NaN Inf 0.0 0.0 ``` **After** ```julia julia> Array(x .* y') 2×2 Matrix{Float64}: NaN Inf 0.0 0.0 ``` `Array(kron(x, y'))` changes in the same way and now equals `kron(Array(x), Array(y)')`. The outer-product shortcut `_outer` multiplies stored entries with stored entries alone, which is the product only when every stored entry times a zero is a zero. `_copy(*, x, y')` now checks that with `_outerkeepszeros`, an O(nnz) pass over both vectors, and otherwise takes the general broadcast kernel. Both branches return the same `SparseMatrixCSC` type, so the result stays inferable. `x .* y'` with 10^3 stored `Float64` entries in each vector of length 10^4 (10^6 stored results), minimum of 20 runs on a shared machine: | | time | allocations | |---|---|---| | `main` | 2.87 ms | 9 (16087432 bytes) | | this PR | 2.77 ms | 9 (16087432 bytes) | Left out: `x .* y'` for an eltype without `zero`, such as a vector of matrices, throws a `MethodError` as it does on `main`. Split from #1022. Backport candidate: a wrong result, also on Julia 1.13. Written by Claude Code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 3d2e6df) Backport adjustment: applied by hand to src/higherorderfns.jl, where the outer product dispatches on `SparseVectorUnion` on release-1.13. release-1.13 has no `_densezero`, so the zeros are `zero(eltype(x))` and `zero(eltype(y))`, and `wrapperop` is called qualified. The test is a new testset in test/higherorderfns.jl, unguarded.
ViralBShah
added a commit
that referenced
this pull request
Oct 14, 2026
Backports of the merged PRs labelled `backport 1.13`, cherry-picked with `-x` in the order they were merged on `main`, one per push, with CI checked before the next. Ticked PRs are included and passed CI. - [x] #750 - [x] #762 - [x] #764 - [x] #853 - [x] #884 - [x] #891 - [x] #919 - [x] #925 - [x] #926 - [x] #928 - [x] #933 - [x] #932 - [x] #930 - [x] #921 - [x] #922 - [x] #964 - [x] #942 - [x] #967 - [x] #965 - [x] #960 - [x] #970 - [x] #946 - [x] #944 - [ ] #1034 (not applicable to release-1.13, see below) - [x] #1052 - [x] #1053 - [x] #1068 - [x] #1058 - [ ] #1062 (not applicable to release-1.13, see below) - [x] #1057 - [x] #1071 - [x] #1061 - [x] #1054 - [x] #1060 - [x] #1064 - [x] #1072 - [x] #1059 - [x] #1065 - [x] #1074 - [x] #1079 - [x] #1077 - [x] #1087 ## Adjustments `main` reorganized `src/` and `test/` by area in #872 and #874, after the 1.13 branch point. From #884 on, hunks for `src/indexing.jl`, `src/concatenation.jl`, `src/matmul.jl`, `src/reductions.jl` and `src/constructors.jl` go to the release branch file that holds the same code (mostly `src/sparsematrix.jl`), and tests go to the corresponding release test file. Tests that `main` guards with `@static if COMPREHENSIVE` and checks with `mismatch` are added unguarded and compared with `==`, since `release-1.13` has neither. Each commit message ends with a `Backport adjustment:` line when more than that was needed. Nontrivial ones: - #853: `test/ambiguous.jl` keeps `unbound_args=(; broken=true)`. `release-1.13` has further unbound type parameters (`_absspvec_hcat`, `_getmode`) that `main` fixed in PRs not backported here; the code fix itself is unchanged. - #891: the `_splice_source` hunk and its `copyto!`-into-a-view test are dropped. That code comes from #799, which is not on `release-1.13`. - #928: the `reducedim_init` methods are defined for `AbstractSparseMatrixCSC` and `AbstractCompressedVector` only. On `release-1.13`, reductions over adjoints and views of sparse matrices don't go through the sparse kernels (#798 isn't backported), so the adjoint and view cases and the `sparse = true` keyword test are dropped. - #933: the new tests assert the index type `Int`, since `Ti` isn't bound in that part of `test/cholmod.jl` on `release-1.13`. The `F' \ B` case is dropped: on `release-1.13`, solving with the adjoint of a CHOLMOD factor and a sparse right-hand side still returns a `CHOLMOD.Sparse`. `main` changed that in a PR that isn't labelled for backport. - #930: the `AdjointQRSparse` (`lq`) hunk and its test are dropped, because sparse `lq` isn't on `release-1.13`. The CHOLMOD tests assert the index type `Int`, as for #933. - #921: the reroute to `lu` applies to a dense right-hand side only. On `release-1.13`, UMFPACK can't solve with a sparse right-hand side; `main` added that in a later restructuring of `\`. A sparse right-hand side therefore keeps the previous Hermitian path, which works there, and every sparse case behaves as before. The test checks only values for a sparse right-hand side. It skips the complex transpose case, which fails on `release-1.13` both before and after this change. It drops `@inferred`, because `\` isn't inferred for these eltypes on `release-1.13`. - #942: `release-1.13` has no `CholmodWS` and no workspace keyword on CHOLMOD `ldiv!`, so the `deepcopy` hunk and the workspace parts of the test are dropped. Where an aliased right-hand side is copied, `solve!`/`ldiv!` is called again with the copy instead of rebinding `b`. In `release-1.13`'s UMFPACK matrix kernel `b` is a column view, so rebinding it to a `Vector` made it type-unstable and broke the zero-allocation test in `test/umfpack.jl`. - #960: the `factorize` and `\` checks on a fixed-pattern Hermitian matrix are dropped. With this fix `ishermitian` no longer throws, but on `release-1.13` the call then fails later, in `copytrito!` (it threw before this change too). `main` fixed that later step in PRs that aren't labelled for backport. The test uses a local `same_pattern` helper. - #946: ported by hand. The fixed-pattern part (`_setindex_fixed!`) is dropped: `release-1.13`'s linear-index kernel has no fixed-pattern branch and no `_throwfixedinsert`, both from later fixed-pattern work on `main`. The walk over a sparse mask is defined for `AbstractSparseMatrixCSC{Bool}` and its transposes, since `release-1.13` has no `SparseMatrixCSCOrColumnSubset`; views of a sparse mask take the `findall` path. The fixed-pattern test cases are dropped accordingly. - #1052: ported by hand. `release-1.13` has no `_lower_indices` helper (from #857) and no `_checkpatternwritable`, so the lowering (`to_indices`, `checkbounds`, `ensure_indexable`) is written out in `dropstored!`. - #1057: on `release-1.13`, `M[false:true, 2]` returned a wrong result (a vector of length 2) without throwing. The method goes next to the logical `getindex` methods in `src/sparsematrix.jl`. - #1071: `release-1.13` densifies with its own `_densestructure!`, without `main`'s fixed-pattern checks, so `_checkdensestructure` and `_guardfill` check only the index type and the fill value. `S .= x` with a scalar goes through `fill!` and `_fillnonzero!` on `release-1.13`, not `_densestructure!`, so `_fillnonzero!` gets the same checks before it resizes anything. - #1064: `release-1.13`'s `opnorm` has no 2-norm estimate for a general sparse matrix; `main` added `opnorm2est` in a PR that isn't labelled for backport. Only the up-front check of `p` is ported, with `release-1.13`'s error message. - #1059: on `release-1.13`, a 3-d index array segfaulted in the matrix kernel. `release-1.13` has no `_densezero` (for an element type whose zero is of another type), so an unstored entry is `zero(Tv)`, the result is always an `Array{Tv}`, and the test for such an element type is dropped. - #1074: `release-1.13` has the symmetric/Hermitian kernel as `_mul!` in `src/linalg.jl`, which `main` renamed `_symherm_mul!` and moved to `src/matmul.jl`. The test loads Base's Quaternions test helper directly and uses literal values in place of the `fixturedense` fixture. - #1079: the aliasing check goes into `release-1.13`'s product kernels, which are in `src/linalg.jl`. `release-1.13` has no kernels of its own for dense times symmetric/Hermitian sparse or dense times triangular sparse; those products go through LinearAlgebra's generic code. So those two aliased cases still don't throw there, and their test cases are dropped. Not included: - #1034: not applicable. It fixes the lock that #1029 added to UMFPACK `logabsdet`. #1029 isn't labelled for backport, and on `release-1.13` `logabsdet` takes no lock (neither does `lu!`), so the code it fixes isn't there. - #1062: not applicable without #857, which made sparse `getindex` lower its indices with `to_indices` and isn't labelled for backport. On `release-1.13`, a vector of `CartesianIndex{1}` as a row or column index throws a `MethodError` before reaching the code #1062 changes. Supporting it there would mean adding new `getindex` methods. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01AffnVrPcPNrYXezDCHacNT --------- Co-authored-by: sotashimozono <shimozono-sota631@g.ecc.u-tokyo.ac.jp> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Viral B. Shah <virals@gmail.com> Co-authored-by: Viral B. Shah <ViralBShah@users.noreply.github.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
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.
x .* y'for sparse vectors drops a non-finite product with an unstored zero:Inf * 0andNaN * 0areNaNin the dense result and an unstored zero in the sparse one.kron(x, y')goes through the same broadcast and has the same defect.Before
After
Array(kron(x, y'))changes in the same way and now equalskron(Array(x), Array(y)').The outer-product shortcut
_outermultiplies stored entries with stored entries alone, which is the product only when every stored entry times a zero is a zero._copy(*, x, y')now checks that with_outerkeepszeros, an O(nnz) pass over both vectors, and otherwise takes the general broadcast kernel. Both branches return the sameSparseMatrixCSCtype, so the result stays inferable.x .* y'with 10^3 storedFloat64entries in each vector of length 10^4 (10^6 stored results), minimum of 20 runs on a shared machine:mainLeft out:
x .* y'for an eltype withoutzero, such as a vector of matrices, throws aMethodErroras it does onmain.Split from #1022. Backport candidate: a wrong result, also on Julia 1.13.
Written by Claude Code.
🤖 Generated with Claude Code