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

Simplify and extend broadcast eltype promotion mechanism #19723

Merged
merged 1 commit into from
Jan 1, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
22 changes: 6 additions & 16 deletions base/broadcast.jl
Original file line number Diff line number Diff line change
Expand Up @@ -272,24 +272,14 @@ end
@inline broadcast_elwise_op(f, As...) =
broadcast!(f, similar(Array{promote_eltype_op(f, As...)}, broadcast_indices(As...)), As...)

ftype(f, A) = typeof(f)
ftype(f, A...) = typeof(a -> f(a...))
ftype(T::Type, A...) = Type{T}

typestuple(a) = (Base.@_pure_meta; Tuple{eltype(a)})
typestuple(T::Type) = (Base.@_pure_meta; Tuple{Type{T}})
typestuple(a, b...) = (Base.@_pure_meta; Tuple{typestuple(a).types..., typestuple(b...).types...})

ziptype(A) = typestuple(A)
ziptype(A, B) = (Base.@_pure_meta; Iterators.Zip2{typestuple(A), typestuple(B)})
@inline ziptype(A, B, C, D...) = Iterators.Zip{typestuple(A), ziptype(B, C, D...)}

_broadcast_type(f, T::Type, As...) = Base._return_type(f, typestuple(T, As...))
_broadcast_type(f, A, Bs...) = Base._default_eltype(Base.Generator{ziptype(A, Bs...), ftype(f, A, Bs...)})
eltypestuple(a) = (Base.@_pure_meta; Tuple{eltype(a)})
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of moving these either to abstractarray.jl or promotion.jl?

Copy link
Member Author

Choose a reason for hiding this comment

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

Moving eltypestuple to abstractarray.jl or promotion.jl? _broadcast_eltype being the only consumer of eltypestuple, wherever _broadcast_eltype lives seems like the appropriate location? Or do you have other (broader) uses in mind for eltypesuple? Or did you mean something else altogether? Thanks for reviewing! :)

Copy link
Contributor

@pabloferz pabloferz Dec 30, 2016

Choose a reason for hiding this comment

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

Sorry for commenting above without providing a context. I was thinking that we could change the promote_op methods to take a function and the tuple of the eltypes and remove the promote_op(Any...) = Any, so it would make sense having eltypestuple somewhere in Base. But that might fit better in another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds like a good plan! Agreed another PR might be better. Perhaps we can chat about cleaning up these mechanisms after feature freeze? Thanks again!

eltypestuple(T::Type) = (Base.@_pure_meta; Tuple{Type{T}})
eltypestuple(a, b...) = (Base.@_pure_meta; Tuple{eltypestuple(a).types..., eltypestuple(b...).types...})
_broadcast_eltype(f, A, Bs...) = Base._return_type(f, eltypestuple(A, Bs...))

# broadcast methods that dispatch on the type of the final container
@inline function broadcast_c(f, ::Type{Array}, A, Bs...)
T = _broadcast_type(f, A, Bs...)
T = _broadcast_eltype(f, A, Bs...)
shape = broadcast_indices(A, Bs...)
iter = CartesianRange(shape)
if isleaftype(T)
Expand All @@ -307,7 +297,7 @@ function broadcast_c(f, ::Type{Tuple}, As...)
end
@inline function broadcast_c(f, ::Type{Nullable}, a...)
nonnull = all(hasvalue, a)
S = _broadcast_type(f, a...)
S = _broadcast_eltype(f, a...)
if isleaftype(S) && null_safe_eltype_op(f, a...)
Nullable{S}(f(map(unsafe_get, a)...), nonnull)
else
Expand Down
6 changes: 2 additions & 4 deletions base/promotion.jl
Original file line number Diff line number Diff line change
Expand Up @@ -231,15 +231,13 @@ end
promote_op(::Any...) = (@_pure_meta; Any)
function promote_op{S}(f, ::Type{S})
@_inline_meta
Z = Tuple{_default_type(S)}
T = _default_eltype(Generator{Z, typeof(f)})
T = _return_type(f, Tuple{_default_type(S)})
isleaftype(S) && return isleaftype(T) ? T : Any
return typejoin(S, T)
end
function promote_op{R,S}(f, ::Type{R}, ::Type{S})
@_inline_meta
Z = Iterators.Zip2{Tuple{_default_type(R)}, Tuple{_default_type(S)}}
T = _default_eltype(Generator{Z, typeof(a -> f(a...))})
T = _return_type(f, Tuple{_default_type(R), _default_type(S)})
isleaftype(R) && isleaftype(S) && return isleaftype(T) ? T : Any
return typejoin(R, S, T)
end
Expand Down
4 changes: 2 additions & 2 deletions base/sparse/higherorderfns.jl
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ function _noshapecheck_map{Tf,N}(f::Tf, A::SparseVecOrMat, Bs::Vararg{SparseVecO
fofzeros = f(_zeros_eltypes(A, Bs...)...)
fpreszeros = _iszero(fofzeros)
maxnnzC = fpreszeros ? min(length(A), _sumnnzs(A, Bs...)) : length(A)
entrytypeC = Base.Broadcast._broadcast_type(f, A, Bs...)
entrytypeC = Base.Broadcast._broadcast_eltype(f, A, Bs...)
indextypeC = _promote_indtype(A, Bs...)
C = _allocres(size(A), indextypeC, entrytypeC, maxnnzC)
return fpreszeros ? _map_zeropres!(f, C, A, Bs...) :
Expand Down Expand Up @@ -101,7 +101,7 @@ function _diffshape_broadcast{Tf,N}(f::Tf, A::SparseVecOrMat, Bs::Vararg{SparseV
fofzeros = f(_zeros_eltypes(A, Bs...)...)
fpreszeros = _iszero(fofzeros)
indextypeC = _promote_indtype(A, Bs...)
entrytypeC = Base.Broadcast._broadcast_type(f, A, Bs...)
entrytypeC = Base.Broadcast._broadcast_eltype(f, A, Bs...)
shapeC = to_shape(Base.Broadcast.broadcast_indices(A, Bs...))
maxnnzC = fpreszeros ? _checked_maxnnzbcres(shapeC, A, Bs...) : _densennz(shapeC)
C = _allocres(shapeC, indextypeC, entrytypeC, maxnnzC)
Expand Down
8 changes: 7 additions & 1 deletion test/broadcast.jl
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_eltype(x -> x + a, 1.0))
@test @inferred(g()) === Float64
end

Expand Down Expand Up @@ -418,3 +418,9 @@ let io = IOBuffer()
broadcast(x -> print(io, x), [Nullable(1.0)])
@test String(take!(io)) == "Nullable{Float64}(1.0)"
end

# Test that broadcast's promotion mechanism handles closures accepting more than one argument.
# (See issue #19641 and referenced issues and pull requests.)
let f() = (a = 1; Base.Broadcast._broadcast_eltype((x, y) -> x + y + a, 1.0, 1.0))
@test @inferred(f()) == Float64
end