make Adjoint/Transpose behave like typical constructors

[Sacha0]

This pull request is the hopefully last set of functional changes in the JuliaLang/LinearAlgebra.jl#57 series (after #24969, #25083, #25125, #25148, #25217, and #25364).

Specifically, this pull request's first commit replaces some instances of Adjoint/Transpose with adjoint/transpose (namely those missed in #25364 but caught by changes in the second commit), and its second commit makes Adjoint/Transpose behave like typical constructors (instead of e.g. satisfying Adjoint(Adjoint(A)) === A).

Unless I am forgetting something (please remind me if so!), after this pull request only news, docs, and cleanup remain for JuliaLang/LinearAlgebra.jl#57. Best!

[JeffBezanson]

makes Adjoint/Transpose behave like typical constructors (instead of e.g. satisfying Adjoint(Adjoint(A)) === A)

I'm curious --- why do we need this? Constructors are not required to be idempotent; that's one of the main differences with convert.

[Sacha0]

makes Adjoint/Transpose behave like typical constructors (instead of e.g. satisfying Adjoint(Adjoint(A)) === A)

I'm curious --- why do we need this? Constructors are not required to be idempotent

Thanks for reviewing! :) I am not certain I follow, so let's iterate:

If you meant "why do we need Adjoint(Adjoint(A)) === A and the equivalent for Transpose", then: This pull request makes Adjoint/Transpose no longer satisfy those identities, as adjoint/transpose now provide that behavior instead.

If you instead meant "why do we need the idempotent Adjoint(A::Adjoint) = A definition with which this pull request replaces the existing Adjoint(A::Adjoint) = A.parent definition, and likewise for Transpose", then: I do not think those definitions are necessary at this point; those definitions were suggested in #25217 (comment) and appeared to receive some support, so I rolled with them.

Thoughts? Thanks!

[JeffBezanson]

why do we need the idempotent Adjoint(A::Adjoint) = A definition with which this pull request replaces the existing Adjoint(A::Adjoint) = A.parent definition

This is the one I meant. It seems confusing to have an Adjoint(x) method that doesn't return some kind of adjoint of x.

[Sacha0]

This is the one I meant. It seems confusing to have an Adjoint(x) method that doesn't return some kind of adjoint of x.

I sympathize. I also appreciate what I believe was the motivation for this suggestion, i.e. that Adjoint(A::Adjoint) = A ensures Adjoint(A)::Adjoint. Proposal? :)

[JeffBezanson]

ensures Adjoint(A)::Adjoint

Eh, we've probably violated that before.

[mbauman]

I agree, this seems strange. I assumed by "proper constructor" in you meant that Adjoint(Adjoint(::Vector)) would return Adjoint{Adjoint{Vector}}. Would that satisfy your objectives here? That'd basically just mean removing all the specializations on these uppercase functions — they just always add another layer of wrapping.

The strange part is adjoint vectors, since they mean something special…

julia> f(x) = Adjoint{eltype(x), typeof(x)}(x)
f (generic function with 1 method)

julia> f([1,2,3])
1×3 Adjoint{Int64,Array{Int64,1}}:
 1  2  3

julia> f(f([1,2,3]))
3×1 Adjoint{Int64,Adjoint{Int64,Array{Int64,1}}}:
 1
 2
 3

julia> f(f(f([1,2,3])))
1×3 Adjoint{Int64,Adjoint{Int64,Adjoint{Int64,Array{Int64,1}}}}:
 1  2  3

julia> f(f(f([1,2,3]))) * [1,2,3]
1-element Array{Int64,1}:
 14

julia> f([1,2,3]) * [1,2,3]
14

It's funny, since we dispatch on Adjoint{T, AbstractVector} to implement those special cases, but Adjoint{T, AbstractVector} is itself an AbstractMatrix.

[Sacha0]

Any of the possible definitions are fine by me at this stage; I am happy to explore whichever definition gains broad favor :). (With adjoint/transpose now the operations of choice, I am not certain that anything hits the relevant methods in practice.)

[martinholters]

My naive expectations would be that Adjoint(x) is

• the adjoint of x
• of type Adjoint.

A case where these would hold but Adjoint(x).parent !== x might make sense would be Adjoint(Adjoint(Adjoint(x))) === Adjoint(x), but I'm not sure whether this is worth a special case.

[timholy]

I agree that Adjoint should always add an extra layer of wrapping. I understand that constructors are often idempotent; however, when the underlying mathematical operation is an involution, making the constructor idempotent would lead to long nights of self-doubt sitting under a ginko tree.

[StefanKarpinski]

I deeply agree with @timholy's point. Perhaps that provides good justification for the adjoint and transpose functions being separate from the Adjoint and Transpose constructors: the constructors always add a layer of wrapping around what they're given. The functions, on the other hand, may undo a layer of wrapping if that would have the desired effect. Then x' should lower to adjoint(x). I've lost track of what we decided to do about the .' syntax, but if it did mean transposition, we would want x.' to lower to transpose(x) in that case.

[garrison]

Eh, we've probably violated that before.

I can think of at least two other constructors that are not idempotent: Iterators.Reverse and ConjArray.

[JeffBezanson]

👍 to adding an extra layer of wrapping. I think I'll add a sentence or two to the manual with some of these examples.

[StefanKarpinski]

I can think of at least two other constructors that are not idempotent: Iterators.Reverse and ConjArray.

While that's true, they probably should return the requested type, which means it would make sense to have a reverse function that removes a layer of Reverse wrapping if present or adds one if not. Similarly for ConjArray. We don't force constructors to produce an object of the requested type, but in general I think it's a good idea to do so and we should keep cases where we do not to an absolute minimum and work towards a state where it is always the case.

[Sacha0]

Seems we have consensus! :) Being loathe to cause long nights of self-doubt sitting underneath a ginko tree, I will revise those methods post-haste.

[oscardssmith]

Would it make sense for reverse et al. to remove as many layers of wrapping as possible rather than just one? This seems likely to be a more useful behavior.

[Sacha0]

Thanks all! :)