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

make setindex with colon fall back to fill! #16310

Merged
merged 1 commit into from
May 13, 2016

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented May 11, 2016

Update to #13459. Fixes #16302 for Arrays. @mbauman mentioned in #13459 (comment) that this could be extended to AbstractArray but that it has the extra complication of dealing with the vararg call.

For now, this at least improves the situation for Arrays as compared to the results in #16302.

julia> function foo(A)
           for i = 1:10^6
               A[:] = 0
           end
       end
foo (generic function with 1 method)

julia> @time foo([3,4,5])
  0.005799 seconds (5 allocations: 272 bytes)

cc @stevengj

@@ -383,6 +383,9 @@ function setindex!{T}(A::Array{T}, X::Array{T}, c::Colon)
return A
end

setindex!{T}(A::Array{T}, x::Number, c::Colon) = fill!(A, x)
Copy link
Member

@stevengj stevengj May 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the {T} is not necessary here. Note also that you don't need to name the c argument since it is never used.

What is the problem with just doing setindex!(A::AbstractArray, x::Number, ::Colon) = fill!(A, x)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or even more generally:

setindex!{T}(A::AbstractArray{T}, x::Union{Number,T}, ::Colon) = fill!(A, x)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defining it on setindex! directly for all AbstractArrays leads to nasty ambiguity issues when custom arrays want to define their own behaviors. They'll want to define a method with a more specific array type but less specific value and index types.

Interestingly, this is a new and exciting way for Base to break packages.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try out your suggestions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your suggested Union does not seem to do the intended overloading @stevengj :

julia> import Base.setindex!

julia> A = rand(3,3);

julia> @which A[:] = 0
setindex!(A::AbstractArray, v, I...) at abstractarray.jl:498

julia> setindex!{T}(A::Array{T}, x::Union{Number,T}, ::Colon) = fill!(A, x)
setindex! (generic function with 90 methods)

julia> @which A[:] = 0
setindex!(A::AbstractArray, v, I...) at abstractarray.jl:498

julia> setindex!{T}(A::Array{T}, x::Number, ::Colon) = fill!(A, x)
setindex! (generic function with 91 methods)

julia> @which A[:] = 0
setindex!{T}(A::Array{T,N<:Any}, x::Number, ::Colon) at REPL[8]:1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yeah, that's #3738 (unions with static type parameters are problematic). Julia can either match the second argument to the Number in the union, or it can bind T to Int and match the typevar. Julia chooses poorly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to allow all non-array values, you need two methods:

setindex!(A::Array, X::AbstractArray, ::Colon) = # manual loop
setindex!(A::Array, x, ::Colon) = fill!(A, x)

@kshyatt kshyatt added the performance Must go faster label May 11, 2016
@JeffBezanson JeffBezanson merged commit 10cc4a6 into JuliaLang:master May 13, 2016
@KristofferC KristofferC deleted the kc/fill_array branch May 13, 2016 14:25
@tkelman
Copy link
Contributor

tkelman commented May 13, 2016

Why was this decided against last time it came up?

@yuyichao
Copy link
Contributor

This was proposed as a fix to a worse issue which we had a better solution of. It wasn't clear that the splatting issue is this bad I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants