StaticArrays.jl icon indicating copy to clipboard operation
StaticArrays.jl copied to clipboard

Use Base.promote instead of promote_tuple_eltype to fik promotion

Open andreasnoack opened this issue 6 years ago • 9 comments

...issue for element types defined after StaticArrays.

Currently, we have this behavior

julia> struct MyFloat64 <: Real
         x::Float64
       end

julia> Base.promote_rule(::Type{Float64}, ::Type{MyFloat64}) = MyFloat64

julia> SArray{Tuple{2}}((2.0, MyFloat64(1.0)))
2-element SArray{Tuple{2},Real,1,2} with indices SOneTo(2):
           2.0
 MyFloat64(1.0)

probably because the generated function promote_tuple_eltype can't handle types defined later than itself. With this PR, the example gives

julia> SArray{Tuple{2}}((2.0, MyFloat64(1.0)))
2-element SArray{Tuple{2},MyFloat64,1,2} with indices SOneTo(2):
 MyFloat64(2.0)
 MyFloat64(1.0)

The PR generally reduces the number of generated functions.

A remaining issue is https://github.com/JuliaArrays/StaticArrays.jl/blob/4cfeb5859235c67bd0e0ca378d045e135fd32921/test/SHermitianCompact.jl#L228 because there is no promote_rule between the matrix valued elements. @tkoolen Any thoughts on the better way to fix this case?

andreasnoack avatar Oct 14 '19 10:10 andreasnoack

Cool. Did you look into performance of promote(x...)?

Interesting corner cases to consider include Union element types, or longer tuples, or both. Certainly we wouldn't want generated code to regress for things like SVector((x::Float64, y::Float64, 0::Int)) on any of the Julia 1.0, 1.1, 1.2, or 1.3 series compilers.

andyferris avatar Oct 15 '19 00:10 andyferris

probably because the generated function promote_tuple_eltype can't handle types defined later than itself.

This is confusing, right? The function generator doesn't currently call promote_type at all... it generates code that can call the latest version by simply expanding out the tuple's type parameters. Does anyone understand what might be going on there?

andyferris avatar Oct 15 '19 00:10 andyferris

Cool. Did you look into performance of promote(x...)?

I did some timings and didn't see any regressions but timing StaticArrays code is always a bit tricky. I guess the most important thing is that I didn't see any new allocations. I assumed that the old issue that made you promote_tuple_eltype would show up as @inferred failures. Is that correct?

andreasnoack avatar Oct 15 '19 06:10 andreasnoack

Coverage Status

Coverage decreased (-0.4%) to 81.158% when pulling c72f42f6c2929f14117b6c84f3d163dfa80a4ee1 on an/promote into 4cfeb5859235c67bd0e0ca378d045e135fd32921 on master.

coveralls avatar Oct 15 '19 07:10 coveralls

Doing some archeology, the origin of promote_tuple_eltype was in the very first StaticArrays code commit cef8abc (under then name promote_eltype). That might even have been prior to julia 0.5 so it seems very likely we don't need this hack anymore. But I also don't know exactly what caused Andy to put it in...

c42f avatar Oct 15 '19 08:10 c42f

lol, nice archaeology. Constructors were done first - promotion behaving like other arrays obviously being pretty important. How time flies :)

andyferris avatar Oct 15 '19 09:10 andyferris

In any case we should double check some generated (native) code first but in general I am in favour of removing generated code.

andyferris avatar Oct 15 '19 09:10 andyferris

This seems to work, so I guess this can be closed @andreasnoack?

moble avatar Jun 24 '25 00:06 moble

I think the consensus was in favor of this PR even though the motivating example was fixed in my other PR.

andreasnoack avatar Jun 24 '25 06:06 andreasnoack