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

zeros function mutates arguments #19265

Closed
jw3126 opened this issue Nov 8, 2016 · 10 comments
Closed

zeros function mutates arguments #19265

jw3126 opened this issue Nov 8, 2016 · 10 comments
Labels
kind:bug Indicates an unexpected problem or unintended behavior

Comments

@jw3126
Copy link
Contributor

jw3126 commented Nov 8, 2016

I was surprised by zeros sometimes mutating an array, I guess this is not intended:

x = [1.];
zeros(Float64, x);
x
1-element Array{Float64,1}:
 0.0

while

x = [1.];
zeros(x);
x
1-element Array{Float64,1}:
 1.0
Julia Version 0.6.0-dev.1202
Commit 42a22b6 (2016-11-06 22:14 UTC)
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
  WORD_SIZE: 64
  BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Haswell)
  LAPACK: libopenblas64_
  LIBM: libopenlibm
  LLVM: libLLVM-3.7.1 (ORCJIT, haswell)
@nalimilan
Copy link
Member

This is an unintended consequence of the fact that Array{T}(dims...) returns x when dims = (x,) and x is an Array{T}. That's kind of annoying. We could limit dims to a tuple of integers (i.e. ::Dims), since passing a type and an array it not really documented, and doesn't make a lot of sense since the type is redundant with the array's element type.

@jw3126
Copy link
Contributor Author

jw3126 commented Nov 8, 2016

I see, adding dims::Dims sounds good. Would it make sense to officially add a
zeros(::Type, ::AbstractArray) method? I used this quite frequently and did not realize it is unsupported until now.

@martinholters
Copy link
Member

Allowing zeros(Float64, x) as a shorthand for zeros(Float64, size(x)) does not seem important, but it should error instead of having the current surprising behavior. Should we do

zeros(T::Type, dims::Dims) = fill!(Array{T}(dims), 0)
zeros(T::Type, dims::Integer...) = zeros(T, convert(Dims, dims))

and likewise for ones?

@nalimilan
Copy link
Member

@martinholters's proposal sounds good.

Would it make sense to officially add a
zeros(::Type, ::AbstractArray) method? I used this quite frequently and did not realize it is unsupported until now.

Why don't use just use zeros(::AbstractArray)? The type must match the array's element type anyway, so it doesn't sound terribly useful.

@jw3126
Copy link
Contributor Author

jw3126 commented Nov 8, 2016

Why don't use just use zeros(::AbstractArray)? The type must match the array's element type anyway, so it doesn't sound terribly useful.

Ah usually I want say an array of Float64 zeros with the layout as a given matrix of Int64. Now I don't explicitly write silly code like zeros(Float64, [1.]), but this case occurs when using generic programming.

@jw3126
Copy link
Contributor Author

jw3126 commented Nov 8, 2016

@martinholters I can provide a PR with your suggestion if you like.

@yuyichao
Copy link
Contributor

yuyichao commented Nov 8, 2016

Ah usually I want say an array of Float64 zeros with the layout as a given matrix of Int64.

We support this in similar so I think it makes sense to support it in zeros too.

@jw3126
Copy link
Contributor Author

jw3126 commented Nov 8, 2016

The signature of similar is
similar(array, [element_type=eltype(array)], [dims=size(array)])

would you suggest to add
zeros(array, [element_type=eltype(array)], [dims=size(array)])
?

@stevengj stevengj added the kind:bug Indicates an unexpected problem or unintended behavior label Nov 8, 2016
@martinholters
Copy link
Member

@jw3126 Go ahead with a PR. But restricting the current signature to ::Dims and ::Integer... might qualify as a backport-worthy bugfix, while supporting a similar-like signature would be a new feature, so best to make these two separate commits.

@yuyichao
Copy link
Contributor

yuyichao commented Nov 9, 2016

Removing a signature will cause code that used to "work" to error so I'm not sure how backportable is that. Sure, those code shouldn't be using this signature in the first place but there many other cases where we don't backport a change because of this.

@jw3126 jw3126 mentioned this issue Dec 17, 2016
jw3126 added a commit to jw3126/julia that referenced this issue Dec 18, 2016
jw3126 added a commit to jw3126/julia that referenced this issue Dec 19, 2016
* Fix JuliaLang#19265.
* Add methods to zeros, ones with analgous signature to similar.
jw3126 added a commit to jw3126/julia that referenced this issue Dec 20, 2016
* Fix JuliaLang#19265.
* Add methods to zeros, ones with analgous signature to similar.
ViralBShah pushed a commit that referenced this issue Dec 22, 2016
* Fix #19265.
* Add methods to zeros, ones with analgous signature to similar.
* news
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

5 participants