Skip to content
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

Merged
merged 2 commits into from
Apr 22, 2023
Merged

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Jan 28, 2023

Currently, adjoint for a Diagonal materializes the diagonal vector. This goes against the documentation of adjoint as a lazy wrapper.

This PR makes the result share memory with the original Diagonal.

julia> D = Diagonal(fill(2im, 2))
2×2 Diagonal{Complex{Int64}, Vector{Complex{Int64}}}:
 0+2im      
       0+2im

julia> D'
2×2 Diagonal{Complex{Int64}, Base.ReshapedArray{Complex{Int64}, 1, Adjoint{Complex{Int64}, Vector{Complex{Int64}}}, Tuple{}}}:
 0-2im      
       0-2im

The implementation isn't perfect, as

julia> D = Diagonal(fill(2im, 2))
2×2 Diagonal{Complex{Int64}, Vector{Complex{Int64}}}:
 0+2im      
       0+2im

julia> D'
2×2 Diagonal{Complex{Int64}, Base.ReshapedArray{Complex{Int64}, 1, Adjoint{Complex{Int64}, Vector{Complex{Int64}}}, Tuple{}}}:
 0-2im      
       0-2im

julia> (D')'
2×2 Diagonal{Complex{Int64}, Base.ReshapedArray{Complex{Int64}, 1, Adjoint{Complex{Int64}, Base.ReshapedArray{Complex{Int64}, 1, Adjoint{Complex{Int64}, Vector{Complex{Int64}}}, Tuple{}}}, Tuple{}}}:
 0+2im      
       0+2im

and, ideally, (D')' would just return D. However, at least this makes the adjoint operation lazy and allocation-free.

@jishnub jishnub added the linear algebra Linear algebra label Jan 28, 2023
@jishnub jishnub requested a review from dkarrasch January 31, 2023 05:37
Copy link
Member

@dkarrasch dkarrasch left a 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?

@dkarrasch
Copy link
Member

The other option would be, of course, to make D' = Adjoint(D), but then we would need to add a couple of multiplication and solve methods (which I removed at some point because one would not get an Adjoint via adjoint, irrespective of the eltype).

@ViralBShah
Copy link
Member

Good to merge?

@jishnub
Copy link
Contributor Author

jishnub commented Feb 13, 2023

Not yet, I'll check the performance implication

@jishnub
Copy link
Contributor Author

jishnub commented Apr 18, 2023

Performance for some simple operations seems almost unaffected. If anything, solve seems slightly improved:
On master

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)

@dkarrasch
Copy link
Member

For the double adjoint, couldn't you add a method for ::Diagonal{<:Any,<:ReshapedArray{<:Adjoint}} and make it do the ideal thing? And mitigate the issue further via

adjoint(D::Diagonal{<:Real}) = transpose(D)

? Then there should remain a smaller niche for this "ugly" behavior.

@jishnub
Copy link
Contributor Author

jishnub commented Apr 19, 2023

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.

@dkarrasch dkarrasch added the merge me PR is reviewed. Merge when all tests are passing label Apr 19, 2023
@dkarrasch dkarrasch removed the merge me PR is reviewed. Merge when all tests are passing label Apr 22, 2023
@dkarrasch dkarrasch merged commit 4044096 into JuliaLang:master Apr 22, 2023
@jishnub jishnub deleted the diagadj branch April 22, 2023 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants