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

Consistently specialize one, zero on args of type Zeros, Ones, Eye #173

Closed
cossio opened this issue Mar 16, 2022 · 5 comments · Fixed by #184
Closed

Consistently specialize one, zero on args of type Zeros, Ones, Eye #173

cossio opened this issue Mar 16, 2022 · 5 comments · Fixed by #184

Comments

@cossio
Copy link

cossio commented Mar 16, 2022

These should be specialized to return FillArrays types:

julia> one(Ones(3,3)) # could return Eye(3)
3×3 Matrix{Float64}:
 1.0  0.0  0.0
 0.0  1.0  0.0
 0.0  0.0  1.0

julia> one(Zeros(3,3)) # could return Eye(3)
3×3 Matrix{Float64}:
 1.0  0.0  0.0
 0.0  1.0  0.0
 0.0  0.0  1.0

julia> one(Eye(3)) # could return Eye(3)
3×3 Diagonal{Float64, Vector{Float64}}:
 1.0        
     1.0    
         1.0

julia> zero(Eye(3)) # could return Zeros(3,3)
3×3 Diagonal{Float64, Vector{Float64}}:
 0.0        
     0.0    
         0.0

Compare with:

julia> zero(Zeros(3,3))
3×3 Zeros{Float64}

julia> zero(Ones(3,3))
3×3 Zeros{Float64}
@jishnub
Copy link
Member

jishnub commented Aug 31, 2022

On Julia v1.8, the zero case is already fixed:

julia> zero(Eye(3))
3×3 Diagonal{Float64, Fill{Float64, 1, Tuple{Base.OneTo{Int64}}}}:
 0.0        
     0.0    
         0.0

I'll try to work on a fix for one

@cossio
Copy link
Author

cossio commented Aug 31, 2022

Can zero(Eye(3)) use Ones instead of a generic Fill in your example?

@jishnub
Copy link
Member

jishnub commented Sep 1, 2022

Is there an advantage to using Ones/Zeros instead of a Fill?

@cossio
Copy link
Author

cossio commented Sep 1, 2022

I think Ones / Zeros have the value information in the type, whereas Fill can assume any scalar value. This can be used in some optimisations, for example multiplying by a Zeros matrix should give another Zeros matrix (thought I'm not sure now if that's done in practice).

@jishnub
Copy link
Member

jishnub commented Sep 1, 2022

I would have expected constant propagation to handle this, but some experimentation shows that you may be right. However, I'll hold off on this for now and wait for the opinions of others, as changing the return type may increase compilation times.

This is indeed not being constant-propagated:

julia> f = Fill(4,3,3)
3×3 Fill{Int64}, with entries equal to 4

julia> @code_warntype (x -> (s = zero(one(x)) * x; s[1]))(f)
MethodInstance for (::var"#1#2")(::Fill{Int64, 2, Tuple{Base.OneTo{Int64}, Base.OneTo{Int64}}})
  from (::var"#1#2")(x) in Main at REPL[3]:1
Arguments
  #self#::Core.Const(var"#1#2"())
  x::Fill{Int64, 2, Tuple{Base.OneTo{Int64}, Base.OneTo{Int64}}}
Locals
  s::Fill{Int64, 2, Tuple{Base.OneTo{Int64}, Base.OneTo{Int64}}}
Body::Int64
1%1 = Main.one(x)::LinearAlgebra.Diagonal{Int64, Ones{Int64, 1, Tuple{Base.OneTo{Int64}}}}
│   %2 = Main.zero(%1)::Core.PartialStruct(LinearAlgebra.Diagonal{Int64, Fill{Int64, 1, Tuple{Base.OneTo{Int64}}}}, Any[Core.PartialStruct(Fill{Int64, 1, Tuple{Base.OneTo{Int64}}}, Any[Core.Const(0), Tuple{Base.OneTo{Int64}}])])
│        (s = %2 * x)
│   %4 = Base.getindex(s, 1)::Int64
└──      return %4

although the result of zero(one(x)) is identified as having elements that are all zero.

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

Successfully merging a pull request may close this issue.

2 participants