deprecate unintended methods of zeros, ones #21183

jw3126 · 2017-03-27T18:35:02Z

See @timholy's comment in #19635.

timholy · 2017-03-28T02:02:29Z

base/deprecated.jl

@@ -1298,6 +1298,10 @@ end
     end
 end

+# #19635
+@deprecate zeros{T <: Number}(::Type{T}, arr::Range)  zeros(T, length(arr))


It's not just Range. 0.5:

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

master:

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

On 0.5, zeros(T, arg) ends up calling fill!(convert(Array{T}, arg), zero(T)). So anything that can be converted to an Array is a plausible input.

Right. In fact a glitch with zeros(T, arr) #19265 was the trigger for the zeros PR #19635.

I could restore zeros(Int, [1,2,3]) with a depwarn, but zeros(Int, [1,2,3]) was buggy, see #19265 and I have a test that this case throws an error now. So removing the error in favor of a depwarn is a bit awkward.

tkelman · 2017-03-28T22:02:42Z

test/arrayops.jl

-        @test x == [1.]
-    end
+    # #19265"
+    @test_throws Exception zeros(Float64, [1.])


@test_throws Exception isn't very useful, there could be a typo in the implementation and it would pass.

Yes originally it was a MethodError. And indeed a MethodError will be thrown, when the depwarn is deleted. Should I change it back to MethodError again and make the depwarn throw a MethodError?

tkelman · 2017-03-28T22:03:12Z

base/deprecated.jl

+    @eval function ($f){T}(::Type{T}, arr::Array{T}) 
+        msg = string("`", $f , "{T}(::Type{T}, arr::Array{T})` is deprecated, use ",
+                            "`", $f , "(T, size(arr))` instead.",
+                            "A `MethodError` will be thrown."


error(::String) throws an ErrorException, not a MethodError

jw3126 · 2017-03-29T08:14:05Z

I encounter a strange behaviour.

@test_throws MethodError zeros(Float64, [1.])

fails depending on where it is called. In the REPL it passes. If I do juliadev runtests.jl arrayops
it also passes. If I just to juliadev runtests.jl I get

Error in testset arrayops:
Test Failed
  Expression: zeros(Float64, [1.0])
    Expected: MethodError
      Thrown: ErrorException

If I just type zeros(Float64, [1.]) in the repl it also works:

   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: http://docs.julialang.org
   _ _   _| |_  __ _   |  Type "?help" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.6.0-pre.alpha.268 (2017-03-29 07:38 UTC)
 _/ |\__'_|_|_|\__'_|  |  depzeros/8148f23* (fork: 3 commits, 1 day)
|__/                   |  x86_64-linux-gnu

julia> zeros(Float64, [1.])
WARNING: `zeros{T}(::Type{T}, arr::Array{T})` is deprecated, use `zeros(T, size(arr))` instead. A `MethodError` will be thrown.
Stacktrace:
 [1] depwarn(::String, ::Symbol) at ./deprecated.jl:64
 [2] zeros(::Type{Float64}, ::Array{Float64,1}) at ./deprecated.jl:1310
 [3] eval(::Module, ::Any) at ./boot.jl:235
 [4] eval_user_input(::Any, ::Base.REPL.REPLBackend) at ./REPL.jl:66
 [5] macro expansion at ./REPL.jl:97 [inlined]
 [6] (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:73
while loading no file, in expression starting on line 0
ERROR: MethodError: no method matching zeros(::Type{Float64}, ::Array{Float64,1})
Closest candidates are:
  zeros(::Type{T}, ::Array{T,N} where N) where T at deprecated.jl:1306
  zeros(::Type, ::Any) at deprecated.jl:50
  zeros(::Type, ::Any...) at array.jl:252
  ...
Stacktrace:
 [1] zeros(::Type{Float64}, ::Array{Float64,1}) at ./deprecated.jl:1311

tkelman · 2017-03-29T10:54:51Z

likely depends on the value of the --depwarn flag, if you're trying to test a method that gives a warning

unfortunately MethodError can't really be customized with a deprecation message at the moment. though might be better to have the message only in the depwarn, then the error can be whatever. Tim, opinion on what to throw?

jw3126 · 2017-03-29T15:43:23Z

Okay, do I get this correctly: CI will never pass if there is a @test_throws of an expression emitting a depwarning?

jw3126 · 2017-04-02T09:50:16Z

I think this PR is ready.

timholy · 2017-04-02T10:08:39Z

base/array.jl

@@ -251,6 +251,14 @@ for (fname, felt) in ((:zeros,:zero), (:ones,:one))
        $fname(dims::Tuple) = ($fname)(Float64, dims)
        $fname(T::Type, dims...) = $fname(T, dims)
        $fname(dims...) = $fname(dims)
+
+        # #19635
+        function ($fname){T}(::Type{T}, arr::Array{T})


I think this should be moved to base/deprecated.jl. That way it will be deleted when we delete the 0.6 deprecations (which happens late in the next release cycle); we might not notice/remember to delete it if it's in array.jl.

One also needs to change a @test_throws when this is deleted. Thats why I put it here and not in deprecated.jl. You are still in favor of deprecated.jl?

Hmm, that is a bit tricky. Still, I think I'd favor moving this to deprecated.jl and then writing the test like this:

@test_throws ErrorException error("some error") # TODO: change this to MethodError when 0.6 deprecations are deleted

timholy · 2017-04-03T21:40:58Z

@jw3126, I was feeling guilty about asking for more changes, so I just pushed #21265. That's mostly for discussion purposes (you can make the modifications yourself if you prefer). The main differences:

zeros(T, i::Integer) wasn't a deprecation, so I moved it back to array.jl;
I wasn't sure why zeros(T, ::Array) was an error rather than a deprecation warning

If you're fine with #21265, we can merge that, but if it has problems please let me know.

jw3126 · 2017-04-04T05:42:20Z

zeros(T, i::Integer) won't be needed anymore, when the deprecations are deleted. Thats why it was in the deprecated.jl.
zeros(T, ::Array) was an error so that I did not have to delete the test. While zeros{T}(T, ::Vector{T}) worked in v0.5, this was by accident and buggy zeros function mutates arguments #19265. Thats why this test exists.

timholy · 2017-04-04T12:08:13Z

Sounds good.

timholy · 2017-04-04T12:08:51Z

Oh, and thanks for doing this!

KristofferC · 2017-04-08T12:13:25Z

This makes the tests in Tensors.jl fail (bisected to here) Ferrite-FEM/Tensors.jl#53 (comment).

Pre this PR:

julia> eltype(zeros(Tensor{2, 3}, 3))
Tensors.Tensor{2,3,Float64,9}

julia> @which zeros(Tensor{2, 3}, 3)
zeros(::Type{T}, dims...) where T<:Tensors.AbstractTensor in Tensors at /home/kristoffer/Dropbox/julia/Tensors/src/constructors.jl:106

After:

julia> eltype(zeros(Tensor{2, 3}, 3))
Tensors.Tensor{2,3,T,M} where M where T<:Real

julia> @which zeros(Tensor{2, 3}, 3)
ERROR: no method found for the specified argument types
Stacktrace:
 [1] which(::Any, ::Any) at ./reflection.jl:824

The error message from @which is confusing because I can apparently call the method...

cc @timholy @iamnapo

jw3126 · 2017-04-08T14:18:51Z

That is weird.

…#21183).

…21844)

…21844) (cherry picked from commit f6bd5fe)

