Poor inference of [1, 2] + [3.0, missing]`
#28382
Comments
|
Inference can be improved, but there's no general solution to the second problem. Mutating an array just seems to be fundamentally incompatible with using functional constructs that pick an element type for you. |
JeffBezanson
added
broadcast
Yeah... I've been almost wondering if the outputs of such functions might even be better off marked immutable... |
I'm still not 100% convinced this is an inference issue per se, rather an issue of the container type chosen by |
|
The way the element type is chosen cannot be changed before 2.0, but inference should certainly be fixed to avoid killing performance of all code using the resulting array without a function barrier. The problem seems to happen with |
|
I mean, the choice of container eltype all stems from how we've chosen to do comprehensions: #7258 — the interesting discussion is on the linked Google Groups topic. This is the same behavior. |
|
Yes, I don't think we should discuss this behavior here. What needs fixing is just inference. |
|
Here's an option that initially looked promising: diff --git a/base/broadcast.jl b/base/broadcast.jl
index 4d6629a02e..3230968463 100644
--- a/base/broadcast.jl
+++ b/base/broadcast.jl
@@ -759,7 +759,12 @@ const NonleafHandlingStyles = Union{DefaultArrayStyle,ArrayConflict}
dest = similar(bc′, typeof(val))
@inbounds dest[I] = val
# Now handle the remaining values
- return copyto_nonleaf!(dest, bc′, iter, state, 1)
+ if dest isa AbstractArray
+ # Give inference a helping hand on the element type and dimensionality (work-around for #28382)
+ return copyto_nonleaf!(dest, bc′, iter, state, 1)::AbstractArray{<:ElType, ndims(dest)}
+ else
+ return copyto_nonleaf!(dest, bc′, iter, state, 1)
+ end
endjulia> @eval Base.Broadcast @inline function copy(bc::Broadcasted{Style}) where {Style}
ElType = combine_eltypes(bc.f, bc.args)
if Base.isconcretetype(ElType)
# We can trust it and defer to the simpler `copyto!`
return copyto!(similar(bc, ElType), bc)
end
# When ElType is not concrete, use narrowing. Use the first output
# value to determine the starting output eltype; copyto_nonleaf!
# will widen `dest` as needed to accommodate later values.
bc′ = preprocess(nothing, bc)
iter = eachindex(bc′)
y = iterate(iter)
if y === nothing
# if empty, take the ElType at face value
return similar(bc′, ElType)
end
# Initialize using the first value
I, state = y
@inbounds val = bc′[I]
dest = similar(bc′, typeof(val))
@inbounds dest[I] = val
# Now handle the remaining values
if dest isa AbstractArray
# Give inference a helping hand on the element type and dimensionality (work-around for #28382)
return copyto_nonleaf!(dest, bc′, iter, state, 1)::AbstractArray{<:ElType, ndims(dest)}
else
return copyto_nonleaf!(dest, bc′, iter, state, 1)
end
end
copy (generic function with 89 methods)
julia> @code_warntype [1, 2] + [3.0, missing]
Body::Array{T,1} where T<:Union{Missing, Float64}Unfortunately it fails in cases like julia> broadcast((x,y)->(x==1 ? 1.0 : x, y), [1 2 3], ["a", "b", "c"])
ERROR: TypeError: in typeassert, expected AbstractArray{#s4,2} where #s4<:Tuple{Union{Float64, Int64},String}, got Array{Tuple{Real,String},2} |
|
Thanks @mbauman. I had been considering the same temporary fix. Maybe we can fix the problem you highlight by calling |
|
I've made an attempt at implementing this strategy at #30485. |
I have been trying to get into using
missingand fastUniontypes in my worflow and seem to be coming up with some issues. It's most easily highlighted by:As you can see, the resultant output is (correctly but imprecisely) inferred to be
Array(not evenVector?).I find this example interesting for two reasons
Regarding performance, I find it quite usual to combine combinations of "functional" operations like
+,map, etc, together with hand-written loops in the same scope. In fact, predictably good performance over loops, recursion, higher-order programming, etc is kind of one of our main selling points. In the above, performance will be pessimised if I loop over the output since neither the container type (even the array dimensionality is lost, somehow!) nor the element type is well known. I'm guessing but I'd predict thatVector{Union{Missing, Float64}}would be fastest for subsequent iteration, but it's possible that if inference found something likeVector{<:Union{Missing, Float64}}then this would also be quite fast (I'm not sure, really).Regarding the second, more semantic issue of what the output actually becomes at run time, I'm a bit confused about what I'm meant to assume I can do with the output container. For these input types, we can get
As a programmer, this scares me somewhat because I might get subtly different program behavior (or program correctness) depending on my input data. My concerm is that I might perform tests with mixtures of
MissingandFloat64input values but be caught out in rare cases in production when all (or none) of the inputs aremissing.In this hypothetical scenario, my (tested, production) code might look something like
But unfortunately this would sometimes fail, depending on my input data, which may vary in size and quality (and maybe 1 in 10,000 times are all non-
missing).As a coder, I need to be able to predict the interface provided by the outputs of operation so I can write reliable code. Generally, I've noted that the output of
+,map,broadcast,filter(forArrayinputs) tend to be mutableArrays, and generally am unafraid to mutate them. Naively, I would track in my head the eltypes ofvec1andvec2and assume I can populatevec3with anything consistent.I'm not sure what the best solution is, but from my perspective I find any potential (performance?) gains in returning the narrowest container that fits the run-time data questionable in value compared to understanding the semantic guarantees as a code author.
The text was updated successfully, but these errors were encountered: