`cumsum` gives bad error message when collection elements can't be added about julia HOT 6 OPEN

restructuring of the logic of that accumulate function to avoid trying to force the input type to be equal to the output type, which is not a constraint imposed by the documentation of accumulate.

👍🏻 If we can avoid promote_op altogether, I think that would be best.

from julia.

Yes! Though inference returning Union{} is not "cannot infer" it's "inferred that it cannot return".

"+(Tuple{Float64, Float64}, Tuple{Float64, Float64}) does not return"

from julia.

Oh, right, cause not being able to infer the type should still fallback to Any. Okay, I can make a PR later if you're fine with that wording and it being an ArgumentError?

from julia.

I am a little bit unclear on why #53461 is thought to fix this. The issue here seems to be that in the implementation of accumulate, it was assumed that promote_op(add_sum, T, T) >: promote_op(reduce_first, typeof(add_sum), T), but that is not a particularly well-founded assumption, since the later is defined to return either T or something equivalent (such as Int for SmallInt), but this is simply one example of why that assumption is not valid.

I think either the correct value there is either some sort of Union{promote_op(add_sum, T, T), promote_op(reduce_first, typeof(add_sum), T)} or typejoin, or a restructuring of the logic of that accumulate function to avoid trying to force the input type to be equal to the output type, which is not a constraint imposed by the documentation of accumulate.

For example, this sample code is also in the docstring, but errors when simple variants of it are attempted to be used:

julia> accumulate(Pair, [i^2 for i in 1:3])
ERROR: MethodError: Cannot `convert` an object of type Int64 to an object of type Pair{Int64, Int64}
The function `convert` exists, but no method is defined for this combination of argument types.

Closest candidates are:
  Pair{A, B}(::Any, ::Any) where {A, B}
   @ Base Base.jl:219
  convert(::Type{Pair{A, B}}, ::Pair{A, B}) where {A, B}
   @ Base pair.jl:61
  convert(::Type{Pair{A, B}}, ::Pair) where {A, B}
   @ Base pair.jl:62
  ...

Stacktrace:
 [1] setindex!(A::Vector{Pair{Int64, Int64}}, x::Int64, i::Int64)
   @ Base ./array.jl:976
 [2] _accumulate1!(op::Type{Pair}, B::Vector{Pair{Int64, Int64}}, v1::Int64, A::Vector{Int64}, dim::Int64)
   @ Base ./accumulate.jl:435
 [3] _accumulate!(op::Type, B::Vector{Pair{Int64, Int64}}, A::Vector{Int64}, dims::Nothing, init::Nothing)
   @ Base ./accumulate.jl:363
 [4] #accumulate!#991
   @ ./accumulate.jl:348 [inlined]
 [5] accumulate(op::Type, A::Vector{Int64}; dims::Nothing, kw::@Kwargs{})
   @ Base ./accumulate.jl:291
 [6] accumulate(op::Type, A::Vector{Int64})
   @ Base ./accumulate.jl:278
 [7] top-level scope
   @ REPL[12]:1

from julia.

The issue is that cumsum and many of the other accumulators in that file are calling promote_op to infer the return eltype. The docstring doesn't recommend using promote_op, but in this case we don't have much of an option. The indexing error makes sense cause you can't assign anything to Vector{Union{}} and reduce_first(+, Tuple{Int, Int}) is fine prior to insertion.

I don't think it makes sense to throw a MethodError from those calls, but we might be able to modify promote_op to include throw_error=false. Then in places where returning Union{} will error anyways we could have it consistently do error with something like "Cannot infer return type for +(Tuple{Float64, Float64}, Tuple{Float64, Float64})". Would an error message like that be helpful?

from julia.

Thanks! Feel free to ping me in the PR.

from julia.