ararslan added the kind:deprecation This change introduces or involves a deprecation label Mar 27, 2017

timholy reviewed Mar 28, 2017

View reviewed changes

tkelman reviewed Mar 28, 2017

View reviewed changes

jw3126 force-pushed the depzeros branch from 8148f23 to 6d7c0e9 Compare March 29, 2017 08:43

jw3126 added 3 commits March 29, 2017 10:50

deprecate unintended methods of zeros, ones

12f3b4b

fix

e5f8659

fix

6b4f531

jw3126 force-pushed the depzeros branch from 6d7c0e9 to 6b4f531 Compare March 29, 2017 08:50

fix

d540dd7

timholy reviewed Apr 2, 2017

View reviewed changes

fix

504944d

timholy mentioned this pull request Apr 3, 2017

Deprecation of zeros and ones #21265

Closed

timholy merged commit 18e864b into JuliaLang:master Apr 4, 2017

KristofferC mentioned this pull request Apr 8, 2017

Add an integer power function for second order tensors Ferrite-FEM/Tensors.jl#53

Merged

Sacha0 added a commit to Sacha0/julia that referenced this pull request May 13, 2017

Add NEWS.md entry for dep of unintended zeros/ones methods (JuliaLang…

7079dcb

…#21183).

Sacha0 added the needs news A NEWS entry is required for this change label May 13, 2017

tkelman removed the needs news A NEWS entry is required for this change label May 14, 2017

tkelman pushed a commit that referenced this pull request May 14, 2017

Add NEWS.md entry for dep of unintended zeros/ones methods (#21183). (#…

f6bd5fe

…21844)

tkelman pushed a commit that referenced this pull request May 14, 2017

Add NEWS.md entry for dep of unintended zeros/ones methods (#21183). (#…

c7e4da6

…21844) (cherry picked from commit f6bd5fe)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deprecate unintended methods of zeros, ones #21183

deprecate unintended methods of zeros, ones #21183

jw3126 commented Mar 27, 2017

timholy Mar 28, 2017

jw3126 Mar 28, 2017

jw3126 Mar 28, 2017 •

edited

Loading

tkelman Mar 28, 2017

jw3126 Mar 29, 2017

tkelman Mar 28, 2017

jw3126 commented Mar 29, 2017

tkelman commented Mar 29, 2017

jw3126 commented Mar 29, 2017

jw3126 commented Apr 2, 2017

timholy Apr 2, 2017

jw3126 Apr 2, 2017

timholy Apr 2, 2017

timholy commented Apr 3, 2017

jw3126 commented Apr 4, 2017

timholy commented Apr 4, 2017

timholy commented Apr 4, 2017

KristofferC commented Apr 8, 2017 •

edited

Loading

jw3126 commented Apr 8, 2017

deprecate unintended methods of zeros, ones #21183

deprecate unintended methods of zeros, ones #21183

Conversation

jw3126 commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jw3126 Mar 28, 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

jw3126 commented Mar 29, 2017

tkelman commented Mar 29, 2017

jw3126 commented Mar 29, 2017

jw3126 commented Apr 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy commented Apr 3, 2017

jw3126 commented Apr 4, 2017

timholy commented Apr 4, 2017

timholy commented Apr 4, 2017

KristofferC commented Apr 8, 2017 • edited Loading

jw3126 commented Apr 8, 2017

jw3126 Mar 28, 2017 •

edited

Loading

KristofferC commented Apr 8, 2017 •

edited

Loading