Skip to content

Return NaN from prod of a sparse matrix that stores an Inf or NaN next to a zero - #1065

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

ViralBShah merged 2 commits into
mainfrom
vs/prod-nonfinite

Conversation

@ViralBShah

Copy link
Copy Markdown
Member

prod of a sparse matrix with an unstored entry returns zero without looking at the stored ones, but a stored Inf or NaN times zero is NaN, which is what the dense prod gives.

Before (main, and Julia 1.13)

julia> prod(sparse([0.0 Inf; 1.0 2.0])), prod([0.0 Inf; 1.0 2.0])
(0.0, NaN)

julia> prod(sparse([0.0 NaN; 1.0 2.0]); dims=(1, 2))
1×1 Matrix{Float64}:
 0.0

After

julia> prod(sparse([0.0 Inf; 1.0 2.0])), prod([0.0 Inf; 1.0 2.0])
(NaN, NaN)

julia> prod(sparse([0.0 NaN; 1.0 2.0]); dims=(1, 2))
1×1 Matrix{Float64}:
 NaN

The * kernel of Base._mapreduce for a SparseMatrixCSC or a column view of one short-circuits when the unstored entries multiply to zero. That is kept only where f returns an integer (Base.promote_op(f, T) <: Integer), which has no value that survives a zero. Otherwise the zero is multiplied by the product of the stored entries. That product can overflow to Inf on its own and turn the zero into a NaN that the dense fold never sees, so when the result is nonzero the stored entries are folded into the zero one at a time instead: prod(sparse([0.0 1e200; 1e200 1.0])) is still 0.0.

The cost is that prod of a float matrix with unstored entries now reads the stored entries, O(nnz) where it was O(1). For a 2000×2000 Float64 matrix with 10^6 stored entries:

time allocations
main 0.00006 ms 0 bytes
this PR 0.096 ms 0 bytes

Left out: prod of a sparse vector and prod(A; dims=1) or dims=2 go through other kernels and still give NaN where the stored entries of the vector or slice overflow next to a zero, as on main (prod(sparsevec([0.0, 1e200, 1e200])) is NaN, dense gives 0.0).

Split from #1022. This is a wrong result on released Julia, so it is a backport candidate.

Written by Claude Code.

🤖 Generated with Claude Code

ViralBShah and others added 2 commits October 13, 2026 03:05
…NaN` next to a zero

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@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.79%. Comparing base (c0f75f0) to head (676fb30).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1065      +/-   ##
==========================================
+ Coverage   94.75%   94.79%   +0.03%     
==========================================
  Files          16       16              
  Lines        9885     9885              
==========================================
+ Hits         9367     9370       +3     
+ Misses        518      515       -3     

☔ 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 676fb30.

No actionable findings within the stated scope. Restricting the zero shortcut to integer mapped values allows stored non-finites to participate, and the fallback avoids an artificial NaN caused solely by overflow while reducing finite stored entries separately.

@ViralBShah
ViralBShah merged commit 775677b into main Oct 13, 2026
10 checks passed
@ViralBShah
ViralBShah deleted the vs/prod-nonfinite branch October 13, 2026 05:35
@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
…NaN` next to a zero (#1065)

`prod` of a sparse matrix with an unstored entry returns zero without
looking at the stored ones, but a stored `Inf` or `NaN` times zero is
`NaN`, which is what the dense `prod` gives.

**Before** (`main`, and Julia 1.13)
```julia
julia> prod(sparse([0.0 Inf; 1.0 2.0])), prod([0.0 Inf; 1.0 2.0])
(0.0, NaN)

julia> prod(sparse([0.0 NaN; 1.0 2.0]); dims=(1, 2))
1×1 Matrix{Float64}:
 0.0
```

**After**
```julia
julia> prod(sparse([0.0 Inf; 1.0 2.0])), prod([0.0 Inf; 1.0 2.0])
(NaN, NaN)

julia> prod(sparse([0.0 NaN; 1.0 2.0]); dims=(1, 2))
1×1 Matrix{Float64}:
 NaN
```

The `*` kernel of `Base._mapreduce` for a `SparseMatrixCSC` or a column
view of one short-circuits when the unstored entries multiply to zero.
That is kept only where `f` returns an integer (`Base.promote_op(f, T)
<: Integer`), which has no value that survives a zero. Otherwise the
zero is multiplied by the product of the stored entries. That product
can overflow to `Inf` on its own and turn the zero into a `NaN` that the
dense fold never sees, so when the result is nonzero the stored entries
are folded into the zero one at a time instead: `prod(sparse([0.0 1e200;
1e200 1.0]))` is still `0.0`.

The cost is that `prod` of a float matrix with unstored entries now
reads the stored entries, O(nnz) where it was O(1). For a 2000×2000
`Float64` matrix with 10^6 stored entries:

| | time | allocations |
|---|---|---|
| `main` | 0.00006 ms | 0 bytes |
| this PR | 0.096 ms | 0 bytes |

Left out: `prod` of a sparse vector and `prod(A; dims=1)` or `dims=2` go
through other kernels and still give `NaN` where the stored entries of
the vector or slice overflow next to a zero, as on `main`
(`prod(sparsevec([0.0, 1e200, 1e200]))` is `NaN`, dense gives `0.0`).

Split from #1022. This is a wrong result on released Julia, so it is a
backport candidate.

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 775677b)

Backport adjustment: applied to src/sparsematrix.jl and test/sparsematrix_ops.jl (for src/reductions.jl and test/reductions.jl).
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