Skip to content

Make sparse and spdiagm statically dispatchable / --trim compatible - #751

Merged
araujoms merged 1 commit into
JuliaSparse:mainfrom
topolarity:ct/trim-static-dispatch
Sep 2, 2026
Merged

Make sparse and spdiagm statically dispatchable / --trim compatible#751
araujoms merged 1 commit into
JuliaSparse:mainfrom
topolarity:ct/trim-static-dispatch

Conversation

@topolarity

Copy link
Copy Markdown
Contributor

This allows these basic SparseArrays constructors to be --trim compatible:

    A = sparse([1, 2, 3], [1, 2, 3], [1.0, 2.0, 3.0])
    B = spdiagm(0 => ones(3), 1 => ones(2))

This commit was written with the assistance of generative AI (Claude) 🤖

@topolarity

Copy link
Copy Markdown
Contributor Author

Tests for trimmability will be added downstream in the Julia dep bump

@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.85%. Comparing base (ce9fe78) to head (e8dc728).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/sparsematrix.jl 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #751      +/-   ##
==========================================
- Coverage   84.39%   83.85%   -0.55%     
==========================================
  Files          13       13              
  Lines        9382     9387       +5     
==========================================
- Hits         7918     7871      -47     
- Misses       1464     1516      +52     

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

dkarrasch
dkarrasch previously approved these changes Aug 28, 2026
@ViralBShah

Copy link
Copy Markdown
Member

Is this good to merge?

@ViralBShah ViralBShah added the backport 1.13 Change should be backported to release-1.13 label Aug 30, 2026
@topolarity

Copy link
Copy Markdown
Contributor Author

It is from my POV but I do not have merge permissions. @dkarrasch ?

@araujoms

araujoms commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

@dkarrasch is taking a break.

Can't you add an @inferred test to make sure --trim will keep working?

@topolarity

Copy link
Copy Markdown
Contributor Author

We could, but @inferred checks the return type which was already good before this fix

@araujoms

araujoms commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

As far as I understand what wasn't well-inferred before was the eltype of V, so an @inferred on spdiagm_eltype would be useful, no?

Two constructors defeated static call resolution (e.g. under juliac's
`--trim`) for reasons invisible at the call site:

The `sparse(I, J, V, m, n, combine)` methods took `combine` as an
unconstrained, unspecialized argument, so the pass-through call into the
`Tv`/`Ti`-typed method was made on an abstractly-typed function value.
Parameterizing the methods on `combine::F` specializes the pass-through
and the call chain resolves statically.

`spdiagm`'s eltype computation mapped `eltype` over the `kv::Pair...`
tuple; under Vararg widening the compiled signature is
`(::Pair, ::Vararg{Pair})`, for which the tuple `map` itself cannot be
resolved. A dispatch-based `spdiagm_eltype` (pairwise recursion, with a
homogeneous fast path that binds the common eltype) mirrors
`Base.promote_eltypeof` and resolves statically for every arity.

Both changes only sharpen dispatch; behavior is unchanged. Compiling

    A = sparse([1, 2, 3], [1, 2, 3], [1.0, 2.0, 3.0])
    B = spdiagm(0 => ones(3), 1 => ones(2))

with `juliac --output-exe --trim=safe` fails before this change with ten
verifier errors (unresolved `sparse!`, the tuple `map`, its
`_apply_iterate` splat, and a `Vector{_A}` allocation whose type never
became concrete) and verifies cleanly with it.

This commit was written with the assistance of generative AI (Claude).

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

Copy link
Copy Markdown
Contributor Author

@inferred doesn't quite work due to a technicality with Type{...}, but I've added a Base.infer_return_type test now

@araujoms

araujoms commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Thanks. It's not quite what we needed but it'll have to do.

@araujoms
araujoms merged commit 88fa040 into JuliaSparse:main Sep 2, 2026
10 of 11 checks passed
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.

4 participants