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

Should dropdims accept dims as a regular argument? #28906

Open
yurivish opened this issue Aug 26, 2018 · 7 comments
Open

Should dropdims accept dims as a regular argument? #28906

yurivish opened this issue Aug 26, 2018 · 7 comments

Comments

@yurivish
Copy link
Contributor

yurivish commented Aug 26, 2018

Among the functions for reshaping arrays (reshape, permutedims, dropdims), dropdims is the only one that accepts dims as a keyword argument — the rest take dimensions as a positional argument.

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

Also, it feels a little redundant to have to say "dims" twice, once in the argument and once in the name of the function:

julia> dropdims([1 2 3], dims=(1,))
3-element Array{Int64,1}:
 1
 2
 3

julia> dropdims([1 2 3], (1,))
ERROR: MethodError: no method matching dropdims(::Array{Int64,2}, ::Tuple{Int64})

Note: It's possible to make this change backwards-compatibly by accepting both the keyword and positional argument.

@jebej
Copy link
Contributor

jebej commented Aug 27, 2018

Note: It's possible to make this change backwards-compatibly by accepting both the keyword and positional argument.

I agree, I think this should be the standard for a lot of these new keyword argument methods.

Also see #28303 .

@mbauman
Copy link
Member

mbauman commented Aug 27, 2018

Yeah, this was a choice that seemed much more obvious back when the function was still called squeeze. For many of the dims= keyword accepting functions there was an ambiguity with other optional positional arguments, and overall I very much like the change.

One thing specific to dropdims is its relation to reduction functions. We had wanted to allow squeeze(sum, A, dims=1) to perform the reduction and drop dimensions together in a slightly more terse API (#23500). While it's not ambiguous, does dropdims(f, A, 1) still make sense? Would you still expect to see a dims keyword argument there given that you're effectively doing dropdims(f(A, dims=1), 1)?

@yurivish
Copy link
Contributor Author

yurivish commented Aug 27, 2018

Yes, it does make much more sense in light of the name squeeze.

For reductions, maybe dropdims(A, 1, reduce=f)?

In that issue Jeff and Keno say

This was done for consistency. All functions that operate on dimensions now take those as a keyword argument.

but that does not seem to be the case e.g. for permutedims and reshape, though I know there is a large number of functions that do obey the convention.

julia> reshape([1 2 3 4], (4,))
4-element Array{Int64,1}:
 1
 2
 3
 4

julia> reshape([1 2 3 4], dims=(4,))
ERROR: function reshape does not accept keyword arguments

@JeffBezanson
Copy link
Member

dims is used when you're specifying which dimensions to operate on. reshape is totally different; there you're passing sizes, not selecting dimensions. permutedims is closer, since the contents of the tuple have the same meaning, but is still a bit different since the order is so significant.

@aminya
Copy link

aminya commented Sep 19, 2019

shouldn't dropdims be intelligent and drop the dimensions that are 1?

Say A=[1 2 3 4], becomes a 1*4 Array. dropdims should be able to drop 1st dimension without providing any keyword argument.

julia> A1
1×4 Array{Int64,2}:
 1  2  3  4

 dropdims(A1,dims=1)
4-element Array{Int64,1}:
 1
 2
 3
 4

proposed dropdims

 dropdims(A1)
4-element Array{Int64,1}:
 1
 2
 3
 4

@timholy
Copy link
Member

timholy commented Sep 19, 2019

Doing it automatically is a source of bugs (e.g., https://groups.google.com/d/msg/julia-users/wfEEvYIHVGs/caBXtuZulXgJ), and also not inferrable.

@aminya
Copy link

aminya commented Sep 20, 2019

Doing it automatically is a source of bugs (e.g., https://groups.google.com/d/msg/julia-users/wfEEvYIHVGs/caBXtuZulXgJ), and also not inferrable.

Although it is good to keep the code type stable, we shouldn't forget that Julia is a dynamic language and having such method will get the job done, especially if the array is constructed somewhere else and we need to squeeze the dimensions.

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

6 participants