RFC: Make matrix transpose default for `permutedims` #24839

andyferris · 2017-11-29T12:14:04Z

There has been a lot of discussion at JuliaLang/LinearAlgebra.jl#408 about the future of transpose (and adjoint, etc).

Basically, we would like a nicer way of doing a "shallow" transpose of an array of data, that has no linear algebra connotations. The current syntax would be permutedims(matrix, (2,1))

The approach here is to simplify this to permutedims(matrix) by making a default argument for perm = (2,1).

andyferris · 2017-11-29T12:20:38Z

The second commit adds permutedims(vector) to create a row-matrix (like the old transpose, avoiding RowVector altogether). Happy for feedback on this one (seems convenient, but slightly misnamed).

jebej · 2017-11-29T14:02:04Z

IIUC this won't work for AbstractArrays with more than 2 dimensions. A general default might be to shift the dimensions circularly (1,2,3) -> (3,1,2)

andyferris · 2017-11-29T20:27:55Z

That was on purpose - happy to discuss but I worried about making it too magical.

ararslan · 2017-11-29T21:02:37Z

base/multidimensional.jl

+"""
+    permutedims(v::AbstractVector)
+
+Reshapes vector `v` into a `1 × length(v)` row matrix.


Typically we stick to the imperative in docstrings, so this should be "reshape" instead of "reshapes."

Thanks - I always forget this!

ararslan · 2017-11-29T22:05:36Z

base/permuteddimsarray.jl

-`ndims(A)`. This is a generalization of transpose for multi-dimensional arrays. Transpose is
-equivalent to `permutedims(A, [2,1])`.
+`ndims(A)`. This is a generalization of matrix transposition for multi-dimensional arrays.
+The value of `perm` defaults to `(2,1)` for transposing the elements of a matrix.


"Transposing the elements of a matrix" makes it sound (at least to me) like it recursively calls permutedims on the elements. Perhaps "transposing the dimensions of a matrix"? Unless that isn't a standard way to say it.

Perhaps "flips the array cross its diagonal" or "mirrors the array across its diagonal"? Edit: That is, perhaps best to avoid "transpose" in this docstring?

Yes, avoiding the word "transpose" would be ideal.

Sacha0 · 2017-12-01T02:03:59Z

base/multidimensional.jl

@@ -1466,7 +1466,7 @@ end

 ## Permute array dims ##

-function permutedims(B::StridedArray, perm)
+function permutedims(B::StridedArray, perm = (2,1))


Perhaps better to provide a default (2, 1) perm only for two-dimensional StridedArrays? For example, perhaps add the short child

permutedims(B::StridedMatrix) = permutedims(B, (2, 1))

rather than modify this general method's signature?

I thought of that at first, but I note that you get a pretty coherent error message for 3D arrays as this is.

Sacha0 · 2017-12-01T02:05:24Z

base/multidimensional.jl

@@ -1487,7 +1489,7 @@ function checkdims_perm(P::AbstractArray{TP,N}, B::AbstractArray{TB,N}, perm) wh
 end

 for (V, PT, BT) in [((:N,), BitArray, BitArray), ((:T,:N), Array, StridedArray)]
-    @eval @generated function permutedims!(P::$PT{$(V...)}, B::$BT{$(V...)}, perm) where $(V...)
+    @eval @generated function permutedims!(P::$PT{$(V...)}, B::$BT{$(V...)}, perm = (2,1)) where $(V...)


Likewise here, perhaps better to provide a default (2, 1) permutation only for two-dimensional BitArrays?

Sacha0 · 2017-12-01T02:06:34Z

base/permuteddimsarray.jl

@@ -108,11 +108,18 @@ julia> permutedims(A, [3, 2, 1])
 6  8
 ```
 """
-function Base.permutedims(A::AbstractArray, perm)
+function Base.permutedims(A::AbstractArray, perm = (2,1))


And likewise here re. confining the default (2, 1) permutation to two-dimensional objects? :)

andyferris · 2017-12-02T13:25:24Z

OK I did another pass, do you think the docstrings are clearer now?

Sacha0

Looks great! :) (Potentially modulo the permutedims(v::AbstractVector) method, about which I lack an opinion, but I imagine others might feel strongly about.)

andyferris · 2017-12-03T04:59:33Z

OK I don't understand the documentation reference system. It's currently complaining that it can't find a reference to permutedims. There are docstrings, and it's listed in the "stdlib" reference.

Sacha0 · 2017-12-03T05:03:03Z

test/arrayops.jl

+
+    m = [1 2; 3 4]
+    @test permutedims(m) == [1 3; 2 4]
+


CI reports a trailing whitespace failure on this line (583)?

Sacha0 · 2017-12-03T05:05:17Z

base/permuteddimsarray.jl

@@ -77,11 +77,10 @@ end
 @inline genperm(I, perm::AbstractVector{Int}) = genperm(I, (perm...,))

 """
-    permutedims(A, perm)
+    permutedims(A::AbstractArray, perm])


Rogue ]? :)

Sacha0 · 2017-12-03T05:06:24Z

base/permuteddimsarray.jl

+Permute the dimensions of the matrix `m`, by flipping the elements across the diagonal of
+the matrix. Differs from [`transpose`](@ref) in that the operation is not recursive.
+"""
+Base.permutedims(A::AbstractMatrix) = permutedims(A, (2,1))


The Base. qualification should be unnecessary?

andyferris · 2017-12-09T05:19:01Z

This is working now.

andyferris · 2017-12-10T04:32:59Z

Alright, news done.

@Sacha0 How do you think this will work in combination with the changes you are making for transpose?

In particular, what is the best way of getting a left-eigenvector out of an eigen.vectors matrix? In this case (for Hermitian eigendecomposition) you want non-recursive vector transpose, but you do want a RowVector. Do you think permutedims(::AbstractVector) should be the way of creating the RowVector non-recursively (as in permutedims(eigen.vectors[i, :])), or is there a better trick?

