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

Transpose deprecation "permutedims tip" doesn't work. #18320

Closed
Ismael-VC opened this issue Sep 1, 2016 · 8 comments
Closed

Transpose deprecation "permutedims tip" doesn't work. #18320

Ismael-VC opened this issue Sep 1, 2016 · 8 comments

Comments

@Ismael-VC
Copy link
Contributor

Reference: #17374

julia> x = ["x" for x = 1:5];

julia> x'
WARNING: the no-op `transpose` fallback is deprecated, and no more specific `transpose` method for String exists. Consider `permutedims(x, [2, 1])` or writing a specific `transpose(x::String)` method if appropriate.
 at depwarn(::String, ::Symbol) at deprecated.jl:64
 at transpose(::String) at deprecated.jl:771
 at ctranspose at operators.jl:310 [inlined]
 at (::Base.##211#212)(::Tuple{Int64,String}) at <missing>:0
 at copy!(::Array{String,2}, ::Base.Generator{Base.Prod2{Base.OneTo{Int64},Array{String,1}},Base.##211#212}) at abstractarray.jl:394
 at ctranspose(::Array{String,1}) at arraymath.jl:417
 at eval(::Module, ::Any) at boot.jl:234
 at eval_user_input(::Any, ::Base.REPL.REPLBackend) at REPL.jl:64
 at macro expansion at REPL.jl:95 [inlined]
 at (::Base.REPL.##3#4{Base.REPL.REPLBackend})() at event.jl:68
while loading no file, in expression starting on line 0
1×5 Array{String,2}:
 "x"  "x"  "x"  "x"  "x"

julia> permutedims(x, [2, 1])
------------------------------------------------------------------------------------------
ArgumentError                                           Stacktrace (most recent call last)
[#1] — permutedims(::Array{String,1}, ::Array{Int64,1})
       ⌙ at multidimensional.jl:825

ArgumentError: no valid permutation of dimensions

I'd love to read the whole discussion of why this behavior changed, but I can't, I guess it's because some kind of mathematical purity consideration, I just wanted to do x' and move on, I had to read the source code of the referenced issue, to understand both what it meant by no-op and how to fix this issue:

julia> Base.transpose(s::AbstractString) = s

julia> x'
1×5 Array{String,2}:
 "x"  "x"  "x"  "x"  "x"

I don't get why it's worth removing these definitions, and force people to implement them for their own types too. Even if permutedims is the answer, I just wanted to use x', as soon as I saw this I was like: ok ...I don't get this, but anyway I can (or so I thought) do this to make it work with all vectors again, it's just one line:

# this is what someone who whants to do `x'` could try:
julia> Base.transpose(xs::Vector) = permutedims(xs, [2, 1]) 

julia> type Foo end

julia> [Foo() for i in 1:3]'     # lol ...yeah right! -_- 

But of course not, that was too naive.

I'm sure you guys know what you are doing and I trust you, but if we deprecate something, the warning should include the solution by today standards, not a tip, or consider and it have to be tested, I don't want to have to read 3+ year long discussions to figure out this or have to look at the Base code to understand what this means.

If I do:

julia> Base.transpose(x) = x
WARNING: Method definition transpose(Any) in module Base at deprecated.jl:771 overwritten in module Main at REPL[1]:1.

I'll get a warning because now I'm redefining the method (but it works), do I really have to define transpose(x::T) for any new type if I may want to transpose a vector of those separately?

I also think it is a good idea to explain concisely why a feature is added, deprecated or removed in NEWS.md:

The no-op transpose fallback has been deprecated. Consider introducing suitable transpose methods or calling permutedims(x, [2,1]) (#13171, #17075, #17374).

This tells me nothing and suggests (forces) me to read 3 threads.

@Ismael-VC
Copy link
Contributor Author

This works:

julia> a = [:a, :b, :c]
3-element Array{Symbol,1}:
 :a
 :b
 :c

julia> reshape(a, 1, length(a))
1×3 Array{Symbol,2}:
 :a  :b  :c

From: #13171 (comment)

Should I change the deprecation warning from:

  • WARNING: the no-op transpose fallback is deprecated, and no more specific transpose method for Foo exists. Consider permutedims(x, [2, 1]) or writing a specific transpose(x::Foo) method if appropriate.

To:

  • WARNING: the no-op transpose fallback is deprecated, and no more specific transpose method for Foo exists. Use permutedims(x, [2, 1]) for matrices and reshape(x, 1, length(x)) for vectors or writing a specific transpose(x::Foo) method if appropriate.

@andreasnoack
Copy link
Member

andreasnoack commented Sep 1, 2016

@Ismael-VC Yes. It would be great if you could update the deprecation warning and the README.mdNEWS.jl.

@Ismael-VC
Copy link
Contributor Author

Ismael-VC commented Sep 1, 2016

@andreasnoack you ment NEWS.md instead of README.md? Could you please help me describe in one sentence why this change was implemented, ie in NEWS.md:

The no-op transpose fallback has been deprecated. Use permutedims(x, [2, 1]) for matrices and reshape(x, 1, length(x)) for vectors or writing a specific transpose(x::Foo) method if appropriate. (#13171, #17075, #17374). This change eases the implementation of ... blah ... blah

Just to be concise and get a feeling of what we lost and what we gained. I can't write that sentence becuase, even after reading the issues I don't get what this is all about, thank you in advance.

  • This change eases the implementation of higher multidimensional (ie. tensor) algorithms...

In one short sentence would that describe it?

@ahwillia
Copy link
Contributor

ahwillia commented Sep 1, 2016

This tells me nothing and suggests (forces) me to read 3 threads.

Thank you. I am also quite frustrated about this.

I have a few comments on your new suggestion (which I like!):

WARNING: the no-op transpose fallback

What is this? The average user will have no idea what this refers to

is deprecated, and no more specific transpose method for Foo exists.

  1. Should this be Array{Foo}? It seems off that I'm supposed to implement transpose(x::String) or transpose(x::String)...
  2. "no more specific transpose method" -- am I right to read this as "numeric arrays can still be transposed like this but non-numeric arrays can't be?" If so I think this should be mentioned.

Maybe something like:

WARNING: transposing a non-numeric array would result in a no-op which is deprecated. Intead, use permutedims(x, [2, 1]) for matrices and reshape(x, 1, length(x)) for vectors or write a specific transpose(x::Array{Foo}) method if appropriate.

I still don't get the no-op part. Is there ever a context where transpose does work on non-numeric vectors/matrices now?

@KristofferC
Copy link
Sponsor Member

KristofferC commented Sep 1, 2016

transpose is recursive so it is trying to transpose the elements as well. If you have an element type that does not support transpose then you will get the warning. Previously transpose defaulted to nothing (no-op) so if you had a vector of Symbols then the fact that transpose ran on the symbols as well was not visible.

julia> a = Matrix{Matrix{Float64}}(2,2)
2×2 Array{Array{Float64,2},2}:
 #undef  #undef
 #undef  #undef

julia> for i in eachindex(a); a[i] = rand(2,2); end

julia> a[1]
2×2 Array{Float64,2}:
 0.903635  0.532792
 0.653868  0.454383

julia> a'[1] # Transposed from above even though it is an element of `a`
2×2 Array{Float64,2}:
 0.903635  0.653868
 0.532792  0.454383

@ahwillia
Copy link
Contributor

ahwillia commented Sep 1, 2016

Got it. Thanks for helping me understand the internals a bit more. I suppose this is nice for transposing a block matrix.

I think the no-op part of the depwarn should be removed and it should just say something like: you can't transpose anything that is non-numeric unless you define a no-op transpose function for the data type: transpose(f::Foo) = f

I still don't quite understand why this deprecation was necessary in the first place ಠ_ಠ

@andreasnoack
Copy link
Member

@Ismael-VC I don't think NEWS.md is the right place to explain why we have made the changes. That is why we link to the issues. The NEWS.md file should be as a short and precise description of the changes to make it easier to update your code to a new version of Julia.

@simonster
Copy link
Member

For whoever is going to fix this warning, I think we should be encouraging the use of permutedims(x, (2, 1)) instead of permutedims(x, [2, 1]).

@tkelman tkelman closed this as completed in f1544bc Nov 5, 2016
tkelman pushed a commit that referenced this issue Feb 22, 2017
* close #18320

* Update NEWS.md

* Update deprecated.jl

* Update deprecated.jl

(cherry picked from commit f1544bc)
fcard pushed a commit to fcard/julia that referenced this issue Feb 28, 2017
* close JuliaLang#18320

* Update NEWS.md

* Update deprecated.jl

* Update deprecated.jl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants