optimize and fix map/broadcast over Adjoint/Transpose vectors, take 2 #25238
Conversation
…and fix inference. (Take 2.)
Sacha0
added
bugfix
|
The CI straight flush seems progressively more common! 🎉 |
|
Absent objections or requests for time, I plan to merge these changes midday tomorrow PT at the earliest. Thanks all! |
| # | ||
| # note that the caller's operation f operates in the domain of the wrapped vectors' entries. | ||
| # hence the Adjoint->f->Adjoint shenanigans applied to the parent vectors' entries. | ||
| map(f, avs::AdjointAbsVec...) = Adjoint(map((xs...) -> Adjoint(f(Adjoint.(xs)...)), parent.(avs)...)) |
There was a problem hiding this comment.
given recent discussions shouldn't this be adjoint.(xs)...?
There was a problem hiding this comment.
Absolutely, these calls should eventually become adjoint/transpose given recent discussion :). That said, with master as it stands, Adjoint/Transpose are correct, whereas I am not entirely confident that adjoint/transpose are correct and will remain correct through the few remaining pull requests in the #5332 series. After those remaining pull requests in the #5332 series, changing these calls to adjoint/transpose should become correct with certainty. Thoughts? :) Thanks!
There was a problem hiding this comment.
As long as this is an intermediate state, I'm fine with it.
There was a problem hiding this comment.
With a bit more thought, writing these methods in Adjoint/Transpose may also be ultimately best: The relevant objects are <:AbstractVector, and as such Adjoint/Transpose-wrapping is correct, precise, and unambiguous (whereas adjoint/transpose will be more nebulous). Best!
|
Absent further commentary I plan to merge this change tomorrow morning PT or later. Best! |
|
Thanks all! :) |
Second take on #25219: This pull request implements the optimization @timholy suggested in #25083 (comment) (hence closes JuliaLang/LinearAlgebra.jl#492) and fixes the inference issue @martinholters caught in #25083 (comment). This version addresses the linalg/dense test failures discovered in #25219 as well, and adds a couple direct tests for that issue to linalg/adjtrans. Thanks and best!