Conversation
…at.jl consistently (tests still pass functionally; only the known two tiny allocation assertions remain).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Remove _select_storage_type function that was silently falling back to alternative storage types when promotion failed. This implicit behavior could cause severe performance degradation (e.g., GPU->CPU fallback). Now throws a helpful LinearOperatorException when storage types cannot be promoted to a concrete type, guiding users to ensure compatible storage types across operators. This aligns with Julia's philosophy of making performance characteristics explicit and prevents mysterious performance issues.
Contributor
There was a problem hiding this comment.
Pull request overview
Implements in-place update support for LinearOperator instances to address broadcast-style operator replacement (Issue #196), and improves error messaging when composing operators with incompatible storage types.
Changes:
- Add
copyto!/copy!for in-place updates between type-compatibleLinearOperators, plus amaterialize!hook to supportop .= new_op. - Treat
AbstractLinearOperatoras scalar-like in broadcasting viabroadcastable. - Improve error messages in
*and+whenstorage_typepromotion fails, and add tests for the new in-place update behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/abstract.jl |
Adds broadcasting hooks and in-place update APIs (copyto!, copy!, materialize!) for LinearOperator. |
src/operations.jl |
Enhances error messages for non-concrete promoted storage types in operator multiplication/addition. |
test/test_linop.jl |
Adds test coverage for copyto!, copy!, .=, and expected failure modes. |
Comments suppressed due to low confidence (1)
src/abstract.jl:257
copyto!assignsdest.Mv5/dest.Mtu5directly fromsrc, which makes the two operators share mutable workspace buffers. Those buffers are written to inmul!/allocate_vectors_args3!(seesrc/operations.jl), so usingdestandsrcindependently (especially from different tasks/threads) can lead to interference and data races. Consider allocating fresh buffers fordest(and copying contents if needed), or resetting them (e.g., empty +allocated5=false) sodestowns its workspace after the update; also preserve theMv5 === Mtu5aliasing relationship withindestwithout sharing withsrc.
Return the size of a linear operator as a tuple.
"""
size(op::AbstractLinearOperator) = (op.nrow, op.ncol)
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The recent struct changes added const modifiers to LinearOperator fields for performance. However, this makes copyto! impossible since const fields cannot be modified after construction. This commit removes const from the struct fields to enable the in-place update feature requested in issue JuliaSmoothOptimizers#196. While this may have minor performance implications, it is necessary for the .= syntax to work. Also updated copyto! implementation to match current struct fields (Mv/Mtu instead of Mv5/Mtu5/args5/use_prod5/allocated5).
After struct changes, operations.jl was calling the old CompositeLinearOperator
constructor with args5 parameter which no longer exists. Updated to use the
simplified LinearOperator{T,S} constructor.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #413 +/- ##
==========================================
- Coverage 95.00% 94.98% -0.02%
==========================================
Files 17 23 +6
Lines 1100 1276 +176
==========================================
+ Hits 1045 1212 +167
- Misses 55 64 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
@amontoison @dpo @tmigot All checks pass. |
This branch has not been deployed
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
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.
Fixes #196 by implementing safe in-place update methods for
LinearOperatorobjects.This PR adds three methods:
copyto!(dest, src)- Explicit in-place copy for compatible operatorscopy!(dest, src)- Alias forcopyto!following Base conventionsop1 .= op2- Convenient syntax viamaterialize!hookThe implementation:
broadcastable(op) = Ref(op))T,S,I,F,Ft,Fctparameters)prod!,tprod!,ctprod!)@tmigot As my previous pr is from the main branch, I plan to get that merged before this pr.