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.

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

Use Base.promote instead of promote_tuple_eltype to fik promotion #670

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andreasnoack
Copy link
Member

@andreasnoack andreasnoack commented Oct 14, 2019

...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

@test adjoint(SHermitianCompact{3}(a)) == adjoint(a)
because there is no promote_rule between the matrix valued elements. @tkoolen Any thoughts on the better way to fix this case?

@andyferris
Copy link
Member

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
Copy link
Member

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?

@generated function (::Type{SArray{S}})(x::T) where {S <: Tuple, T <: Tuple}
return quote
@_inline_meta
SArray{S, $(promote_tuple_eltype(T)), $(tuple_length(S)), $(tuple_prod(S))}(x)
Copy link
Member

Choose a reason for hiding this comment

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

@andyferris ah hah; here's where we call promote_tuple_eltype from within a generated function — looking through this PR I think we'd only need to change this one line (by removing the $ character :-) ) to fix the original problem. Is that right @andreasnoack?

Regardless of that it's always nice to remove some generated functions so I'm generally positive about doing these changes as a cleanup. Provided the original reason for needing promote_tuple_eltype is gone.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just tried it and it doesn't seem to be sufficient to fix the issue.

Copy link
Member

@c42f c42f Oct 15, 2019

Choose a reason for hiding this comment

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

That's weird, I was sure this would fix it. Locally removing the $ seems to work for me:

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},MyFloat64,1,2} with indices SOneTo(2):
 MyFloat64(2.0)
 MyFloat64(1.0)

(btw beware that you can't always rely on Revise to work with this stuff... changes may require restarting Julia and reloading StaticArrays)

Copy link
Member

Choose a reason for hiding this comment

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

Good catch, @c42f.

I guess if we just change this one line, then it should unambiguously have no negative impact on generated code. If we merge the whole PR we should probably also stress test this with long Union-type static arrays, etc, first.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what went wrong but indeed it seems to be sufficient to fix the issue. I've opened #671. I still think we should consider geetting rid of the generated function here but it's not urgent.

issue for element types defined after StaticArrays.
@andreasnoack
Copy link
Member Author

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?

@coveralls
Copy link

Coverage Status

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

@c42f
Copy link
Member

c42f commented Oct 15, 2019

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...

@andyferris
Copy link
Member

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

@andyferris
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants