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

Remove Adjoint|Transpose(::UniformScaling) definition #27479

Merged
merged 1 commit into from
Jun 8, 2018

Conversation

tkf
Copy link
Member

@tkf tkf commented Jun 7, 2018

Reading #25461, it looks like that the consensus was that Adjoint and Transpose have to add an extra layer of indirection by wrapping the given object. However, Adjoint(::UniformScaling) and Transpose(::UniformScaling) "eagerly execute" adjoint and transpose:

transpose(J::UniformScaling) = J
Transpose(S::UniformScaling) = transpose(S)
adjoint(J::UniformScaling) = UniformScaling(conj(J.λ))
Adjoint(S::UniformScaling) = adjoint(S)

(FYI: those are the added in commit e80697d#diff-2053e27c93f0362bfb456f36c77bb232 during #24969.)

If Adjoint(::UniformScaling) and Transpose(::UniformScaling) are just unnoticed during #25461, I suggest simply removing those lines (hence this PR). When I test stdlib/LinearAlgebra locally, those definitions were not necessary.

Note that, even though Adjoint{T, UniformScaling{T}} can be constructed with this PR, I'm not sure if it is necessary to add a special handling. Since Adjoint <: AbstractMatrix but not UniformScaling <: AbstractMatrix, an object of type Adjoint{T, UniformScaling{T}} is not well-defined; for example, after this PR, typing Adjoint(I) in REPL shows error from Base.show as would happen with other meaningless combinations like Adjoint("str").

They always should add an extra wrapping, instead of eagerly do
adjoint/transpose.
See JuliaLang#25461
@StefanKarpinski
Copy link
Member

StefanKarpinski commented Jun 7, 2018

Makes sense to me. We've generally settled on Adjoint and Transpose always returning Adjoint and Transpose objects whereas adjoint and transpose can return something that is logically the adjoint or the transpose but not necessarily Adjoint or Transpose objects, e.g. by returning the argument in Hermitian or Symmetric cases or undoing a layer of Adjoint or Transpose wrapping.

@fredrikekre fredrikekre requested a review from Sacha0 June 7, 2018 20:05
@fredrikekre fredrikekre added the linear algebra Linear algebra label Jun 7, 2018
@Sacha0
Copy link
Member

Sacha0 commented Jun 8, 2018

Makes sense to me. We've generally settled on Adjoint and Transpose always returning Adjoint and Transpose objects whereas adjoint and transpose can return something that is logically the adjoint or the transpose but not necessarily Adjoint or Transpose objects, e.g. by returning the argument in Hermitian or Symmetric cases or undoing a layer of Adjoint or Transpose wrapping.

👍

Chances are these methods are vestigial. If not, I can't remember why :).

Copy link
Member

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tkf! :)

@tkf
Copy link
Member Author

tkf commented Jun 8, 2018

@Sacha0 Thanks for the review!

tkf added a commit to tkf/MatrixCombinators.jl that referenced this pull request Jun 8, 2018
@fredrikekre fredrikekre merged commit 0dbcc2a into JuliaLang:master Jun 8, 2018
haampie pushed a commit to haampie/julia that referenced this pull request Jun 9, 2018
They always should add an extra wrapping, instead of eagerly do
adjoint/transpose.
See JuliaLang#25461
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.

4 participants