Validate before densifying the destination of `map!` and `broadcast!` by ViralBShah · Pull Request #1071 · JuliaSparse/SparseArrays.jl

ViralBShah · 2026-10-13T03:07:11Z

In-place map! and broadcast! with a function that is not zero at the zeros store every entry of the destination. They densified it first and only then hit failures that could have been found beforehand, so a throw left the destination corrupt: column pointers half rewritten, or a full pattern with uninitialized values.

using SparseArrays
C = spzeros(Float64, Int8, 12, 12)
try; C .= 1.0; catch e; println(typeof(e)); end
println(SparseArrays.getcolptr(C))
Ci = sparse([1 0; 0 2])
try; Ci .= Ci .+ 0.5; catch e; println(typeof(e)); end
println(nnz(Ci), " ", Ci == [1 0; 0 2])

Before

InexactError
Int8[1, 13, 25, 37, 49, 61, 73, 85, 97, 109, 121, 1, 1]
InexactError
4 false

After

InexactError
Int8[1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
InexactError
2 true

_densestructure! resized the buffers and wrote the column pointers before the last one, m * n + 1, turned out not to fit the index type, and the fill kernels called it before fill! converted the fill value to the element type. _checkdensestructure now holds what _densestructure! checks before it writes anything (the fixed-pattern check, and for a matrix that the last column pointer converts), and _guardfill runs it and the conversion of the fill value ahead of _map_notzeropres!, _broadcast_notzeropres! and the zero-argument C .= f.() path. The conversion is skipped for an empty destination, which stores nothing. Fixed destinations throw the same errors as before. map!(cos, C, A) and C .= A .+ 1.0 at 10^6 entries take the same time and allocations as on main.

Left out: an f that throws part-way through the kernel, after the destination is densified, still leaves it with values that are not its own. That is a follow-up stacked on this branch.

Split from #1022. A backport candidate: the destination is corrupt after the throw.

Written by Claude Code.

🤖 Generated with Claude Code

… writing it A function that is not zero at the zeros stores every entry of the destination. `_densestructure!` rewrote the column pointers before finding that the last one does not fit the index type, and the kernels densified before finding that the fill value does not convert to the element type. Both are now checked first, by `_checkdensestructure` and `_guardfill`, and leave the destination untouched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

codecov · 2026-10-13T04:05:26Z

Codecov Report

❌ Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.78%. Comparing base (c0f75f0) to head (049609c).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
src/higherorderfns.jl	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1071      +/-   ##
==========================================
+ Coverage   94.75%   94.78%   +0.02%     
==========================================
  Files          16       16              
  Lines        9885     9893       +8     
==========================================
+ Hits         9367     9377      +10     
+ Misses        518      516       -2

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

…!` (#1082) When `f`, or the conversion of its result to the destination's element type, throws part-way through an in-place `map!` or `broadcast!`, the sparse destination is left with a pattern that is half-written: column pointers of the new result next to those of the old one. The next use of it fails an internal assertion or reads the wrong entries. **Before** (on #1071; the same on `main` and Julia 1.13) ```julia julia> B = sparse([1.0 0 0 4; 0 2 0 0; 0 0 3 0; 1 1 0 0]); julia> B .= ((x, y) -> x == 3 ? error("boom") : x + y).(B, sparse(ones(4, 4))) ERROR: boom julia> Array(B) ERROR: AssertionError: _goodbuffers(S) ``` **After** ```julia julia> B .= ((x, y) -> x == 3 ? error("boom") : x + y).(B, sparse(ones(4, 4))) ERROR: boom julia> Array(B) 4×4 Matrix{Float64}: 0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0 julia> nnz(B) 0 ``` The kernels rebuild the pattern of the destination, or densify it, as they go, so a throw from `f` leaves it between two patterns. The dispatchers of `map!` and `broadcast!` now call the kernel inside a `try` (`_guardpattern` for the zero-preserving kernels, the existing `_guardfill` for those that fill, and the zero-argument `C .= f.()` path), and on a throw empty the destination before rethrowing. The kernels themselves are unchanged. Behaviour to review: after a throw, a destination that is not fixed is in one of three states, never structurally invalid. - **Untouched** when the failure can be found first: the index type cannot address every entry, or the fill value `f(0)` does not convert. That is #1071. - **Partly written**, as a dense destination is, for one argument whose pattern the destination already has (`C .= f.(C)`). This is what `main` does. - **Empty** otherwise, since the pattern, or the filled values, are then no longer the destination's own. It is not restored. A fixed destination keeps its pattern throughout, as before. Truncating at the failing column instead of emptying needs loop state in the `catch`, which costs the hot loop. The `try` is outside the kernels. Minimum of 15 runs, time in ms and allocated bytes, `n = 10^5` square with about 10^6 stored entries per argument (`map!(cos, C, A)`: a 1000×1000 destination, all 10^6 entries stored): | | #1071 | this PR | |---|---|---| | `map(sin, A)` | 10.548, 16815672 | 10.458, 16815672 | | `map(+, A, B)` | 15.268, 32839224 | 15.249, 32839224 | | `A .* B` | 12.570, 32839320 | 12.599, 32839320 | | `map!(+, C, A, B)` | 12.577, 48 | 12.800, 48 | | `C .= A .+ B` | 12.563, 144 | 12.689, 144 | | `map!(cos, C, A)` | 0.207, 48 | 0.211, 48 | The out-of-place rows do not go through the new code. The inferred result types of all six are unchanged. Split from #1022. Stacked on #1071 (`vs/densify-validate-first`); only the last two commits are new. Backport: this fixes a corrupted destination, but it also changes what a destination holds after a throw (empty, where it used to be invalid but with some of its old entries readable), so it is a behaviour change as well as a fix. Written by Claude Code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

…#1071) In-place `map!` and `broadcast!` with a function that is not zero at the zeros store every entry of the destination. They densified it first and only then hit failures that could have been found beforehand, so a throw left the destination corrupt: column pointers half rewritten, or a full pattern with uninitialized values. ```julia using SparseArrays C = spzeros(Float64, Int8, 12, 12) try; C .= 1.0; catch e; println(typeof(e)); end println(SparseArrays.getcolptr(C)) Ci = sparse([1 0; 0 2]) try; Ci .= Ci .+ 0.5; catch e; println(typeof(e)); end println(nnz(Ci), " ", Ci == [1 0; 0 2]) ``` **Before** ``` InexactError Int8[1, 13, 25, 37, 49, 61, 73, 85, 97, 109, 121, 1, 1] InexactError 4 false ``` **After** ``` InexactError Int8[1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1] InexactError 2 true ``` `_densestructure!` resized the buffers and wrote the column pointers before the last one, `m * n + 1`, turned out not to fit the index type, and the fill kernels called it before `fill!` converted the fill value to the element type. `_checkdensestructure` now holds what `_densestructure!` checks before it writes anything (the fixed-pattern check, and for a matrix that the last column pointer converts), and `_guardfill` runs it and the conversion of the fill value ahead of `_map_notzeropres!`, `_broadcast_notzeropres!` and the zero-argument `C .= f.()` path. The conversion is skipped for an empty destination, which stores nothing. Fixed destinations throw the same errors as before. `map!(cos, C, A)` and `C .= A .+ 1.0` at 10^6 entries take the same time and allocations as on `main`. Left out: an `f` that throws part-way through the kernel, after the destination is densified, still leaves it with values that are not its own. That is a follow-up stacked on this branch. Split from #1022. A backport candidate: the destination is corrupt after the throw. 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 4cbae1e) Backport adjustment: release-1.13 densifies with its own `_densestructure!` in src/higherorderfns.jl (without the fixed-pattern checks of main), so `_checkdensestructure` and `_guardfill` are defined there and check only the index type and the fill value. `S .= x` with a scalar goes through `fill!` and `_fillnonzero!` on release-1.13, not through `_densestructure!` as on main, so `_fillnonzero!` gets the same checks before it resizes anything. The test is a new testset in test/higherorderfns.jl that compares with `==`.

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>

ViralBShah mentioned this pull request Oct 13, 2026

Leave a valid destination when f throws inside map! or broadcast! #1082

Merged

Trim the densify-before-validate tests to one case per check

049609c

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

ViralBShah mentioned this pull request Oct 13, 2026

Fix wrong results in sparse prod, mapreduce and findmin, and make map!/broadcast! safe with missing, fixed destinations and a throwing f #1022

Closed

ViralBShah enabled auto-merge (squash) October 13, 2026 03:27

ViralBShah merged commit 4cbae1e into main Oct 13, 2026
8 checks passed

ViralBShah deleted the vs/densify-validate-first branch October 13, 2026 04:05

dkarrasch added the backport 1.13 Change should be backported to release-1.13 label Oct 13, 2026

dkarrasch mentioned this pull request Oct 13, 2026

Backports for release 1.13 #1089

Merged

40 of 42 tasks

ViralBShah removed the backport 1.13 Change should be backported to release-1.13 label Oct 27, 2026

Navigation Menu

Repository navigation

Validate before densifying the destination of `map!` and `broadcast!` - #1071

Validate before densifying the destination of `map!` and `broadcast!`#1071
ViralBShah merged 2 commits into
mainfrom
vs/densify-validate-first

ViralBShah commented Oct 13, 2026

Uh oh!

Uh oh!

codecov Bot commented Oct 13, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ViralBShah commented Oct 13, 2026

Uh oh!

Uh oh!

codecov Bot commented Oct 13, 2026

Codecov Report

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants