rewrite all zeros(...) calls #25392

fredrikekre · 2018-01-04T19:22:54Z

This rewrites all zeros([T, ] dims...) calls, first commit is #25087

fredrikekre · 2018-01-04T19:31:18Z

One interesting thing that I encountered while doing this transformation is the difficulty to understand calls like zeros(A, B) -- you have no idea if it is a Vector{A} of length B or if it is a Matrix{Float64} with size A × B. In here there was even calls like zeros(T, n) which I naturally thought was a Vector{T}, but turned out that in that context T was just an integer...

Therefore; I think we should consider doing one of the following:

@deprecate zeros(::Type{T}, dims...) fill(T(0), dims...) i.e. make zeros always return Float64
@deprecate zeros(dims...) zeros(Float64, dims...) i.e. require the first argument to be a type

ararslan · 2018-01-04T21:20:48Z

+1 for requiring the first argument to be a type.

GunnarFarneback · 2018-01-04T23:11:23Z

stdlib/SuiteSparse/test/umfpack.jl

@@ -159,7 +159,7 @@
        N = 10
        p = 0.5
        A = N*I + sprand(N, N, p)
-        X = zeros(Complex{Float64}, N, N)
+        X = fill(0.0+im, N, N)


That's not a complex zero.

GunnarFarneback · 2018-01-04T23:19:36Z

base/multidimensional.jl

@@ -1156,7 +1156,7 @@ the other elements are left untouched.
 ```jldoctest
 julia> x = [1., 0., 3., 0., 5.];

-julia> y = zeros(7);
+julia> y = Vector{Float64}(uninitialized, 7);


The example is meant to show the last two zeros being preserved, so filling with uninitialized doesn't seem right.

ggggggggg · 2018-01-05T16:31:51Z

What about something like this, so it is really obvious where the type should go?

julia> struct Zeros{T} end

julia> function (::Type{Zeros{T}})(n...) where T
       zeros(T,n...)
       end

julia> Zeros{Int}(4,5)
4×5 Array{Int64,2}:
 0  0  0  0  0
 0  0  0  0  0
 0  0  0  0  0
 0  0  0  0  0

JeffBezanson · 2018-01-05T19:34:46Z

Triage likes zeros. Since zero is a globally-reasonable default for initializing arrays, it makes sense to draw the line at having a special function for zeros, and using fill for everything else.

… something else is clearly better

fredrikekre · 2018-01-11T16:28:49Z

Rebased and regrouped the commits: The first commit removes all calls to zeros where the value 0 is not special, and where something else is better to use. The second commit rewrites all zeros(dims) calls (i.e. calls withouth a type specified) and then follows a number of commits where we use literal 0's in combination with fill. Those rewrites typically results in less code. The last commit rewrites zeros(T, dims) to fill(zero(T), dims) which typically becomes a little bit longer, and gives a taste of a post zeros world.

If we decide to keep zeros (which it looks like we will) then I will drop the last commit, and then submit a follow up PR with deprecation of zeros(dims) to zeros(Float64, dims).

Sacha0 · 2018-01-13T04:24:09Z

stdlib/IterativeEigensolvers/docs/src/index.md

@@ -10,7 +10,7 @@ general nonsymmetric matrices respectively.

 For the single matrix version,

-`eigs(A; nev=6, ncv=max(20,2*nev+1), which=:LM, tol=0.0, maxiter=300, sigma=nothing, ritzvec=true, v0=zeros((0,))) -> (d,[v,],nconv,niter,nmult,resid)`
+`eigs(A; nev=6, ncv=max(20,2*nev+1), which=:LM, tol=0.0, maxiter=300, sigma=nothing, ritzvec=true, v0=[]) -> (d,[v,],nconv,niter,nmult,resid)`


In case preservation of the element type matters in this case, Float64[]? :)

Sacha0 · 2018-01-13T04:24:36Z

stdlib/IterativeEigensolvers/docs/src/index.md

@@ -125,7 +125,7 @@ julia> λ

 For the two-input generalized eigensolution version,

-`eigs(A, B; nev=6, ncv=max(20,2*nev+1), which=:LM, tol=0.0, maxiter=300, sigma=nothing, ritzvec=true, v0=zeros((0,))) -> (d,[v,],nconv,niter,nmult,resid)`
+`eigs(A, B; nev=6, ncv=max(20,2*nev+1), which=:LM, tol=0.0, maxiter=300, sigma=nothing, ritzvec=true, v0=[]) -> (d,[v,],nconv,niter,nmult,resid)`


Likewise here, in case preservation of the element type matters in this case, Float64[]? :)

Sacha0 · 2018-01-13T04:25:23Z

stdlib/IterativeEigensolvers/src/IterativeEigensolvers.jl

@@ -17,7 +17,7 @@ using .ARPACK

 ## eigs
 """
-    eigs(A; nev=6, ncv=max(20,2*nev+1), which=:LM, tol=0.0, maxiter=300, sigma=nothing, ritzvec=true, v0=zeros((0,))) -> (d,[v,],nconv,niter,nmult,resid)
+    eigs(A; nev=6, ncv=max(20,2*nev+1), which=:LM, tol=0.0, maxiter=300, sigma=nothing, ritzvec=true, v0=[]) -> (d,[v,],nconv,niter,nmult,resid)


(Likewise here the Float64[] question.)

Sacha0 · 2018-01-13T04:25:30Z

stdlib/IterativeEigensolvers/src/IterativeEigensolvers.jl

@@ -57,15 +57,15 @@ function eigs(A::AbstractMatrix, B::AbstractMatrix; kwargs...)
    eigs(convert(AbstractMatrix{Tnew}, A), convert(AbstractMatrix{Tnew}, B); kwargs...)
 end
 """
-    eigs(A, B; nev=6, ncv=max(20,2*nev+1), which=:LM, tol=0.0, maxiter=300, sigma=nothing, ritzvec=true, v0=zeros((0,))) -> (d,[v,],nconv,niter,nmult,resid)
+    eigs(A, B; nev=6, ncv=max(20,2*nev+1), which=:LM, tol=0.0, maxiter=300, sigma=nothing, ritzvec=true, v0=[]) -> (d,[v,],nconv,niter,nmult,resid)


(Likewise here the Float64[] question.)

Sacha0 · 2018-01-13T04:26:04Z

stdlib/IterativeEigensolvers/src/IterativeEigensolvers.jl

@@ -246,7 +246,7 @@ function svds(A::AbstractMatrix{T}; kwargs...) where T
 end

 """
-    svds(A; nsv=6, ritzvec=true, tol=0.0, maxiter=1000, ncv=2*nsv, v0=zeros((0,))) -> (SVD([left_sv,] s, [right_sv,]), nconv, niter, nmult, resid)
+    svds(A; nsv=6, ritzvec=true, tol=0.0, maxiter=1000, ncv=2*nsv, v0=[]) -> (SVD([left_sv,] s, [right_sv,]), nconv, niter, nmult, resid)


(Likewise here the Float64[] question.)

Sacha0 · 2018-01-13T04:26:51Z

test/abstractarray.jl

-    @test_throws DimensionMismatch LinAlg.checksquare(zeros(2,3))
+    A2x2, A3x3, A2x3 = Matrix{Float64}.(uninitialized, ((2,2), (3,3), (2,3)))
+    @test LinAlg.checksquare(A2x2) == 2
+    @test LinAlg.checksquare(zA2x2, A3x3) == [2, 3]


zA2x2 -> A2x2? :)

Sacha0 · 2018-01-13T04:49:28Z

test/linalg/lapack.jl


-        @test_throws DimensionMismatch LAPACK.orghr!(1, 10, C, zeros(elty,11))
+        @test_throws DimensionMismatch LAPACK.orghr!(1, 10, A10x10, x11)
    end
 end


Beautifully done rewriting this section :).

Sacha0 · 2018-01-13T04:56:30Z

test/linalg/matmul.jl

@@ -15,12 +15,13 @@ using Base.LinAlg: mul!
            ((0,0), (0,4), (0,4)),
            ((3,0), (0,0), (3,0)),
            ((0,0), (0,0), (0,0)) )
-        @test Matrix{Float64}(uninitialized, dimsA) * Matrix{Float64}(uninitialized, dimsB) == zeros(dimsC)
+        A, B, C = Matrix{Float64}.(uninitialized, (dimsA, dimsB, dimsC))
+        @test A * B == C


Will the ((3,0), (0,4), (3,4)) case not fail?

Sacha0 · 2018-01-13T04:57:27Z

test/linalg/matmul.jl

+    A = Matrix{Float64}(uninitialized, 5, 0)
+    B = Matrix{ComplexF64}(uninitialized, 5, 0)
+    @test A'A  == B'B  == fill(0, (0,0))
+    @test A*A' == B*B' == fill(0, (5,5))


Nicely done :).

Sacha0 · 2018-01-13T05:02:14Z

test/linalg/symmetric.jl

-        b = eltya == Int ? rand(1:7, n, n) : convert(Matrix{eltya}, eltya <: Complex ? complex.(b, zeros(n,n)) : b)
+        x = eltya == Int ? rand(1:7, n) : copy!(Vector{eltya}(uninitialized, n), randn(n))
+        y = eltya == Int ? rand(1:7, n) : copy!(Vector{eltya}(uninitialized, n), randn(n))
+        b = eltya == Int ? rand(1:7, n, n) : copy!(Matrix{eltya}(uninitialized, n, n), randn(n,n)/2)


Would simpler convert(Vector{eltya}, randn(n))-like rewrites work? :)

Sacha0 · 2018-01-16T04:59:20Z

base/linalg/transpose.jl

@@ -20,7 +20,7 @@ julia> A = [3+2im 9+2im; 8+7im  4+6im]
 3+2im  9+2im
 8+7im  4+6im

-julia> B = zeros(Complex{Int64}, 2, 2)
+julia> B = [0+0im 0+0im; 0+0im 0+0im]


0im suffices? :)

Sacha0 · 2018-01-16T05:07:09Z

base/env.jl

@@ -13,7 +13,7 @@ if Sys.iswindows()
        if len == 0
            return Libc.GetLastError() != ERROR_ENVVAR_NOT_FOUND ? "" : onError(str)
        end
-        val = zeros(UInt16,len)
+        val = fill(zero(UInt16), len)


Slightly simpler: UInt16(0). (Edit: Likewise below with a number of different concrete types.)

Sacha0 · 2018-01-16T05:17:50Z

An appreciable fraction of the first commit's rewrites seem like strict improvements; merging those rewrites would be lovely independent of zeros remaining around.

Though more subjective, I also find most rewrites in commits three through nine improvements; merging some agreeable subset of those rewrites would be lovely independent of zeros remaining around as well.

Much thanks for your efforts on this front @fredrikekre! :)

vtjnash · 2021-05-29T04:16:14Z

Were you still interested in doing the changes suggested by Sacha0, or should we close this?

KristofferC · 2022-05-06T13:48:36Z

Since zeros was kept I think this can be closed.

fredrikekre mentioned this pull request Jan 4, 2018

rewrite all calls to ones(...) #25087

Merged

GunnarFarneback reviewed Jan 4, 2018

View reviewed changes

fredrikekre force-pushed the fe/rewrite-zeros branch from b96ccdf to 1a72f32 Compare January 5, 2018 00:38

replace zeros(...) calls where the value 0 has no special meaning and…

366289f

… something else is clearly better

fredrikekre force-pushed the fe/rewrite-zeros branch from 1a72f32 to c3513ca Compare January 11, 2018 16:17

fredrikekre added 9 commits January 11, 2018 17:19

rewrite zeros(dims...) to zeros(Float64, dims...)

f7714a1

rewrite zeros(...) with array literals where it makes sense

3143742

rewrite zeros(Int, dims) to fill(0, dims)

5ba4a3c

rewrite zeros(UInt8, dims) to fill(0x0, dims)

ff3d1bc

rewrite zeros(Float64, dims) to fill(0.0, dims)

9f50dbb

rewrite zeros(Complex{Float64}, dims) to fill(0.0+0im, dims)

091be61

rewrite zeros(Complex{Int}, dims) to fill(0+0im, dims)

d376d1f

rewrite zeros(Bool, dims) to fill(false, dims)

acf43b5

rewrite zeros(T, dims) to fill(zero(T), dims)

32bcb30

fredrikekre force-pushed the fe/rewrite-zeros branch from c3513ca to 32bcb30 Compare January 11, 2018 16:19

Sacha0 reviewed Jan 13, 2018

View reviewed changes

Sacha0 reviewed Jan 16, 2018

View reviewed changes

JeffBezanson mentioned this pull request Sep 13, 2018

zeros etc. is arguably broken for mutable element types #29168

Closed

KristofferC closed this May 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rewrite all zeros(...) calls #25392

rewrite all zeros(...) calls #25392

fredrikekre commented Jan 4, 2018

fredrikekre commented Jan 4, 2018

ararslan commented Jan 4, 2018

GunnarFarneback Jan 4, 2018

GunnarFarneback Jan 4, 2018

ggggggggg commented Jan 5, 2018

JeffBezanson commented Jan 5, 2018

fredrikekre commented Jan 11, 2018

Sacha0 Jan 13, 2018

Sacha0 Jan 13, 2018

Sacha0 Jan 13, 2018

Sacha0 Jan 13, 2018

Sacha0 Jan 13, 2018

Sacha0 Jan 13, 2018

Sacha0 Jan 13, 2018

Sacha0 Jan 13, 2018

Sacha0 Jan 13, 2018

Sacha0 Jan 13, 2018

Sacha0 Jan 16, 2018

Sacha0 Jan 16, 2018 •

edited

Loading

Sacha0 commented Jan 16, 2018

vtjnash commented May 29, 2021

KristofferC commented May 6, 2022

rewrite all zeros(...) calls #25392

rewrite all zeros(...) calls #25392

Conversation

fredrikekre commented Jan 4, 2018

fredrikekre commented Jan 4, 2018

ararslan commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggggggggg commented Jan 5, 2018

JeffBezanson commented Jan 5, 2018

fredrikekre commented Jan 11, 2018

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

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

Choose a reason for hiding this comment

Sacha0 Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Sacha0 commented Jan 16, 2018

vtjnash commented May 29, 2021

KristofferC commented May 6, 2022

Sacha0 Jan 16, 2018 •

edited

Loading