Revert the SPQR adjoint changes that landed with #810 - #817
Merged
Merged
Conversation
PR #810 was meant to bump the version and julia compat to 1.14, but it was accidentally based on the branch of #804 and squash-merged with the "Solve with the adjoint of a sparse QR factorization" change included. This reverts the SPQR portion of a248d20 so that #804 can be reviewed and merged on its own; the Project.toml bump is kept. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RFzXKGsJuSogARQpDGipPr
This was referenced Sep 11, 2026
Member
Author
|
Superseded by #818, which reverts the whole commit. |
Member
Author
|
Reopened: this is the intended fix. It reverts only the SPQR adjoint change from a248d20 and keeps the Project.toml bump. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #817 +/- ##
=======================================
Coverage 92.54% 92.55%
=======================================
Files 12 12
Lines 8550 8497 -53
=======================================
- Hits 7913 7864 -49
+ Misses 637 633 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ViralBShah
added a commit
that referenced
this pull request
Sep 11, 2026
The `Union` aliases 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 of
`abstractsparse.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{...}` in `src`:
| | `main` | this PR |
|---|---|---|
| All `Union` aliases | 44 | 32 |
| Solver scalar / C-struct types (`ITypes`, `VTypes`, `UMFITypes`, ...)
| 10 | 9 |
| Dispatch aliases | 34 | 23 |
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 to `linalg.jl` (`DenseMatrixUnion`,
`DenseInputVector`, `MatrixWrappers`, `MatrixWrappersOrView`,
`QuasiSparseMatrix`, `QuasiStridedMatrix`, the two kron groups), 3 in
CHOLMOD (`StridedVecOrMatMaybeAdjOrTrans`, `SparseVectorOrMatrixCSC`,
`FactorComponentRHS`), `UMFAdjOrTransLU` in UMFPACK, and
`SparseVecOrMatStyle`, `SparseOrStructuredMatrix` and 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's
`RealHermSymComplexHermSSL`) no longer spell out a `Union` because they
are built on a LinearAlgebra alias.
### Consolidation
- `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`.
- "X or a view of X" is spelled `<X>OrView` / `<X>OrColumnSubset`. Alias
names use the `Sparse` family name and accept the abstract tier of their
base type; only the concrete CHOLMOD alias says otherwise in its name:
`SparseMatrixCSCUnion` becomes `SparseMatrixCSCOrView`,
`SparseMatrixCSCUnion2` becomes `SparseMatrixCSCOrColumnSubset`,
`SparseVectorUnion` becomes `SparseVectorOrView`, and
`AdjOrTransSparseVectorUnion` becomes `AdjOrTransSparseVectorOrView`.
The two-tier CSC split stays: the `getcolptr`-based kernels need a
unit-range column view, the `nzrange`-based ones accept any column
subset (#476).
- `SorF` is gone; `SparseMatrixCSC` and `FixedSparseCSC` each define
their accessors.
- The seven one-off kron intermediates are folded into
`_SparseKronGroup` / `_DenseKronGroup`.
- The inline spellings of the vec-or-mat union in the broadcast style
rules, `fkeep!` and `fill!` use `SparseVecOrMat`; the two
adjoint/transpose `BroadcastStyle` rules become one.
- CHOLMOD's concrete `SparseVecOrMat` is renamed
`SparseVectorOrMatrixCSC` so it no longer shares a name with the
top-level abstract alias. It stays concrete because `Sparse(B)` only
accepts those two types.
- The remaining cryptic or misleading local names are renamed: UMFPACK's
`ATLU` becomes `UMFAdjOrTransLU`, `SPVM` becomes `SparseVecOrMatStyle`,
and `WrapperMatrixTypes` (which is `MatrixWrappers` plus `SubArray`)
becomes `MatrixWrappersOrView`. The single-use `SuiteSparseStruct` is
inlined.
- `SparseOrStructuredMatrix` is built on `SparseMatrixCSCOrView` instead
of listing `FixedSparseCSC`, `SparseMatrixCSC` and `SparseMatrixCSCView`
explicitly, so `map` over any `AbstractSparseMatrixCSC` subtype mixed
with structured matrices takes the sparse path. The pre-existing
all-sparse `map`/`map!` disambiguation methods widen from
`Vararg{SparseMatrixCSC}` to `Vararg{AbstractSparseMatrixCSC}` to stay
more specific than both competing methods.
### Reusing LinearAlgebra's names
- `BiTriSym` was redefined identically; it is now imported from
LinearAlgebra.
- `HigherOrderFns.StructuredMatrix` shadowed the larger
`LinearAlgebra.StructuredMatrix` in the same file. Its set (`Diagonal`,
`Bidiagonal`, `Tridiagonal`, `SymTridiagonal`) is exactly
`LinearAlgebra.BandedMatrix`, which is used instead.
- CHOLMOD's `RealHermSymComplexHermSSL` is now
`RealHermSymComplexHerm{Tr,<:SparseMatrixCSC{<:Any,Ti}}`; dispatch is
unchanged.
### Downstream-facing names
`SparseMatrixCSCView`, `SparseColumnView` and `SparseVectorView` are
unchanged and used both internally and by downstream packages
(LowRankOpt, IntervalMDP, Distances, and unregistered ones).
`SparseMatrixCSCUnion` (Enzyme, SparseLinearAlgebra) and
`SparseVectorUnion` (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. `AdjOrTransSparseVectorUnion` has no external user on
GitHub or JuliaHub and is dropped.
### Behavior changes
- `SparseTriangular` now uses `UpperOrLowerTriangular`, so
`UnitUpperTriangular`/`UnitLowerTriangular` wrappers of sparse matrices
take the Gustavson product path like the non-unit ones (they are
materialized via `sparse(A)` first; the temporary widens to `Int`
indices 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, including `UnitUpper * UnitUpper`
staying unit.
- The sparse-vector fast path dispatches on `SparseVectorOrView` rather
than `AbstractSparseVector`, so custom `AbstractSparseVector` types
outside the kernel's supported set take the generic product. This also
fixes `UpperTriangular(S) * v` for such vectors, a `MethodError` on
`main`.
- `nnz` and `indtype` are extended to triangular wrappers of
column-range views. On `main`, `UpperTriangular(view(S, :, 1:n)) * S`
and 14 similar combinations throw `MethodError`; 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 former `MethodError`s now passing. Full suite and
Aqua/ambiguity check pass locally, and CI is green on all platforms
after rebasing onto `main` past #813, #817 and #819 (the all-sparse
`map!` disambiguation now carries #813's `_unaliasargs`).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01MFPoSXGFJAGgJrHT98z2AY
---------
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#810 was meant to only bump the version and julia compat to 1.14, but it was accidentally based on the branch of #804 and squash-merged with the "Solve with the adjoint of a sparse QR factorization" change included (see #810 (comment)). This reverts the
src/solvers/spqr.jlandtest/spqr.jlportion of a248d20 so that #804 can be reviewed and merged on its own. TheProject.tomlbump is kept.Both files are restored byte-for-byte to their state before a248d20, and
test/spqr.jlpasses locally on the 1.14-DEV build.🤖 Generated with Claude Code
https://claude.ai/code/session_01RFzXKGsJuSogARQpDGipPr