RFC: Deprecate Array(T, dims...) #19989

ararslan · 2017-01-12T02:02:37Z

This PR implements one of the changes mentioned in #19598: deprecate the Array(T, dims...) syntax in favor of Array{T,N}(dims...). The manual has claimed the former syntax to be deprecated ever since the latter syntax was introduced, though no formal deprecation ever happened... until now!

Thanks to @vtjnash and @JeffBezanson for their help on this.

vtjnash · 2017-01-12T03:04:08Z

base/sysimg.jl

-Array{T}(::Type{T}, d::Integer...) = Array(T, convert(Tuple{Vararg{Int}}, d))
-Array{T}(::Type{T}, m::Integer)                       = Array{T,1}(Int(m))
-Array{T}(::Type{T}, m::Integer,n::Integer)            = Array{T,2}(Int(m),Int(n))
-Array{T}(::Type{T}, m::Integer,n::Integer,o::Integer) = Array{T,3}(Int(m),Int(n),Int(o))


move these to deprecations.jl?

Good catch, thanks.

vtjnash · 2017-01-12T03:05:23Z

src/julia-syntax.scm

@@ -2242,7 +2242,7 @@
      ,.(map (lambda (v r) `(= ,v (call (top length) ,r))) lengths rv)
      (scope-block
       (block
-        (= ,result (call (core Array) ,atype ,@lengths))
+        (= ,result (call (curly Array ,atype) ,@lengths))


can also add ,(length lengths) to the (curly, if I'm not mistaken, to splice in the number of dimensions also

tkelman · 2017-01-12T04:39:42Z

@nanosoldier runbenchmarks(ALL, vs = ":master")

ararslan · 2017-01-12T04:57:48Z

...huh. Well, the fact that this works on Linux and macOS but not on Windows is rather concerning. Looks like it's failing on mmap because something in there is using the old syntax on Windows.

quinnj · 2017-01-12T05:37:27Z

I think this line is the offender.

ararslan · 2017-01-12T05:47:29Z

Excellent catch, @quinnj, thanks! I didn't even realize that Array(T) created a zero-dimensional array. Fix incoming.

~~Edit: Side note, may have inadvertently canceled Nanosoldier by manually squashing the fix and force pushing...~~

musm · 2017-01-12T06:26:27Z

base/libc.jl

@@ -277,7 +277,7 @@ if is_windows()
        const FORMAT_MESSAGE_FROM_SYSTEM = UInt32(0x1000)
        const FORMAT_MESSAGE_IGNORE_INSERTS = UInt32(0x200)
        const FORMAT_MESSAGE_MAX_WIDTH_MASK = UInt32(0xFF)
-        lpMsgBuf = Array(Ptr{UInt16})
+        lpMsgBuf = Array{Ptr{UInt16},0}(())


isn't this the same as lpMsgBuf = Array{Ptr{UInt16},0}() ?

No, that's actually a MethodError.

julia> Array{Ptr{UInt16},0}() ERROR: MethodError: no method matching Array{Ptr{UInt16},0}() Closest candidates are: Array{Ptr{UInt16},0}{T,N}(::Tuple{Vararg{Int64,N}}) at boot.jl:310 Array{Ptr{UInt16},0}{T}(::Any) at sysimg.jl:53

(The above is from Julia 0.5; that behavior wasn't introduced in this PR)

Strange I don't get a method error (Commit db4aaa3* (3 days old master))

julia> Array{Ptr{UInt16},0}() 0-dimensional Array{Ptr{UInt16},0}: Ptr{UInt16} @0x0000000000000003 julia> versioninfo() Julia Version 0.6.0-dev.2005 Commit db4aaa3* (2017-01-09 10:16 UTC) Platform Info: OS: Windows (x86_64-w64-mingw32) CPU: Intel(R) Core(TM) i7-4510U CPU @ 2.00GHz WORD_SIZE: 64 BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Haswell) LAPACK: libopenblas64_ LIBM: libopenlibm LLVM: libLLVM-3.9.1 (ORCJIT, haswell)

Oh yeah, looks like the behavior changed in 0.6, nice.

nanosoldier · 2017-01-12T08:49:59Z

Something went wrong when running your job:

NanosoldierError: error when preparing/pushing to report repo: ErrorException("failed process: Process(`git push`, ProcessExited(1)) [1]")

Unfortunately, the logs could not be uploaded.
cc @jrevels

jrevels · 2017-01-12T14:01:51Z

Looks like there was a major service outage on GitHub at about the time that Nanosoldier attempted to upload the logs. Retriggering:

@nanosoldier runbenchmarks(ALL, vs = ":master")

StefanKarpinski · 2017-01-12T16:54:08Z

Unrelated Pkg test failure on one of the Windows tests. Since those tests are only run after all the other tests have already passed, this is pretty safe.

mbauman · 2017-01-12T17:04:31Z

Looks good, thanks! Maybe remove the deprecated statement in the manual now, too? We don't typically include prose like that and it'll likely be forgotten and become outdated.

Also, SharedArrays have a constructor of the same style.

nanosoldier · 2017-01-12T18:13:12Z

Something went wrong when running your job:

NanosoldierError: error when preparing/pushing to report repo: ErrorException("failed process: Process(`git push`, ProcessExited(1)) [1]")

Unfortunately, the logs could not be uploaded.
cc @jrevels

jrevels · 2017-01-12T18:21:24Z

Wow, Nanosoldier's push is getting rejected because the all depwarns here are causing the STDERR log to exceed GitHub's file size limit of 100 MB. That's...a lot of depwarns 😛 Mostly in HDF5 and JLD. I'll remove that big log and just upload the other (much smaller) logs and post the link here.

EDIT: Here is the benchmark report. Looks like some of the benchmarks will have to be updated.

Here's how I "confirmed" that most of the depwarns in the oversized log were this PR's depwarns:

➜  logs git:(master) awk '/^WARNING/' 459bc3a62c227c795747b217a870280ac4827e0b_primary.err | wc -l
19170
➜  logs git:(master) awk '/^WARNING\: Array/' 459bc3a62c227c795747b217a870280ac4827e0b_primary.err | wc -l
19126

tkelman · 2017-01-12T19:10:30Z

base/array.jl

@@ -25,6 +25,14 @@ typealias DenseVecOrMat{T} Union{DenseVector{T}, DenseMatrix{T}}

 import Core: arraysize, arrayset, arrayref

+"""
+    Array{T}(dims)


list both parameters {T,N} here?

ararslan · 2017-01-12T22:36:20Z

Hm, I'm not entirely sure what to do about SharedArrays. When I make the appropriate changes, I get

sharedarray.jl
WARNING: static parameter T does not occur in signature for Type at sharedarray.jl:57.
The method will not be callable.
WARNING: static parameter T does not occur in signature for Type at sharedarray.jl:113.
The method will not be callable.
WARNING: static parameter T does not occur in signature for Type at sharedarray.jl:147.
The method will not be callable.

during the build. These are the defined methods:

julia> methods(SharedArray)
# 9 methods for generic function "(::Type)":
(::Type{SharedArray}){T}(I::Int64...; kwargs...) in Base at sharedarray.jl:113
(::Type{SharedArray}){T,N}(dims::Tuple{Vararg{Int64,N}}; init, pids) in Base at sharedarray.jl:57
(::Type{SharedArray}){N}(filename::AbstractString, dims::Tuple{Vararg{Int64,N}}) in Base at sharedarray.jl:147
(::Type{SharedArray}){T,N}(filename::AbstractString, dims::Tuple{Vararg{Int64,N}}, offset::Integer; init, mode, pids) in Base at sharedarray.jl:147
(::Type{SharedArray}){T,N}(::Type{T}, dims::Tuple{Vararg{Int64,N}}; kwargs...) in Base at deprecated.jl:49
(::Type{SharedArray}){T}(::Type{T}, dims::Int64...; kwargs...) in Base at deprecated.jl:49
(::Type{SharedArray}){T,N}(filename::AbstractString, ::Type{T}, dims::Tuple{Vararg{Int64,N}}, offset; kwargs...) in Base at deprecated.jl:49
(::Type{SharedArray}){T}(filename::AbstractString, ::Type{T}, dims::Tuple{Vararg{T<:Any,N<:Any}}, offset; kwargs...) in Base at deprecated.jl:49
(::Type{T}){T}(arg) in Base at sysimg.jl:24

Clearly I've done something wrong here, just quite sure what yet.

yuyichao · 2017-01-12T22:51:14Z

The type parameter is not used as the warning suggests. You need to replace the implementation with one of the methods above.

ararslan · 2017-01-13T00:16:18Z

I'm still having a hard time figuring out what's wrong with the SharedArray changes. I've pushed the commit (but skipped CI because I'm sure it'll fail) so if anyone is willing to provide some guidance that would be much appreciated.

yuyichao · 2017-01-13T00:51:24Z

base/sharedarray.jl

@@ -53,7 +53,7 @@ computation with the master process acting as a driver.
 If an `init` function of the type `initfn(S::SharedArray)` is specified, it is called on all
 the participating workers.
 """
-function SharedArray{T,N}(::Type{T}, dims::Dims{N}; init=false, pids=Int[])
+function SharedArray{T,N}(dims::Dims{N}; init=false, pids=Int[])


Remove the definition on line 31 and move it here. This isn't a valid method definition at all. I.e. this does NOT define a method for SharedArray{T,N}.

yuyichao · 2017-01-13T00:54:10Z

base/sharedarray.jl

@@ -110,10 +110,10 @@ function SharedArray{T,N}(::Type{T}, dims::Dims{N}; init=false, pids=Int[])
    S
 end

-SharedArray(T, I::Int...; kwargs...) = SharedArray(T, I; kwargs...)
+SharedArray{T}(I::Int...; kwargs...) = SharedArray{T,length(I)}(I; kwargs...)


This should become a method of ShardArray{T} too replacing the ones on line 33 to 39.

yuyichao · 2017-01-13T00:55:20Z

base/sharedarray.jl

@@ -142,7 +142,7 @@ file is not writable.
 `offset` allows you to skip the specified number of bytes at the
 beginning of the file.
 """
-function SharedArray{T,N}(filename::AbstractString, ::Type{T}, dims::NTuple{N,Int},
+function SharedArray{T,N}(filename::AbstractString, dims::NTuple{N,Int},


This should become a new method of SharedArray{T} this also needs to go in Compat.jl since it's not available on old julia versions AFAICT.

yuyichao · 2017-01-13T00:56:44Z

base/sharedarray.jl

@@ -307,21 +307,25 @@ localindexes(S::SharedArray) = S.pidx > 0 ? range_1dim(S, S.pidx) : 1:0
 unsafe_convert{T}(::Type{Ptr{T}}, S::SharedArray) = unsafe_convert(Ptr{T}, sdata(S))

 function convert(::Type{SharedArray}, A::Array)
-    S = SharedArray(eltype(A), size(A))
+    sz = size(A)
+    S = SharedArray{eltype(A),length(sz)}(sz)


Note that there isn't any methods defined for SharedArray{T,N} apart from the inner constructor so this won't work. You need to either not use them and use SharedArray{T} instead or check for conflict/ambiguity and define them.

yuyichao · 2017-01-13T00:58:18Z

base/deprecated.jl

@@ -1649,4 +1649,12 @@ isempty(::Task) = error("isempty not defined for Tasks")
 @deprecate Array{T}(::Type{T}, m::Integer,n::Integer)            Array{T,2}(Int(m),Int(n))
 @deprecate Array{T}(::Type{T}, m::Integer,n::Integer,o::Integer) Array{T,3}(Int(m),Int(n),Int(o))

+# Likewise for SharedArrays
+@deprecate SharedArray{T,N}(::Type{T}, dims::Dims{N}; kwargs...) SharedArray{T,N}(dims; kwargs...)


Similar to comment below, these are calling non-existing methods.

ararslan · 2017-01-13T03:33:43Z

I think I've addressed all of your comments, @yuyichao. Thanks so much for your help!

StefanKarpinski · 2017-01-13T19:41:46Z

base/libc.jl

@@ -277,7 +277,7 @@ if is_windows()
        const FORMAT_MESSAGE_FROM_SYSTEM = UInt32(0x1000)
        const FORMAT_MESSAGE_IGNORE_INSERTS = UInt32(0x200)
        const FORMAT_MESSAGE_MAX_WIDTH_MASK = UInt32(0xFF)
-        lpMsgBuf = Array(Ptr{UInt16})
+        lpMsgBuf = Array{Ptr{UInt16},0}()


Not necessary to change in this PR since this is the exact equivalent of what was there, but shouldn't this be updated to use a Ref{Ptr{UInt16}} instead of a zero-dimensional array?

jrevels · 2017-01-14T17:37:02Z

This PR's depwarns will "break" Nanosoldier's benchmark runs against master now that it's merged. Can we merge JuliaIO/JLD.jl#116 so that I can deploy it to Nanosoldier ASAP (I would merge it but I don't have commit access to JLD)?

jrevels · 2017-01-14T17:56:37Z

Also would be good to get JuliaIO/HDF5.jl#354 merged for the same reason.

Array(T, dims...) to Array{T}(dims...) (ref JuliaLang/julia#19989)

ararslan · 2017-01-14T19:11:20Z

Very sorry, should have waited to merge!

as the latter has been deprecated in JuliaLang/julia#19989.

stevengj · 2017-01-24T16:12:02Z

~~I have to say that I dislike the deprecation of Array{T}(dims...)~~, oh, it seems like that has been removed already.

stevengj · 2017-01-24T16:17:46Z

Maybe I got confused by the deprecation message. Can we change it so that Array(T, dims...) is deprecated in favor of Array{T}(dims...), so that it doesn't falsely imply that you need to write the number of dimensions manually?

Ref JuliaLang/julia#19989

…).

)

) (cherry picked from commit 609b2d6)

…19989.

…21995)

…21995) (cherry picked from commit e68690d)

vtjnash approved these changes Jan 12, 2017

View reviewed changes

ararslan force-pushed the aa/array-syntax branch from 2b086fe to fdc945a Compare January 12, 2017 03:43

ararslan force-pushed the aa/array-syntax branch from fdc945a to 91f7cb9 Compare January 12, 2017 06:03

musm reviewed Jan 12, 2017

View reviewed changes

jrevels added a commit to jrevels/JLD.jl that referenced this pull request Jan 12, 2017

Array(T, dims...) to Array{T}(dims...) (ref JuliaLang/julia#19989)

b7e64de

jrevels mentioned this pull request Jan 12, 2017

Array(T, dims...) to Array{T}(dims...) (ref JuliaLang/julia#19989) JuliaIO/JLD.jl#116

Merged

ararslan force-pushed the aa/array-syntax branch from 91f7cb9 to 4bea194 Compare January 12, 2017 19:03

tkelman reviewed Jan 12, 2017

View reviewed changes

kshyatt added the deprecation This change introduces or involves a deprecation label Jan 12, 2017

ararslan force-pushed the aa/array-syntax branch from 4bea194 to f0faa78 Compare January 13, 2017 00:15

yuyichao requested changes Jan 13, 2017

View reviewed changes

ararslan force-pushed the aa/array-syntax branch from f0faa78 to 753861e Compare January 13, 2017 03:28

ararslan force-pushed the aa/array-syntax branch 3 times, most recently from ba77c5a to 92ee7ed Compare January 13, 2017 19:24

StefanKarpinski reviewed Jan 13, 2017

View reviewed changes

ararslan force-pushed the aa/array-syntax branch from 2fa11d1 to 36122a7 Compare January 14, 2017 02:22

ararslan merged commit 50d0d5d into JuliaLang:master Jan 14, 2017

ararslan deleted the aa/array-syntax branch January 14, 2017 17:31

jrevels added a commit to JuliaCI/BaseBenchmarks.jl that referenced this pull request Jan 14, 2017

Array(T, dims...) to Array{T}(dims...) (ref JuliaLang/julia#19989)

8c1ca8c

jrevels mentioned this pull request Jan 14, 2017

make report uploading robust to oversized logs JuliaCI/Nanosoldier.jl#24

Open

tkelman added a commit to JuliaIO/JLD.jl that referenced this pull request Jan 14, 2017

Merge pull request #116 from jrevels/jr/arraydepwarn

0cb8489

Array(T, dims...) to Array{T}(dims...) (ref JuliaLang/julia#19989)

jrevels added a commit to JuliaCI/BaseBenchmarks.jl that referenced this pull request Jan 14, 2017

Array(T, dims...) to Array{T}(dims...) (ref JuliaLang/julia#19989)

7c0f31d

StefanKarpinski mentioned this pull request Jan 14, 2017

things we should deprecate, 0.6 edition #19598

Closed

22 tasks

jrevels added a commit to JuliaCI/BaseBenchmarks.jl that referenced this pull request Jan 14, 2017

Array(T, dims...) to Array{T}(dims...) (ref JuliaLang/julia#19989) (#59)

41de7be

martinholters added a commit to HSU-ANT/ACME.jl that referenced this pull request Jan 16, 2017

Avoid Array(Float64, ...) in favour Array{Float64}(...)

9053a75

as the latter has been deprecated in JuliaLang/julia#19989.

stevengj mentioned this pull request Jan 24, 2017

change Array(T, dims...) deprecation message to Array{T}(dims) #20217

Merged

maleadt added a commit to JuliaAttic/CUDArt.jl that referenced this pull request Feb 16, 2017

Fix deprecated array constructor syntax.

9d04776

Ref JuliaLang/julia#19989

vchuravy pushed a commit to JuliaAttic/CUDArt.jl that referenced this pull request Feb 17, 2017

Fix deprecated array constructor syntax.

8e0a006

Ref JuliaLang/julia#19989

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

Add NEWS.md entry for deprecation of Array(T, dims...) (JuliaLang#19989…

92bad43

…).

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

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

Add NEWS.md entry for deprecation of Array(T, dims...) (JuliaLang#19989…

b31a509

…).

tkelman self-assigned this May 15, 2017

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

Add NEWS.md entry for deprecation of Array(T, dims...) (#19989). (#21857

609b2d6

)

tkelman removed their assignment May 15, 2017

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

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

Add NEWS.md entry for deprecation of Array(T, dims...) (#19989). (#21857

d552789

) (cherry picked from commit 609b2d6)

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

Mention SharedArrays constructor deps in NEWS.md entry for JuliaLang#…

463464d

…19989.

ararslan pushed a commit that referenced this pull request May 21, 2017

Mention SharedArrays constructor deps in NEWS.md entry for #19989. (#…

e68690d

…21995)

tkelman pushed a commit that referenced this pull request Jun 3, 2017

Mention SharedArrays constructor deps in NEWS.md entry for #19989. (#…

2d9d5d3

…21995) (cherry picked from commit e68690d)

fredrikekre mentioned this pull request Dec 2, 2017

deprecate constructors with type as first argument JuliaArrays/OffsetArrays.jl#34

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Deprecate Array(T, dims...) #19989

RFC: Deprecate Array(T, dims...) #19989

ararslan commented Jan 12, 2017

vtjnash Jan 12, 2017

ararslan Jan 12, 2017

vtjnash Jan 12, 2017

tkelman commented Jan 12, 2017

ararslan commented Jan 12, 2017 •

edited

Loading

quinnj commented Jan 12, 2017

ararslan commented Jan 12, 2017 •

edited

Loading

musm Jan 12, 2017

ararslan Jan 12, 2017 •

edited

Loading

musm Jan 12, 2017

ararslan Jan 12, 2017

nanosoldier commented Jan 12, 2017

jrevels commented Jan 12, 2017

StefanKarpinski commented Jan 12, 2017

mbauman commented Jan 12, 2017

nanosoldier commented Jan 12, 2017

jrevels commented Jan 12, 2017 •

edited

Loading

tkelman Jan 12, 2017

ararslan commented Jan 12, 2017 •

edited

Loading

yuyichao commented Jan 12, 2017

ararslan commented Jan 13, 2017

yuyichao Jan 13, 2017

yuyichao Jan 13, 2017

yuyichao Jan 13, 2017

yuyichao Jan 13, 2017

yuyichao Jan 13, 2017

ararslan commented Jan 13, 2017

StefanKarpinski Jan 13, 2017

musm Jan 14, 2017

jrevels commented Jan 14, 2017

jrevels commented Jan 14, 2017

ararslan commented Jan 14, 2017

stevengj commented Jan 24, 2017 •

edited

Loading

stevengj commented Jan 24, 2017

RFC: Deprecate Array(T, dims...) #19989

RFC: Deprecate Array(T, dims...) #19989

Conversation

ararslan commented Jan 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Jan 12, 2017

ararslan commented Jan 12, 2017 • edited Loading

quinnj commented Jan 12, 2017

ararslan commented Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

ararslan Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nanosoldier commented Jan 12, 2017

jrevels commented Jan 12, 2017

StefanKarpinski commented Jan 12, 2017

mbauman commented Jan 12, 2017

nanosoldier commented Jan 12, 2017

jrevels commented Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

ararslan commented Jan 12, 2017 • edited Loading

yuyichao commented Jan 12, 2017

ararslan commented Jan 13, 2017

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

ararslan commented Jan 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrevels commented Jan 14, 2017

jrevels commented Jan 14, 2017

ararslan commented Jan 14, 2017

stevengj commented Jan 24, 2017 • edited Loading

stevengj commented Jan 24, 2017

ararslan commented Jan 12, 2017 •

edited

Loading

ararslan commented Jan 12, 2017 •

edited

Loading

ararslan Jan 12, 2017 •

edited

Loading

jrevels commented Jan 12, 2017 •

edited

Loading

ararslan commented Jan 12, 2017 •

edited

Loading

stevengj commented Jan 24, 2017 •

edited

Loading