transition linalg to Adjoint/Transpose externally #25083
Conversation
There was a problem hiding this comment.
This was such a heroic effort, can't thank you enough for tackling it @Sacha0!
I haven't had the time to follow this carefully, but I noticed one concern. Upon (very) superficial reading the rest of this seems fine and definitely should be part of 1.0.
| # instead of simply mapping the caller's operation over the set of unwrapped vectors, | ||
| # we map Adjoint/Transpose composed with the caller's operationt over the set of unwrapped | ||
| # vectors. then re-wrapping the result vector yields a wrapped vector with the correct entries. | ||
| map(f, avs::AdjointAbsVec...) = Adjoint(map(Adjoint∘f, vectorfyall(avs...)...)) |
There was a problem hiding this comment.
I don't understand why getindex doesn't just solve this for you. Are you trying to return an Adjoint-wrapped object? If so, why?
There was a problem hiding this comment.
Indeed, returning an Adjoint/Transpose-wrapped vector from higher order operations over Adjoint/Transpose-wrapped vectors is these methods' purpose :).
Why? The superficial reason is preservation of ConjRowVector/RowVector's behavior in Adjoint/Transpose-wrapped vectors. The reason for ConjRowVector/RowVector's behavior: Without re-wrapping, the result of these operations is a single-row Matrix, which has different semantics from ConjRowVector/RowVector. Consequently, for example, foo.(anadjointvector) * avector would produce a one-by-one matrix rather than a scalar.
In other words, re-wrapping preserves 4774 semantics through higher order operations insofar as possible.
Does that clarify? Thoughts? :)
There was a problem hiding this comment.
(Sidenote: typo in the comment operationt)
There was a problem hiding this comment.
What about Adjoint(map(Adjoint∘f∘Adjoint, parent.(avs)...))?
There was a problem hiding this comment.
(so you don't have to create a temporary)
There was a problem hiding this comment.
Avoiding the temporaries would be lovely! Deciphering the above expression is more than I have mental bandwidth for right at the moment though 😄. Perhaps a post-freeze optimization? :)
There was a problem hiding this comment.
@timholy, revisiting this rewrite briefly, given Adjoint is unary I imagine you instead meant e.g. Adjoint(map((xs...) -> Adjoint(f(Adjoint.(xs)...)), parent.(avs)...))? Thanks again! :)
Sacha0
changed the title
|
Much thanks for the kind words A tangential observation: The lazification of adjoint and transpose creates substantial future optimization opportunity. A simple and obvious example, efficient |
…ose(...), .', ', and A[ct]_(mul|ldiv|rdiv)_B[ct][!].
… to linalg/adjtrans.jl.
…associated soon-to-fail tests.
…ition associated tests to Adjoint/Transpose.
… to Adjoint/Transpose.
…viors to Adjoint/Transpose, and test.
…ncatenation behaviors.
…ve behaviors to Adjoint/Transpose.
|
Rebased and settled a couple todos. Assuming CI remains happy, I plan to merge this pull request tomorrow morning and carry on with downstream steps towards #5332. Thanks all! :) |
|
The AV i686 failure is unrelated, the FreeBSD logs are not accessible (but FreeBSD passed formerly IIRC), and (given CircleCI did not run over night) continuing to wait for CircleCI does not seem worthwhile. Thanks all! :) |
|
I haven't bisected, but most probably this PR or one of its cousins is responsible for julia> v = SparseVector([1.0])'
1×1 Adjoint{Float64,SparseVector{Float64,Int64}}:
1.0
julia> v - v
1×1 Adjoint{Any,SparseVector{Any,Int64}}:
Error showing value of type Adjoint{Any,SparseVector{Any,Int64}}:
ERROR: MethodError: no method matching zero(::Type{Any})Note that the error by itself is only a consequence of the element type becoming @Sacha0 do you have anything in the pipeline that will fix this, or should I open a dedicated issue? |
|
This seems to fix it: --- a/base/linalg/adjtrans.jl
+++ b/base/linalg/adjtrans.jl
@@ -158,8 +158,8 @@ map(f, tvs::TransposeAbsVec...) = Transpose(map(Transpose∘f, vectorfyall(tvs..
# broadcast over collections of Adjoint/Transpose-wrapped vectors and numbers
# similar explanation for these definitions as for map above
-broadcast(f, avs::Union{Number,AdjointAbsVec}...) = Adjoint(broadcast(Adjoint∘f, vectorfyall(avs...)...))
-broadcast(f, tvs::Union{Number,TransposeAbsVec}...) = Transpose(broadcast(Transpose∘f, vectorfyall(tvs...) ...))
+broadcast(f, avs::Union{Number,AdjointAbsVec}...) = Adjoint(broadcast((x->Adjoint(x))∘f, vectorfyall(avs...)...))
+broadcast(f, tvs::Union{Number,TransposeAbsVec}...) = Transpose(broadcast((x->Transpose(x))∘f, vectorfyall(tvs...) ...))I don't have the time to prepare a PR right now, so if anyone would be so kind to pick this up... |
|
Great catch @martinholters! Perhaps #25134 will resolve this issue as a side effect. I will have a look later today. Thanks! :) |
#24969 transitioned linalg to
Adjoint/Transposeinternally; this pull request transitions linalg toAdjoint/Transposeexternally, completing item seven in #24969's OP and constituting the next major step towards #5332.In greater detail, this pull request contains commits that : (1) give
Adjoint/Transposeall ofConjRowVector/RowVector's behaviors, making the former a drop-in replacement for the latter; (2) shieldConjRowVector/RowVectorfrom downstream changes toadjoint(...),transpose(...),.',', andA[ct]_(mul|ldiv|rdiv)_B[ct]; and (3) subsequently transition all forms of construction ofConjRowVector/RowVector(apart from their constructors) toAdjoint/Transposeand clean up the fallout. All intermediate commits pass the linalg, sparse, and stdlib test suites locally.Ref. #5332. Best!