Skip to content

Keep a non-finite value times an unstored zero in the sparse outer product x .* y' - #1077

Merged
ViralBShah merged 2 commits into
mainfrom
vs/outer-nonfinite
Oct 13, 2026
Merged

ViralBShah merged 2 commits into
mainfrom
vs/outer-nonfinite

Conversation

@ViralBShah

Copy link
Copy Markdown
Member

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> 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> 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

@codecov

codecov Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.76%. Comparing base (6324de6) to head (74b08f0).
⚠️ Report is 4 commits behind head on main.

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.
📢 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 ViralBShah left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
ViralBShah enabled auto-merge (squash) October 13, 2026 05:43
ViralBShah and others added 2 commits 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
ViralBShah merged commit 3d2e6df into main Oct 13, 2026
8 checks passed
@ViralBShah
ViralBShah deleted the vs/outer-nonfinite branch October 13, 2026 05:54
@dkarrasch dkarrasch added the backport 1.13 Change should be backported to release-1.13 label Oct 13, 2026
@dkarrasch dkarrasch mentioned this pull request Oct 13, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport 1.13 Change should be backported to release-1.13

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants