RFC reduce promotion: default to system default #20607
Conversation
ararslan
added
maths
base/reduce.jl
Outdated
| @@ -13,20 +13,23 @@ const SmallUnsigned = Union{UInt8,UInt16,UInt32} | |||
| end | |||
|
|
|||
| const CommonReduceResult = Union{UInt64,UInt128,Int64,Int128,Float32,Float64} | |||
| const WidenReduceResult = Union{SmallSigned, SmallUnsigned, Float16} | |||
| const SmallReduceResult = Union{SmallSigned, SmallUnsigned, Float16} | |||
There was a problem hiding this comment.
SmallReduceResult seems like a misnomer here. The result is Int, which is not small.
Float16 should still be widened to Float32, presumably, which is not a "system size". Or I guess the widening there could be eliminated.
There was a problem hiding this comment.
If Float16 is a "real computational type" #5942 (comment) it shouldn't be widened, right?
|
|
Bump. |
| r_promote_type{T<:WidenReduceResult}(op, ::Type{T}) = widen(T) | ||
| r_promote_type{T<:WidenReduceResult}(::typeof(+), ::Type{T}) = widen(T) | ||
| r_promote_type{T<:WidenReduceResult}(::typeof(*), ::Type{T}) = widen(T) | ||
| r_promote_type{T<:WidenReduceResult}(op, ::Type{T}) = promote_sys_size(T) |
There was a problem hiding this comment.
As noted in #21523, widening is not necessarily appropriate for an arbitrary user-defined operator. Maybe the default should be T.
|
Will try and do this in June (changed jobs recently, hence different availability to before). If I understand the consensus correctly, it's to remove any promotion from With apologies, the thing that held this up in the first place is that I couldn't figure out how to run doctests. @KristofferC |
|
@felixrehren, are you still able to work on this? |
|
Don't let the doctests hold you up then, they can be fixed later. |
|
Hey @felixrehren, I've continued this PR in #22825. Thanks for your work on it! |
KristofferC
removed
needs news
Before this PR, the result type of a
map_reduceis erratic on integers.After this PR,
sum(xs)foreltype(xs) = IntXreturnsIntY, whereYthe bit-size of the system, whenX <= Y. I.e. on a 32-bit system,sum(Int8[])::Int32and on a 64-bit systemsum(Int8[])::Int64, etc. Types wider than the system size are untouched, as before. The same changes hold for unsigned integers andprod.This paves the way for
sum(T, xs)::T, orsum(zero(T), xs)::Tto be type-stable, see #20561Float16are no longer widened, cf. is Float16 a real computational type or not? #5942 (comment)widen(::Type{Int8}) = Int32. Can we change this towiden(::Type{Int8}) = Int16? (in line with every other case, wherewiden(::Type{IntX}) = Int2X)