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 broadcast treat all type arguments as scalars (fix DataArrays.jl#229) #19922

Merged
merged 1 commit into from
Jan 9, 2017

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Jan 7, 2017

Base.Broadcast.broadcast_containertype is incorrect for ::Type{T} arguments to upstream broadcast where T <: Union{Tuple, Ref, AbstractArray, Nullable}, e.g. Base.Broadcast.broadcast_containertype(AbstractArray) yields Array rather than Any. That incorrect return causes, e.g.,

julia> broadcast(==, [1], AbstractArray)
ERROR: MethodError: no method matching size(::Type{AbstractArray})
Closest candidates are:
  size{N}(::Any, ::Integer, ::Integer, ::Integer...) at abstractarray.jl:23
  size(::BitArray{1}) at bitarray.jl:49
  size(::BitArray{1}, ::Any) at bitarray.jl:53
  ...
Stacktrace:
 [1] broadcast_indices(::Type{Array}, ::Type{T}) at ./broadcast.jl:54
 [2] broadcast_c at ./broadcast.jl:290 [inlined]
 [3] broadcast(::Function, ::Array{Int64,1}, ::Type{T}) at ./broadcast.jl:406

This pull request addresses the above issue. Thanks to @ararslan for catching this issue in JuliaStats/DataArrays.jl#229!

@Sacha0 Sacha0 added kind:bugfix This change fixes an existing bug domain:broadcast Applying a function over a collection labels Jan 7, 2017
@@ -25,13 +25,14 @@ function broadcast!{T,S,N}(::typeof(identity), x::AbstractArray{T,N}, y::Abstrac
end

# logic for deciding the resulting container type
containertype(x) = containertype(typeof(x))
containertype(x) = _containertype(typeof(x))
containertype(::Type) = Any
Copy link
Contributor

@TotalVerb TotalVerb Jan 8, 2017

Choose a reason for hiding this comment

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

For clarity, this should perhaps be reordered so that all containertype methods are together, after all the _containertype methods.

It seems to me that the containertype(::Type) = Any method is redundant and can be removed, because _containertype(DataType) will already return Any.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call (and incorporated) on both counts. Thanks!

Copy link
Contributor

@TotalVerb TotalVerb left a comment

Choose a reason for hiding this comment

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

@andreasnoack
Copy link
Member

For ImageFiltering.jl, I think we can ping @timholy. I'll take care of DistributedArrays.jl.

@andreasnoack andreasnoack merged commit 7c34d69 into JuliaLang:master Jan 9, 2017
@timholy
Copy link
Sponsor Member

timholy commented Jan 9, 2017

I'll handle ImageFiltering.

@Sacha0 Sacha0 deleted the saferct branch January 9, 2017 18:04
@Sacha0
Copy link
Member Author

Sacha0 commented Jan 9, 2017

Thanks all!

Sacha0 added a commit to Sacha0/julia that referenced this pull request Jan 9, 2017
Sacha0 added a commit to Sacha0/julia that referenced this pull request Jan 9, 2017
Sacha0 added a commit to Sacha0/julia that referenced this pull request Jan 9, 2017
vtjnash added a commit that referenced this pull request Jan 10, 2017
Fix simultaneous merge issue between #19724 and #19922
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:broadcast Applying a function over a collection kind:bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants