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

Jump to bottom

Limit broadcast mechanism over Nullables #19787

Merged
merged 5 commits into from
Jan 1, 2017
Merged

Limit broadcast mechanism over Nullables #19787

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e0f1cb1
Limit broadcast mechanism over Nullables
pabloferz Dec 30, 2016
e5c270a
Test that broadcast doesn't propagate too much
pabloferz Dec 31, 2016
f3aec3b
[ci skip] Update manual section on Nullables
pabloferz Dec 31, 2016
dcba165
Address @TotalVerb comments
pabloferz Dec 31, 2016
afdd247
Address @Sacha0 comments
pabloferz Dec 31, 2016
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
10 changes: 4 additions & 6 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,10 @@ This section lists changes that do not have deprecation warnings.

* `broadcast` now treats `Ref` (except for `Ptr`) arguments as 0-dimensional
arrays ([#18965]).

* `broadcast` now handles missing data (`Nullable`s) allowing operations to
be lifted over `Nullable`s, as if the `Nullable` were like an array with
zero or one element. ([#16961]). Note that many situations where `Nullable`
types had been treated like scalars before will no longer work. For
example, `get.(xs)` on `xs::Array{T <: Nullable}` will now treat the
nullables as a container, and attempt to operate on the data contained.
This use case will need to be migrated to `map(get, xs)`.
be lifted over mixtures of `Nullable`s and scalars, as if the `Nullable`
were like an array with zero or one element. ([#16961], [#19787]).

* The runtime now enforces when new method definitions can take effect ([#17057]).
The flip-side of this is that new method definitions should now reliably actually
Expand Down Expand Up @@ -804,3 +801,4 @@ Language tooling improvements
[#19543]: https://github.com/JuliaLang/julia/issues/19543
[#19598]: https://github.com/JuliaLang/julia/issues/19598
[#19635]: https://github.com/JuliaLang/julia/issues/19635
[#19787]: https://github.com/JuliaLang/julia/issues/19787
94 changes: 25 additions & 69 deletions base/broadcast.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@ module Broadcast
using Base.Cartesian
using Base: promote_eltype_op, linearindices, tail, OneTo, to_shape,
_msk_end, unsafe_bitgetindex, bitcache_chunks, bitcache_size, dumpbitcache,
nullable_returntype, null_safe_eltype_op, hasvalue, is_nullable_array
nullable_returntype, null_safe_eltype_op, hasvalue
import Base: broadcast, broadcast!
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the is_nullable_array import can be removed, and probably the function itself too (in base/nullable.jl).

export broadcast_getindex, broadcast_setindex!, dotview

typealias ScalarType Union{Type{Any}, Type{Nullable}}

## Broadcasting utilities ##
# fallbacks for some special cases
@inline broadcast(f, x::Number...) = f(x...)
Expand All @@ -28,37 +30,28 @@ containertype(::Type) = Any
containertype{T<:Ptr}(::Type{T}) = Any
containertype{T<:Tuple}(::Type{T}) = Tuple
containertype{T<:Ref}(::Type{T}) = Array
containertype{T<:AbstractArray}(::Type{T}) =
is_nullable_array(T) ? Array{Nullable} : Array
containertype{T<:AbstractArray}(::Type{T}) = Array
containertype{T<:Nullable}(::Type{T}) = Nullable
containertype(ct1, ct2) = promote_containertype(containertype(ct1), containertype(ct2))
@inline containertype(ct1, ct2, cts...) = promote_containertype(containertype(ct1), containertype(ct2, cts...))

promote_containertype(::Type{Array}, ::Type{Array}) = Array
promote_containertype(::Type{Array}, ct) = Array
promote_containertype(ct, ::Type{Array}) = Array
promote_containertype(::Type{Tuple}, ::Type{Any}) = Tuple
promote_containertype(::Type{Any}, ::Type{Tuple}) = Tuple
promote_containertype(::Type{Tuple}, ::ScalarType) = Tuple
promote_containertype(::ScalarType, ::Type{Tuple}) = Tuple
promote_containertype(::Type{Any}, ::Type{Nullable}) = Nullable
promote_containertype(::Type{Nullable}, ::Type{Any}) = Nullable
promote_containertype(::Type{Nullable}, ::Type{Array}) = Array{Nullable}
promote_containertype(::Type{Array}, ::Type{Nullable}) = Array{Nullable}
promote_containertype(::Type{Array{Nullable}}, ::Type{Array{Nullable}}) =
Array{Nullable}
promote_containertype(::Type{Array{Nullable}}, ::Type{Array}) = Array{Nullable}
promote_containertype(::Type{Array}, ::Type{Array{Nullable}}) = Array{Nullable}
promote_containertype(::Type{Array{Nullable}}, ct) = Array{Nullable}
promote_containertype(ct, ::Type{Array{Nullable}}) = Array{Nullable}
promote_containertype{T}(::Type{T}, ::Type{T}) = T

## Calculate the broadcast indices of the arguments, or error if incompatible
# array inputs
broadcast_indices() = ()
broadcast_indices(A) = broadcast_indices(containertype(A), A)
broadcast_indices(::Union{Type{Any}, Type{Nullable}}, A) = ()
broadcast_indices(::ScalarType, A) = ()
broadcast_indices(::Type{Tuple}, A) = (OneTo(length(A)),)
broadcast_indices(::Type{Array}, A::Ref) = ()
broadcast_indices{T<:Array}(::Type{T}, A) = indices(A)
broadcast_indices(::Type{Array}, A) = indices(A)
@inline broadcast_indices(A, B...) = broadcast_shape((), broadcast_indices(A), map(broadcast_indices, B)...)
# shape (i.e., tuple-of-indices) inputs
broadcast_shape(shape::Tuple) = shape
Expand Down Expand Up @@ -132,9 +125,7 @@ end

Base.@propagate_inbounds _broadcast_getindex(A, I) = _broadcast_getindex(containertype(A), A, I)
Base.@propagate_inbounds _broadcast_getindex(::Type{Array}, A::Ref, I) = A[]
Base.@propagate_inbounds _broadcast_getindex(::Type{Any}, A, I) = A
Base.@propagate_inbounds _broadcast_getindex(::Union{Type{Any},
Type{Nullable}}, A, I) = A
Base.@propagate_inbounds _broadcast_getindex(::ScalarType, A, I) = A
Base.@propagate_inbounds _broadcast_getindex(::Any, A, I) = A[I]

## Broadcasting core
Expand Down Expand Up @@ -285,28 +276,20 @@ ftype(f, A) = typeof(f)
ftype(f, A...) = typeof(a -> f(a...))
ftype(T::Type, A...) = Type{T}

# nullables need to be treated like scalars sometimes and like containers
# other times, so there are two variants of typestuple.

# if the first argument is Any, then Nullable should be treated like a
# scalar; if the first argument is Array, then Nullable should be treated
# like a container.
typestuple(::Type, a) = (Base.@_pure_meta; Tuple{eltype(a)})
typestuple(::Type{Any}, a::Nullable) = (Base.@_pure_meta; Tuple{typeof(a)})
typestuple(::Type, T::Type) = (Base.@_pure_meta; Tuple{Type{T}})
typestuple{T}(::Type{T}, a, b...) = (Base.@_pure_meta; Tuple{typestuple(T, a).types..., typestuple(T, b...).types...})
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...})

# these functions take the variant of typestuple to be used as first argument
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...)}
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{S}(::Type{S}, f, T::Type, As...) = Base._return_type(f, typestuple(S, T, As...))
_broadcast_type{T}(::Type{T}, f, A, Bs...) = Base._default_eltype(Base.Generator{ziptype(T, A, Bs...), ftype(f, A, Bs...)})
_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...)})
Copy link
Contributor

@tkelman tkelman Dec 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

this is used a couple of times in the new sparse code that I suspect you'll have to change done


# broadcast methods that dispatch on the type of the final container
@inline function broadcast_c(f, ::Type{Array}, A, Bs...)
T = _broadcast_type(Any, f, A, Bs...)
T = _broadcast_type(f, A, Bs...)
shape = broadcast_indices(A, Bs...)
iter = CartesianRange(shape)
if isleaftype(T)
Expand All @@ -317,21 +300,14 @@ _broadcast_type{T}(::Type{T}, f, A, Bs...) = Base._default_eltype(Base.Generator
end
return broadcast_t(f, Any, shape, iter, A, Bs...)
end
@inline function broadcast_c(f, ::Type{Array{Nullable}}, A, Bs...)
@inline rec(x) = broadcast(f, x)
@inline rec(x, y) = broadcast(f, x, y)
@inline rec(x, y, z) = broadcast(f, x, y, z)
@inline rec(xs...) = broadcast(f, xs...)
broadcast_c(rec, Array, A, Bs...)
end
function broadcast_c(f, ::Type{Tuple}, As...)
shape = broadcast_indices(As...)
n = length(shape[1])
return ntuple(k->f((_broadcast_getindex(A, k) for A in As)...), n)
end
@inline function broadcast_c(f, ::Type{Nullable}, a...)
nonnull = all(hasvalue, a)
S = _broadcast_type(Array, f, a...)
S = _broadcast_type(f, a...)
if isleaftype(S) && null_safe_eltype_op(f, a...)
Nullable{S}(f(map(unsafe_get, a)...), nonnull)
else
Expand All @@ -349,7 +325,7 @@ end

Broadcasts the arrays, tuples, `Ref`, nullables, and/or scalars `As` to a
Copy link
Member

Choose a reason for hiding this comment

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

Not from this pull request, but should `Ref` be plural `Ref`s for consistency?

container of the appropriate type and dimensions. In this context, anything
that is not a subtype of `AbstractArray`, `Ref` (except for `Ptr`s) or `Tuple`,
that is not a subtype of `AbstractArray`, `Ref` (except for `Ptr`s), `Tuple`
Copy link
Member

Choose a reason for hiding this comment

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

(Serial/Oxford) comma after Tuple?

or `Nullable` is considered a scalar. The resulting container is established by
the following rules:

Expand All @@ -359,16 +335,9 @@ the following rules:
(and treats any `Ref` as a 0-dimensional array of its contents and any tuple
as a 1-dimensional array) expanding singleton dimensions.
Copy link
Member

@Sacha0 Sacha0 Dec 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Not from this pull request, but perhaps "If there is at least an array or a Ref in the arguments" -> "If there is at least one array or Ref in the arguments"? Similarly, perhaps "and treats any Ref as a 0-dimensional array of its contents and any tuple as a 1-dimensional array" -> "treating Refs as 0-dimensional arrays, and tuples as 1-dimensional arrays"?

Edit: Perhaps "If the arguments contain at least one array or Ref, it returns an array, treating Refs as 0-dimensional arrays, tuples as 1-dimensional arrays, and expanding singleton dimensions." would be better still?


The following additional rules apply to `Nullable` arguments:

- If there is at least a `Nullable`, and all the arguments are scalars or
`Nullable`, it returns a `Nullable`.
- If there is at least an array or a `Ref` with `Nullable` entries, or there
is at least an array or a `Ref` (perhaps with scalar entries instead of
`Nullable` entries) and a nullable, then the result is an array of
`Nullable` entries.
- If there is a tuple and a nullable, the result is an error, as this case is
not currently supported.
The following additional rule applies to `Nullable` arguments: If there is at
least a `Nullable`, and all the arguments are scalars or `Nullable`, it returns
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps "at least a" should be "at least one"?

a `Nullable` treating `Nullable`s as "containers".

A special syntax exists for broadcasting: `f.(args...)` is equivalent to
`broadcast(f, args...)`, and nested `f.(g.(args...))` calls are fused into a
Expand Down Expand Up @@ -433,21 +402,8 @@ Nullable{String}("XY")
julia> broadcast(/, 1.0, Nullable(2.0))
Nullable{Float64}(0.5)

julia> [Nullable(1), Nullable(2), Nullable()] .* 3
3-element Array{Nullable{Int64},1}:
3
6
#NULL

julia> [1+im, 2+2im, 3+3im] ./ Nullable{Int}()
3-element Array{Nullable{Complex{Float64}},1}:
#NULL
#NULL
#NULL

julia> Ref(7) .+ Nullable(3)
0-dimensional Array{Nullable{Int64},0}:
10
julia> (1 + im) ./ Nullable{Int}()
Nullable{Complex{Float64}}()
```
"""
@inline broadcast(f, A, Bs...) = broadcast_c(f, containertype(A, Bs...), A, Bs...)
Expand Down
4 changes: 0 additions & 4 deletions base/nullable.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,10 +302,6 @@ hasvalue(x) = true
all(f::typeof(hasvalue), t::Tuple) = f(t[1]) & all(f, tail(t))
all(f::typeof(hasvalue), t::Tuple{}) = true

is_nullable_array(::Any) = false
is_nullable_array{T}(::Type{T}) = eltype(T) <: Nullable
is_nullable_array(A::AbstractArray) = eltype(A) <: Nullable

# Overloads of null_safe_op
# Unary operators

Expand Down
4 changes: 2 additions & 2 deletions base/sparse/higherorderfns.jl
View file
Open in desktop
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 = fofzeros == zero(fofzeros)
maxnnzC = fpreszeros ? min(length(A), _sumnnzs(A, Bs...)) : length(A)
entrytypeC = Base.Broadcast._broadcast_type(Any, f, A, Bs...)
entrytypeC = Base.Broadcast._broadcast_type(f, A, Bs...)
Copy link
Contributor

Choose a reason for hiding this comment

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

(should be squashed on merge to avoid broken intermediate commits)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already squashed it to prevent that.

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 = fofzeros == zero(fofzeros)
indextypeC = _promote_indtype(A, Bs...)
entrytypeC = Base.Broadcast._broadcast_type(Any, f, A, Bs...)
entrytypeC = Base.Broadcast._broadcast_type(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
12 changes: 0 additions & 12 deletions doc/src/manual/types.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1426,15 +1426,3 @@ conveniently using `.`-prefixed operators:
julia> Nullable(2) ./ Nullable(3) .+ Nullable(1.0)
Nullable{Float64}(1.66667)
```

[`broadcast()`](@ref) also allows one to work with multiple data at the same
time, without manually writing for loops. This enables performing the same
operation to arrays where the data is possibly missing; for example

```julia
julia> [Nullable(2), Nullable(), Nullable(3)] .+ 3
3-element Array{Nullable{Int64},1}:
5
#NULL
6
```
11 changes: 10 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(Any, x -> x + a, 1.0))
g() = (a = 1; Base.Broadcast._broadcast_type(x -> x + a, 1.0))
@test @inferred(g()) === Float64
end

Expand Down Expand Up @@ -409,3 +409,12 @@ Base.Broadcast.broadcast_c(f, ::Type{Array19745}, A, Bs...) =
@test isa(aa .+ 1, Array19745)
@test isa(aa .* aa', Array19745)
end

# broadcast should only "peel off" one container layer
Copy link
Member

Choose a reason for hiding this comment

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

A simpler way to test this behavior might be nice if you can think of one?

Copy link
Contributor

Choose a reason for hiding this comment

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

get.([Nullable(1), Nullable(2)]) == [1, 2] will work as a simpler test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately I don't have much time for looking for better/simpler tests for today. Maybe we could revisit them another occasion?

Copy link
Member

Choose a reason for hiding this comment

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

If @TotalVerb's suggestion does not suffice, sounds good on this end. Best!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should work fine. Test added.

let io = IOBuffer()
broadcast(x -> print(io, x), [Nullable(1.0)])
@test String(take!(io)) == "Nullable{Float64}(1.0)"
Copy link
Member

Choose a reason for hiding this comment

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

Sounds a bit weird to use string comparison to test the type of the result. Why not e.g. call push! to add values to a vector instead?

v = []
broadcast(x -> push!(v, x), [Nullable(1)])
@test get.(v) == get.([Nullable(1)])
end
47 changes: 0 additions & 47 deletions test/nullable.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,48 +507,6 @@ end
@test Nullable(10.5) ===
@inferred(broadcast(+, 1, 2, Nullable(3), Nullable(4.0), Nullable(1//2)))

# broadcasting for arrays
@test istypeequal(@inferred(broadcast(+, [1, 2, 3], Nullable{Int}(1))),
Nullable{Int}[2, 3, 4])
@test istypeequal(@inferred(broadcast(+, Nullable{Int}[1, 2, 3], 1)),
Nullable{Int}[2, 3, 4])
@test istypeequal(@inferred(broadcast(+, Nullable{Int}[1, 2, 3], Nullable(1))),
Nullable{Int}[2, 3, 4])
@test istypeequal(@inferred(broadcast(+, Nullable{Int}[1, Nullable()], Nullable(1))),
Nullable{Int}[2, Nullable()])
@test istypeequal(@inferred(broadcast(+, Nullable{Int}[Nullable(), 1],
Nullable{Int}())),
Nullable{Int}[Nullable(), Nullable()])
@test istypeequal(@inferred(broadcast(+, Nullable{Int}[Nullable(), 1],
Nullable{Int}[1, Nullable()])),
Nullable{Int}[Nullable(), Nullable()])
@test istypeequal(@inferred(broadcast(+, Nullable{Int}[Nullable(), 1],
Nullable{Int}[Nullable(), 1])),
Nullable{Int}[Nullable(), 2])
@test istypeequal(@inferred(broadcast(+, Nullable{Int}[Nullable(), Nullable()],
Nullable{Int}[1, 2])),
Nullable{Int}[Nullable(), Nullable()])
@test istypeequal(@inferred(broadcast(+, Nullable{Int}[Nullable(), 1],
Nullable{Int}[1])),
Nullable{Int}[Nullable(), 2])
@test istypeequal(@inferred(broadcast(+, Nullable{Float64}[1.0, 2.0],
Nullable{Float64}[1.0 2.0; 3.0 4.0])),
Nullable{Float64}[2.0 3.0; 5.0 6.0])
@test istypeequal(@inferred(broadcast(+, Nullable{Int}[1, 2], [1, 2], 1)),
Nullable{Int}[3, 5])

@test istypeequal(@inferred(broadcast(/, 1, Nullable{Int}[1, 2, 4])),
Nullable{Float64}[1.0, 0.5, 0.25])
@test istypeequal(@inferred(broadcast(muladd, Nullable(2), 42,
[Nullable(1337), Nullable{Int}()])),
Nullable{Int}[1421, Nullable()])

# heterogenous types (not inferrable)
@test istypeequal(broadcast(+, Any[1, 1.0], Nullable(1//2)),
Any[Nullable(3//2), Nullable(1.5)])
@test istypeequal(broadcast(+, Any[Nullable(1) Nullable(1.0)], Nullable(big"1")),
Any[Nullable(big"2") Nullable(big"2.0")])

# test fast path taken
for op in (+, *, -)
for b1 in (false, true)
Expand All @@ -557,11 +515,6 @@ for op in (+, *, -)
@inferred(broadcast(op, Nullable{Int}(1, b1),
Nullable{Int}(2, b2)))
end
A = [1, 2, 3]
res = @inferred(broadcast(op, A, Nullable{Int}(1, b1)))
@test res[1] === Nullable{Int}(op(1, 1), b1)
@test res[2] === Nullable{Int}(op(2, 1), b1)
@test res[3] === Nullable{Int}(op(3, 1), b1)
end
end

Expand Down