The other way to go is ((eigen.vectors')[:, i])', which should be fast after you're done. And the view version of that should also work (if a little convoluted). Also, this seems equivalent to (inv(eigen.vectors)[:, i])' for the general (non-Hermitian) case, so maybe using ' is better than permutedims for this? (EDIT: I'm getting the feeling that I've overthought this... eig doesn't support block matrices anyway).

Does anyone else have an opinion if permutedims(::AbstractVector) should even exist?

* Also allow `permutedims(vector)` to make row matrix * Make clearer the relationships between `transpose`, `adjoint` and `permutedims` in the docstrings.

Sacha0 · 2017-12-10T17:58:01Z

How do you think...

Unfortunately beyond the scope of things I can think about for now 😄. Apologies and best!

andyferris · 2017-12-11T03:57:55Z

No probs, Sacha. I think I may have been overthinking it... I can't think of a proper linear-algebra use of non-recursive transpose other than that, so I think this should be fine.

andyferris · 2017-12-14T10:22:57Z

Given the lack of nay-saying, I'll go ahead and merge this now.

I think this will solve the "data transpose" problem neatly enough for v1.0. We can always improve later on, as opportunities present themselves.

algorithmx · 2018-07-26T16:08:21Z

HOW do I use type annotations/type asserts? Conceptually LinearAlgebra.Adjoint{Float64,Array{Float64,2}} and LinearAlgebra.Transpose{Float64,Array{Float64,2}} are still Array{Float64,2}; if they come as results of a function, say f(), what can I do to secure the type passed to a variable is conceptually Array{Float64,2}? I am not allowed to use
res::Array{Float64,2} = f()

ararslan · 2018-07-26T16:39:50Z

@algorithmx, it's best to ask questions on Discourse.

andyferris mentioned this pull request Nov 29, 2017

Taking matrix transposes seriously JuliaLang/LinearAlgebra.jl#408

Closed

andyferris force-pushed the ajf/permutedims branch from 5a525da to 9b491d1 Compare November 29, 2017 12:48

ararslan reviewed Nov 29, 2017

View reviewed changes

kshyatt added the arrays [a, r, r, a, y, s] label Nov 30, 2017

Sacha0 reviewed Dec 1, 2017

View reviewed changes

andyferris force-pushed the ajf/permutedims branch 2 times, most recently from 7a91959 to 7ed8ac8 Compare December 2, 2017 13:23

andyferris force-pushed the ajf/permutedims branch from 7ed8ac8 to c7e2888 Compare December 2, 2017 13:27

Sacha0 approved these changes Dec 2, 2017

View reviewed changes

Sacha0 reviewed Dec 3, 2017

View reviewed changes

andyferris force-pushed the ajf/permutedims branch 2 times, most recently from cbc4f80 to 646abce Compare December 3, 2017 05:20

andyferris mentioned this pull request Dec 3, 2017

Document recursive (c)transpose #24891

Closed

andyferris force-pushed the ajf/permutedims branch from 646abce to 0e376d2 Compare December 9, 2017 03:50

ararslan added the needs news A NEWS entry is required for this change label Dec 9, 2017

andyferris force-pushed the ajf/permutedims branch from 0e376d2 to 01c8d5c Compare December 10, 2017 04:17

andyferris removed the needs news A NEWS entry is required for this change label Dec 10, 2017

Make shallow matrix transpose default for permutedims

2ba2556

* Also allow `permutedims(vector)` to make row matrix * Make clearer the relationships between `transpose`, `adjoint` and `permutedims` in the docstrings.

andyferris force-pushed the ajf/permutedims branch from 01c8d5c to 2ba2556 Compare December 10, 2017 04:33

ararslan approved these changes Dec 11, 2017

View reviewed changes

andyferris merged commit a9bb7d3 into master Dec 14, 2017

martinholters deleted the ajf/permutedims branch December 14, 2017 10:39

stevengj mentioned this pull request Jun 21, 2018

single-argument permutedims(x) JuliaLang/Compat.jl#582

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Make matrix transpose default for `permutedims` #24839

RFC: Make matrix transpose default for `permutedims` #24839

andyferris commented Nov 29, 2017

andyferris commented Nov 29, 2017 •

edited

Loading

jebej commented Nov 29, 2017

andyferris commented Nov 29, 2017

ararslan Nov 29, 2017

andyferris Nov 29, 2017

ararslan Nov 29, 2017

Sacha0 Dec 1, 2017 •

edited

Loading

andyferris Dec 1, 2017

Sacha0 Dec 1, 2017

andyferris Dec 1, 2017

Sacha0 Dec 1, 2017

Sacha0 Dec 1, 2017

andyferris commented Dec 2, 2017

Sacha0 left a comment

andyferris commented Dec 3, 2017

Sacha0 Dec 3, 2017

Sacha0 Dec 3, 2017

Sacha0 Dec 3, 2017

andyferris commented Dec 9, 2017

andyferris commented Dec 10, 2017 •

edited

Loading

Sacha0 commented Dec 10, 2017

andyferris commented Dec 11, 2017

andyferris commented Dec 14, 2017

algorithmx commented Jul 26, 2018 •

edited

Loading

ararslan commented Jul 26, 2018

RFC: Make matrix transpose default for permutedims #24839

RFC: Make matrix transpose default for permutedims #24839

Conversation

andyferris commented Nov 29, 2017

andyferris commented Nov 29, 2017 • edited Loading

jebej commented Nov 29, 2017

andyferris commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyferris commented Dec 2, 2017

Sacha0 left a comment

Choose a reason for hiding this comment

andyferris commented Dec 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyferris commented Dec 9, 2017

andyferris commented Dec 10, 2017 • edited Loading

Sacha0 commented Dec 10, 2017

andyferris commented Dec 11, 2017

andyferris commented Dec 14, 2017

algorithmx commented Jul 26, 2018 • edited Loading

ararslan commented Jul 26, 2018

RFC: Make matrix transpose default for `permutedims` #24839

RFC: Make matrix transpose default for `permutedims` #24839

andyferris commented Nov 29, 2017 •

edited

Loading

Sacha0 Dec 1, 2017 •

edited

Loading

andyferris commented Dec 10, 2017 •

edited

Loading

algorithmx commented Jul 26, 2018 •

edited

Loading