Improve inferrability#383
Conversation
|
Bump |
|
Any chance someone can take a look at this? Or at least approve the workflow so that the tests can run? (EDIT: now I'm a bit confused about whether this repo actually uses the CI workflow.) |
|
Bump |
|
@timholy I just discovered your PR. |
|
@timholy Can you rebase your branch? |
There is no way to make the `S` keyword argument of `LinearOperator{T}`
inferrable.
This moves `S` to the second position, so anyone who wishes to write
inferrable code can use `LinearOperator{T, Vector{T}}(args...)`.
More inferrability & performance work
timholy
force-pushed
the
teh/inferrability
branch
from
2ae27cf to
b2fe73e
Compare
|
Sorry to be "out" for a bit. Rebased. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the LinearOperator type to improve type inferrability by moving the storage type parameter S from a keyword argument to the second type parameter position. This allows users to write LinearOperator{T, Vector{T}}(args...) for fully inferrable code, while maintaining backward compatibility through wrapper functions.
Changes:
- Reordered type parameters in
LinearOperatorstruct from{T, I, F, Ft, Fct, S}to{T, S, I, F, Ft, Fct} - Updated all constructor call sites to use positional type parameters instead of keyword arguments
- Added backward compatibility wrappers for the old keyword argument syntax
- Changed
CompositeLinearOperatorto accept S as a positional Type parameter
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/abstract.jl | Core type definition changes - reordered type parameters and added backward compatibility wrappers for both LinearOperator and CompositeLinearOperator |
| src/special-operators.jl | Updated all operator constructors (opEye, opOnes, opZeros, opDiagonal, opRestriction) to use new positional S parameter; includes unrelated view() optimization |
| src/operations.jl | Updated CompositeLinearOperator calls in operator multiplication and addition |
| src/linalg.jl | Updated constructors for opInverse, opCholesky, opHouseholder, and opHermitian to use new syntax |
| src/kron.jl | Updated kron operator constructor to use new syntax |
| src/constructors.jl | Updated LinearOperator constructor wrapper with explicit ::Type annotation for S parameter |
| src/cat.jl | Updated hcat and vcat to pass S as positional argument to CompositeLinearOperator |
| ext/LinearOperatorsLDLFactorizationsExt.jl | Updated opLDL constructors in extension to use new syntax |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| function mulRestrict!(res, I, v, α, β) | ||
| res .= v[I] | ||
| res .= view(v, I) |
There was a problem hiding this comment.
This change from v[I] to view(v, I) appears to be unrelated to the type parameter reordering that is the main focus of this PR. While using view may be a performance improvement to avoid allocating a copy, this change should either be removed from this PR (to keep the PR focused on type parameter reordering) or explicitly mentioned in the PR description. If this change is intentional, it should be verified that the behavioral semantics are preserved - view creates a reference to the original array rather than a copy, which could have implications if the result is modified.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #383 +/- ##
==========================================
+ Coverage 95.00% 95.44% +0.44%
==========================================
Files 17 21 +4
Lines 1100 1186 +86
==========================================
+ Hits 1045 1132 +87
+ Misses 55 54 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
amontoison
merged commit 06be7db
into
JuliaSmoothOptimizers:main
|
Thanks @timholy ! |
Add documentation for JuliaSmoothOptimizers#383 Also cleans up an unnecessarily large number of `LinearOperator(M)` methods, preferring to consolidate them by supplying `M`-specific defaults for the `symmetric` and `hermitian` keyword arguments.
There is no way to make the
Skeyword argument ofLinearOperator{T}inferrable.
This moves
Sto the second position, so anyone who wishes to writeinferrable code can use
LinearOperator{T, Vector{T}}(args...).This appears to be non-breaking. A search of the JuliaSmoothOptimizers org for code with
LinearOperator{reveals only one source-code usage with more than one parameter: https://github.com/JuliaSmoothOptimizers/RipQP.jl/blob/6d3c074e72b4e94846a165bed5cc2b78bfdb3623/src/iterations/preconditioners/LDL.jl#L48While the names there will be misleading, there appears to be no functional consequence.