-
-
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
Adjoint for diagonal should be lazy #48443
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.
SGTM. Does that affect multiplication/solving performance somehow?
The other option would be, of course, to make |
Good to merge? |
Not yet, I'll check the performance implication |
Performance for some simple operations seems almost unaffected. If anything, solve seems slightly improved: julia> D = Diagonal([1:1000;]*im)';
julia> v = rand(ComplexF64, 1000);
julia> @btime $D * $D;
1.611 μs (1 allocation: 15.75 KiB)
julia> @btime $D * $v;
1.672 μs (1 allocation: 15.75 KiB)
julia> @btime $D \ $v;
31.136 μs (1 allocation: 15.75 KiB)
julia> @btime $D \ $D;
23.815 μs (1 allocation: 15.75 KiB)
julia> @btime $D \ $(Matrix(D));
22.762 ms (2 allocations: 15.26 MiB) This PR julia> @btime $D * $D;
1.610 μs (1 allocation: 15.75 KiB)
julia> @btime $D * $v;
1.766 μs (1 allocation: 15.75 KiB)
julia> @btime $D \ $v;
25.867 μs (1 allocation: 15.75 KiB)
julia> @btime $D \ $D;
7.162 μs (1 allocation: 15.75 KiB)
julia> @btime $D \ $(Matrix(D));
22.506 ms (2 allocations: 15.26 MiB) |
For the double adjoint, couldn't you add a method for adjoint(D::Diagonal{<:Real}) = transpose(D) ? Then there should remain a smaller niche for this "ugly" behavior. |
Updated, now julia> D = Diagonal([1,2,3]*im)
3×3 Diagonal{Complex{Int64}, Vector{Complex{Int64}}}:
0+1im ⋅ ⋅
⋅ 0+2im ⋅
⋅ ⋅ 0+3im
julia> (D')'
3×3 Diagonal{Complex{Int64}, Vector{Complex{Int64}}}:
0+1im ⋅ ⋅
⋅ 0+2im ⋅
⋅ ⋅ 0+3im The real case already does the right thing, so no special-casing is necessary. |
Currently,
adjoint
for aDiagonal
materializes the diagonal vector. This goes against the documentation ofadjoint
as a lazy wrapper.This PR makes the result share memory with the original
Diagonal
.The implementation isn't perfect, as
and, ideally,
(D')'
would just returnD
. However, at least this makes theadjoint
operation lazy and allocation-free.