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.

Sign up for GitHub

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

Use containertype to determine array type for array broadcast #19745

Merged
merged 1 commit into from
Dec 30, 2016
+35 −13
Merged

Use containertype to determine array type for array broadcast #19745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 1 addition & 12 deletions base/broadcast.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,9 @@ export bitbroadcast, dotview
export broadcast_getindex, broadcast_setindex!

## Broadcasting utilities ##

broadcast_array_type() = Array
broadcast_array_type(A, As...) =
if is_nullable_array(A) || broadcast_array_type(As...) === Array{Nullable}
Array{Nullable}
else
Array
end

# fallbacks for some special cases
@inline broadcast(f, x::Number...) = f(x...)
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 think we can avoid defining these if we delete the method in line 26 and just use this method which was the origin purpose of this PR.

Copy link
Contributor

@pabloferz pabloferz Dec 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, you can remove this, it should do no harm. But I believe the ## Broadcasting utilities ## is still useful for organizing the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, this was an annoying special case I had to add. Thanks for addressing it!

@inline broadcast{N}(f, t::NTuple{N}, ts::Vararg{NTuple{N}}) = map(f, t, ts...)
@inline broadcast(f, As::AbstractArray...) =
broadcast_c(f, broadcast_array_type(As...), As...)

# special cases for "X .= ..." (broadcast!) assignments
broadcast!(::typeof(identity), X::AbstractArray, x::Number) = fill!(X, x)
Expand Down Expand Up @@ -313,7 +302,7 @@ ziptype{T}(::Type{T}, A) = typestuple(T, A)
ziptype{T}(::Type{T}, A, B) = (Base.@_pure_meta; Iterators.Zip2{typestuple(T, A), typestuple(T, B)})
@inline ziptype{T}(::Type{T}, A, B, C, D...) = Iterators.Zip{typestuple(T, A), ziptype(T, B, C, D...)}

_broadcast_type{S}(::Type{S}, f, T::Type, As...) = Base._return_type(S, typestuple(S, T, As...))
_broadcast_type{S}(::Type{S}, f, T::Type, As...) = Base._return_type(f, typestuple(S, T, As...))
Copy link
Member Author

Choose a reason for hiding this comment

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

@TotalVerb I'm not sure what the purpose of the first type argument is but it seemed wrong to pass it as the first argument to _return_type and it also caused a test failure in the test at line 366 below. Does this change seem right to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

The first argument is used to differentiate when Nullable should be treated as scalar (when S === Any) or as container (when S === Array), but should be passed as an argument only for typestuple. This was certainly a mistake.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this was a mistake.

_broadcast_type{T}(::Type{T}, f, A, Bs...) = Base._default_eltype(Base.Generator{ziptype(T, A, Bs...), ftype(f, A, Bs...)})

# broadcast methods that dispatch on the type of the final container
Expand Down
35 changes: 34 additions & 1 deletion test/broadcast.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ StrangeType18623(x,y) = (x,y)
let
f(A, n) = broadcast(x -> +(x, n), A)
@test @inferred(f([1.0], 1)) == [2.0]
g() = (a = 1; Base.Broadcast._broadcast_type(x -> x + a, 1.0))
g() = (a = 1; Base.Broadcast._broadcast_type(Any, x -> x + a, 1.0))
@test @inferred(g()) === Float64
end

Expand All @@ -376,3 +376,36 @@ end

# Check that broadcast!(f, A) populates A via independent calls to f (#12277, #19722).
@test let z = 1; A = broadcast!(() -> z += 1, zeros(2)); A[1] != A[2]; end

# broadcasting for custom AbstractArray
immutable Array19745{T,N} <: AbstractArray{T,N}
data::Array{T,N}
end
Base.getindex(A::Array19745, i::Integer...) = A.data[i...]
Base.size(A::Array19745) = size(A.data)

Base.Broadcast.containertype{T<:Array19745}(::Type{T}) = Array19745

Base.Broadcast.promote_containertype(::Type{Array19745}, ::Type{Array19745}) = Array19745
Base.Broadcast.promote_containertype(::Type{Array19745}, ::Type{Array}) = Array19745
Base.Broadcast.promote_containertype(::Type{Array19745}, ct) = Array19745
Base.Broadcast.promote_containertype(::Type{Array}, ::Type{Array19745}) = Array19745
Base.Broadcast.promote_containertype(ct, ::Type{Array19745}) = Array19745

Base.Broadcast.broadcast_indices(::Type{Array19745}, A) = indices(A)
Base.Broadcast.broadcast_indices(::Type{Array19745}, A::Ref) = ()

getfield19745(x::Array19745) = x.data
getfield19745(x) = x

Base.Broadcast.broadcast_c(f, ::Type{Array19745}, A, Bs...) =
Array19745(Base.Broadcast.broadcast_c(f, Array, getfield19745(A), map(getfield19745, Bs)...))

@testset "broadcasting for custom AbstractArray" begin
a = randn(10)
aa = Array19745(a)
@test a .+ 1 == @inferred(aa .+ 1)
@test a .* a' == @inferred(aa .* aa')
@test isa(aa .+ 1, Array19745)
@test isa(aa .* aa', Array19745)
end