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

spezialized conj of Transpose/Adjoint #33609

Merged
merged 4 commits into from
Oct 28, 2019

Conversation

simeonschaub
Copy link
Member

@simeonschaub simeonschaub commented Oct 19, 2019

I brought this up on Slack and it received quite positive feedback, so I put this together as a PR. The implementation is very minimal. Two things to maybe think about:

  • For AbstractArray{<:Real}, conj is generally a no-op, this breaks this assumption. We still differentiate between Adjoint and Transpose for real matrices though, so I think this is fine.
  • Do we want something similar for broadcasted conj? – Probably not worth it?

I am also not quite sure, how breaking one should consider this change. It will definitely break code, that relies on conj returning a copy here.

@StefanKarpinski
Copy link
Sponsor Member

Clever. This makes sense to me (although it took me a second).

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.

LGTM. I don't have own experience with the lazy vs copy issue of conj. At some point, @andreasnoack suggested to consider to introduce a lazy conj type, in the style of Adjoint and Transpose. This PR kind of introduces that for a very specific situation. Everywhere else, conj does create a copy, and people might rely on that, but I don't know how to check that. OTOH, many people who care seem to be in favor of it, so why not.

stdlib/LinearAlgebra/test/adjtrans.jl Outdated Show resolved Hide resolved
stdlib/LinearAlgebra/test/adjtrans.jl Outdated Show resolved Hide resolved
@simeonschaub
Copy link
Member Author

Thanks for the quick review, @dkarrasch! I've added another test case, that checks, whether we do the right thing for custom matrix types, that have a different implementation for adjoint. The lazy Conj idea is something that occurred to me as well. (You can kind of have that with adjoint(transpose(A)), but that's definitely not an efficient way.) Would make for some more symmetry, so I might open an issue at some point.

@kshyatt
Copy link
Contributor

kshyatt commented Oct 22, 2019

Win32 failure is a no-output timeout. Is this good to go?

@dkarrasch
Copy link
Member

dkarrasch commented Oct 22, 2019

I think so, from the code point of view. The question is whether someone with an actual working experience with complex linear algebra should step up and make a personal statement regarding the lazy vs. copy aspect?

@andreasnoack
Copy link
Member

I think this is fine. We generally try to avoid copies and ask people to rely on implicit copies.

@andreasnoack andreasnoack merged commit 54b212c into JuliaLang:master Oct 28, 2019
@simeonschaub simeonschaub deleted the conj_transpose_adjoint branch October 28, 2019 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants