-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
transition linalg to Adjoint/Transpose externally #25083
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Sidenote: typo in the comment operationt
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Thanks! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about Adjoint(map(Adjoint∘f∘Adjoint, parent.(avs)...))
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(so you don't have to create a temporary)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that looks right.
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 JuliaLang/LinearAlgebra.jl#57. 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 JuliaLang/LinearAlgebra.jl#492 will resolve this issue as a side effect. I will have a look later today. Thanks! :) |
#24969 transitioned linalg to
Adjoint
/Transpose
internally; this pull request transitions linalg toAdjoint
/Transpose
externally, completing item seven in #24969's OP and constituting the next major step towards JuliaLang/LinearAlgebra.jl#57.In greater detail, this pull request contains commits that : (1) give
Adjoint
/Transpose
all ofConjRowVector
/RowVector
's behaviors, making the former a drop-in replacement for the latter; (2) shieldConjRowVector
/RowVector
from 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
/Transpose
and clean up the fallout. All intermediate commits pass the linalg, sparse, and stdlib test suites locally.Ref. JuliaLang/LinearAlgebra.jl#57. Best!