Rationalize the dispatch type aliases - #816
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #816 +/- ##
==========================================
+ Coverage 92.55% 92.61% +0.05%
==========================================
Files 12 12
Lines 8501 8514 +13
==========================================
+ Hits 7868 7885 +17
+ Misses 633 629 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Collect the cross-file Union aliases into one section of abstractsparse.jl, drop the duplicates, and give the survivors one naming scheme. - SparseVecOrMat/_SparseArraysCSC, StructuredMatrix/_SpecialArrays, _SparseVectorUnion/SparseVectorUnion, DenseViewWrappers/WrapperMatrixTypes and SparseTriangular/AbstractTriangularSparse each collapse to one alias. - "X or Adjoint/Transpose of X" is spelled <X>MaybeAdjOrTrans throughout (after LinearAlgebra's StridedMaybeAdjOrTransMat) and built from AdjOrTrans. - HigherOrderFns.StructuredMatrix, which shadowed the larger LinearAlgebra.StructuredMatrix, becomes DiagBiTriSym. - SparseMatrixCSCUnion2 becomes SparseMatrixCSCOrColumnSubset. - SorF is gone; SparseMatrixCSC and FixedSparseCSC define their accessors. - SparseTriangular now uses UpperOrLowerTriangular, so the unit triangular wrappers take the Gustavson product path (materialized via sparse(A)) and Tri*Tri products keep LinearAlgebra's result wrapper. - nnz and indtype are extended to triangular wrappers of column-range views, which previously threw MethodError in sparse products. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MFPoSXGFJAGgJrHT98z2AY
The CHOLMOD-local alias is the concrete SparseVector/SparseMatrixCSC pair that Sparse(B) accepts, so it cannot reuse the top-level abstract alias of the same name; give it a distinct name and make it const. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MFPoSXGFJAGgJrHT98z2AY
Replaces the stale 'unused internally' comment that the alias move dropped. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MFPoSXGFJAGgJrHT98z2AY
SparseMatrixCSCUnion, SparseVectorUnion and AdjOrTransSparseVectorUnion become SparseMatrixCSCOrView, SparseVectorOrView and AdjOrTransSparseVectorOrView, matching SparseMatrixCSCOrColumnSubset. The old names stay as aliases for downstream packages, noted as candidates for future deprecation. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MFPoSXGFJAGgJrHT98z2AY
BiTriSym was redefined identically, DiagBiTriSym duplicated LinearAlgebra.BandedMatrix, and CHOLMOD's RealHermSymComplexHermSSL can be built on RealHermSymComplexHerm. The inline spellings of the vec-or-mat union in the broadcast style rules, fkeep! and fill! now use SparseVecOrMat, and the unused AdjOrTransSparseVectorUnion compat alias is dropped since no package references it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MFPoSXGFJAGgJrHT98z2AY
- ATLU becomes UMFAdjOrTransLU and SPVM becomes SparseVecOrMatStyle. - WrapperMatrixTypes becomes MatrixWrappersOrView, matching the OrView scheme. - SparseOrStructuredMatrix is built on SparseMatrixCSCOrView; the existing all-sparse map/map! disambiguation methods widen to AbstractSparseMatrixCSC so they stay more specific than both competing methods. - Aliases sit where they are used: LinAlgLeftQs moves to the shared section, SparseVectorOrMatrixCSC to the top of cholmod.jl, and the single-use SuiteSparseStruct is inlined. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MFPoSXGFJAGgJrHT98z2AY
75549bd to
2df10d2
Compare
ViralBShah
left a comment
There was a problem hiding this comment.
Found two P2 regressions in the expanded unit-triangular multiplication path, reproduced against base revision 6096fc0.
Validation: the linalg, higherorderfns, and fixed suites passed 9,033 tests, with one marked broken. Additional alias and custom-CSC mapping checks passed.
There was a problem hiding this comment.
🔵 Needs a closer look
The broad cross-file dispatch changes and new triangular multiplication paths warrant final human validation.
Pull request overview
Consolidates sparse dispatch aliases, removes duplicates, and reuses LinearAlgebra aliases while extending triangular sparse-view multiplication.
Changes:
- Centralizes shared dispatch aliases in
abstractsparse.jl. - Renames and simplifies local aliases across linear algebra and solver code.
- Adds coverage for triangular sparse matrices and column views.
File summaries
| File | Description |
|---|---|
src/abstractsparse.jl |
Centralizes shared aliases. |
src/SparseArrays.jl |
Imports LinearAlgebra aliases. |
src/sparsematrix.jl |
Migrates CSC dispatch aliases. |
src/sparsevector.jl |
Migrates sparse-vector aliases. |
src/sparseconvert.jl |
Reuses shared wrapper aliases. |
src/linalg.jl |
Consolidates multiplication and kron dispatch. |
src/higherorderfns.jl |
Simplifies broadcast and map dispatch. |
src/solvers/cholmod.jl |
Renames CHOLMOD-local aliases. |
src/solvers/umfpack.jl |
Renames the UMFPACK wrapper alias. |
test/linalg.jl |
Tests triangular sparse-view multiplication. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…lar path - Materializing a unit-triangular wrapper adds n stored entries, which may not fit the parent's index type; widen the temporary to Int when needed and take the result index type from the original operands. - The sparse-vector fast path only accepts compressed vectors and whole column/vector views, so other AbstractSparseVectors take the generic product as they did on main for unit-triangular wrappers. - Drop the stale TODO about column-range views; SparseOrTri covers them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MFPoSXGFJAGgJrHT98z2AY
Aliases use the Sparse family name and accept the abstract tier of their base type, as SparseVecOrMat and SparseTriangular already did. The only alias whose Abstract prefix distinguished it from a sibling had a single use in CHOLMOD and is inlined there. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MFPoSXGFJAGgJrHT98z2AY
8397ab6 to
9dd7c61
Compare
The
Unionaliases used for dispatch had grown to 36 across 7 files, with duplicates, near-duplicates that hid semantic differences, and no naming convention. This collects the cross-file ones into one section ofabstractsparse.jl, drops the duplicates, reuses LinearAlgebra's aliases where they already exist, and gives the survivors one scheme. Solver scalar-type aliases stay with their solvers.Before and after
Counting every
const X = Union{...}insrc:mainUnionaliasesITypes,VTypes,UMFITypes, ...)The 23 dispatch aliases are now: 9 in the shared section of
abstractsparse.jl(AbstractSparseVecOrMat,SparseVecOrMat,SparseMatrixCSCOrView,SparseMatrixCSCOrColumnSubset,SparseVectorOrView, two<X>MaybeAdjOrTrans,SparseOrTri,LinAlgLeftQs), 8 local tolinalg.jl(DenseMatrixUnion,DenseInputVector,MatrixWrappers,MatrixWrappersOrView,QuasiSparseMatrix,QuasiStridedMatrix, the two kron groups), 3 in CHOLMOD (StridedVecOrMatMaybeAdjOrTrans,SparseVectorOrMatrixCSC,FactorComponentRHS),UMFAdjOrTransLUin UMFPACK, andSparseVecOrMatStyle,SparseOrStructuredMatrixand the two concat groups. Placement follows one rule: used from more than one file goes in the shared section, used across a file goes at the top of that file, used by one section sits at the head of that section. A further three aliases (SparseTriangular,SparseMatrixCSCSymmHerm, CHOLMOD'sRealHermSymComplexHermSSL) no longer spell out aUnionbecause they are built on a LinearAlgebra alias.Consolidation
SparseVecOrMat/_SparseArraysCSC,StructuredMatrix/_SpecialArrays,_SparseVectorUnion/SparseVectorUnion,DenseViewWrappers/WrapperMatrixTypes, andSparseTriangular/AbstractTriangularSparseeach collapse to one alias.Adjoint/Transposeof X" is spelled<X>MaybeAdjOrTransthroughout (after LinearAlgebra'sStridedMaybeAdjOrTransMat) and built fromAdjOrTrans.<X>OrView/<X>OrColumnSubset. Alias names use theSparsefamily name and accept the abstract tier of their base type; only the concrete CHOLMOD alias says otherwise in its name:SparseMatrixCSCUnionbecomesSparseMatrixCSCOrView,SparseMatrixCSCUnion2becomesSparseMatrixCSCOrColumnSubset,SparseVectorUnionbecomesSparseVectorOrView, andAdjOrTransSparseVectorUnionbecomesAdjOrTransSparseVectorOrView. The two-tier CSC split stays: thegetcolptr-based kernels need a unit-range column view, thenzrange-based ones accept any column subset (Fix performance trap for sparse view multiplication #476).SorFis gone;SparseMatrixCSCandFixedSparseCSCeach define their accessors._SparseKronGroup/_DenseKronGroup.fkeep!andfill!useSparseVecOrMat; the two adjoint/transposeBroadcastStylerules become one.SparseVecOrMatis renamedSparseVectorOrMatrixCSCso it no longer shares a name with the top-level abstract alias. It stays concrete becauseSparse(B)only accepts those two types.ATLUbecomesUMFAdjOrTransLU,SPVMbecomesSparseVecOrMatStyle, andWrapperMatrixTypes(which isMatrixWrappersplusSubArray) becomesMatrixWrappersOrView. The single-useSuiteSparseStructis inlined.SparseOrStructuredMatrixis built onSparseMatrixCSCOrViewinstead of listingFixedSparseCSC,SparseMatrixCSCandSparseMatrixCSCViewexplicitly, somapover anyAbstractSparseMatrixCSCsubtype mixed with structured matrices takes the sparse path. The pre-existing all-sparsemap/map!disambiguation methods widen fromVararg{SparseMatrixCSC}toVararg{AbstractSparseMatrixCSC}to stay more specific than both competing methods.Reusing LinearAlgebra's names
BiTriSymwas redefined identically; it is now imported from LinearAlgebra.HigherOrderFns.StructuredMatrixshadowed the largerLinearAlgebra.StructuredMatrixin the same file. Its set (Diagonal,Bidiagonal,Tridiagonal,SymTridiagonal) is exactlyLinearAlgebra.BandedMatrix, which is used instead.RealHermSymComplexHermSSLis nowRealHermSymComplexHerm{Tr,<:SparseMatrixCSC{<:Any,Ti}}; dispatch is unchanged.Downstream-facing names
SparseMatrixCSCView,SparseColumnViewandSparseVectorVieware unchanged and used both internally and by downstream packages (LowRankOpt, IntervalMDP, Distances, and unregistered ones).SparseMatrixCSCUnion(Enzyme, SparseLinearAlgebra) andSparseVectorUnion(FillArrays, ArrayLayouts, Distances, FrankWolfe, SparseLinearAlgebra) are no longer used internally but are kept as aliases of the new names, with a note that they may be deprecated in a future release.AdjOrTransSparseVectorUnionhas no external user on GitHub or JuliaHub and is dropped.Behavior changes
SparseTriangularnow usesUpperOrLowerTriangular, soUnitUpperTriangular/UnitLowerTriangularwrappers of sparse matrices take the Gustavson product path like the non-unit ones (they are materialized viasparse(A)first; the temporary widens toIntindices when the added diagonal would not fit the parent's index type, and the result keeps the operands' index type). Tri × Tri products keep LinearAlgebra's result wrapper, includingUnitUpper * UnitUpperstaying unit.SparseVectorOrViewrather thanAbstractSparseVector, so customAbstractSparseVectortypes outside the kernel's supported set take the generic product. This also fixesUpperTriangular(S) * vfor such vectors, aMethodErroronmain.nnzandindtypeare extended to triangular wrappers of column-range views. Onmain,UpperTriangular(view(S, :, 1:n)) * Sand 14 similar combinations throwMethodError; they now return correct sparse results.A scratchpad probe of 1652 product, kron, dot, broadcast, conversion and solve combinations across sparse, view, wrapper and dense operands shows no result-type or correctness change for anything that worked on
main, plus the 15 formerMethodErrors now passing. Full suite and Aqua/ambiguity check pass locally, and CI is green on all platforms after rebasing ontomainpast #813, #817 and #819 (the all-sparsemap!disambiguation now carries #813's_unaliasargs).🤖 Generated with Claude Code
https://claude.ai/code/session_01MFPoSXGFJAGgJrHT98z2AY