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.

Sign up for GitHub

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

Jump to bottom

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
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/MArray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,12 @@ end
end
end

@generated function (::Type{MArray{S}})(x::T) where {S, T <: Tuple}
return quote
$(Expr(:meta, :inline))
MArray{S,promote_tuple_eltype(T),$(tuple_length(S)),$(tuple_prod(S))}(x)
end
end
# Promote to common element type
@inline (::Type{MArray{S}})(x::Tuple) where {S <: Tuple} = MArray{S}(promote(x...))
# ...once all elements are the same, call the constructor with all parameters determined
@inline (::Type{MArray{S}})(x::NTuple{N,T}) where {S <: Tuple, T, N} = MArray{S, T, tuple_length(S), N}(x)
# ...the empty case requires special casing since T isn't defined in the above definition in the empty case
@inline (::Type{MArray{S}})(x::Tuple{}) where {S <: Tuple} = MArray{S, Union{}, tuple_length(S), 0}(x)

@generated function (::Type{MArray{S,T,N}})(::UndefInitializer) where {S,T,N}
return quote
Expand Down
2 changes: 1 addition & 1 deletion src/MVector.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const MVector{S, T} = MArray{Tuple{S}, T, 1, S}

@inline MVector(x::NTuple{S,Any}) where {S} = MVector{S}(x)
@inline MVector{S}(x::NTuple{S,T}) where {S, T} = MVector{S, T}(x)
@inline MVector{S}(x::NTuple{S,Any}) where {S} = MVector{S, promote_tuple_eltype(typeof(x))}(x)
@inline MVector{S}(x::Tuple) where {S} = MVector{S}(promote(x...))

# Simplified show for the type
#show(io::IO, ::Type{MVector{N, T}}) where {N, T} = print(io, "MVector{$N,$T}")
Expand Down
13 changes: 6 additions & 7 deletions src/SArray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,12 @@ end
end
end

@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 •
edited
Loading

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.

end
end

# Promote to common element type
@inline (::Type{SArray{S}})(x::Tuple) where {S <: Tuple} = SArray{S}(promote(x...))
# ...once all elements are the same, call the constructor with all parameters determined
@inline (::Type{SArray{S}})(x::NTuple{N,T}) where {S <: Tuple, N, T} = SArray{S, T, tuple_length(S), N}(x)
# ...the empty case requires special casing since T isn't defined in the above definition in the empty case
@inline (::Type{SArray{S}})(x::Tuple{}) where {S <: Tuple} = SArray{S, Union{}, tuple_length(S), 0}(x)
@inline SArray(a::StaticArray) = SArray{size_tuple(Size(a))}(Tuple(a))

# Simplified show for the type
Expand Down
2 changes: 1 addition & 1 deletion src/SHermitianCompact.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ end
SHermitianCompact{N, T, L}(a)
end

@inline (::Type{SHermitianCompact{N}})(a::Tuple) where {N} = SHermitianCompact{N, promote_tuple_eltype(a)}(a)
@inline (::Type{SHermitianCompact{N}})(a::Tuple) where {N} = SHermitianCompact{N}(promote(a...))
@inline (::Type{SHermitianCompact{N}})(a::NTuple{M, T}) where {N, T, M} = SHermitianCompact{N, T}(a)
@inline SHermitianCompact(a::StaticMatrix{N, N, T}) where {N, T} = SHermitianCompact{N, T}(a)

Expand Down
14 changes: 8 additions & 6 deletions src/SMatrix.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,34 +16,36 @@ unknown to the compiler (the element type may optionally also be specified).
"""
const SMatrix{S1, S2, T, L} = SArray{Tuple{S1, S2}, T, 2, L}

@generated function (::Type{SMatrix{S1}})(x::NTuple{L,Any}) where {S1,L}
@generated function (::Type{SMatrix{S1}})(x::NTuple{L,T}) where {S1,L,T}
S2 = div(L, S1)
if S1*S2 != L
throw(DimensionMismatch("Incorrect matrix sizes. $S1 does not divide $L elements"))
end

return quote
$(Expr(:meta, :inline))
T = promote_tuple_eltype(typeof(x))
SMatrix{S1, $S2, T, L}(x)
end
end
@inline (::Type{SMatrix{S1}})(x::Tuple) where {S1} = SMatrix{S1}(promote(x...))

@generated function (::Type{SMatrix{S1,S2}})(x::NTuple{L,Any}) where {S1,S2,L}
@generated function (::Type{SMatrix{S1,S2}})(x::NTuple{L,T}) where {S1,S2,L,T}
return quote
$(Expr(:meta, :inline))
T = promote_tuple_eltype(typeof(x))
SMatrix{S1, S2, T, L}(x)
end
end
(::Type{SMatrix{S1,S2}})(x::Tuple{}) where {S1,S2} = SMatrix{S1, S2, Union{}, 0}(x)
@inline (::Type{SMatrix{S1,S2}})(x::Tuple) where {S1,S2} = SMatrix{S1,S2}(promote(x...))

SMatrixNoType{S1, S2, L, T} = SMatrix{S1, S2, T, L}
@generated function (::Type{SMatrixNoType{S1, S2, L}})(x::NTuple{L,Any}) where {S1,S2,L}
@generated function (::Type{SMatrixNoType{S1, S2, L}})(x::NTuple{L,T}) where {S1,S2,L,T}
return quote
$(Expr(:meta, :inline))
T = promote_tuple_eltype(typeof(x))
SMatrix{S1, S2, T, L}(x)
end
end
@inline (::Type{SMatrixNoType{S1,S2,L}})(x::NTuple{L,Any}) where {S1,S2,L} = SMatrixNoType{S1,S2,L}(promote(x...))

@generated function (::Type{SMatrix{S1,S2,T}})(x::NTuple{L,Any}) where {S1,S2,T,L}
return quote
Expand Down
2 changes: 1 addition & 1 deletion src/SVector.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const SVector{S, T} = SArray{Tuple{S}, T, 1, S}

@inline SVector(x::NTuple{S,Any}) where {S} = SVector{S}(x)
@inline SVector{S}(x::NTuple{S,T}) where {S, T} = SVector{S,T}(x)
@inline SVector{S}(x::T) where {S, T <: Tuple} = SVector{S,promote_tuple_eltype(T)}(x)
@inline SVector{S}(x::Tuple) where {S} = SVector{S}(promote(x...))

# conversion from AbstractVector / AbstractArray (better inference than default)
#@inline convert{S,T}(::Type{SVector{S}}, a::AbstractArray{T}) = SVector{S,T}((a...))
Expand Down
16 changes: 0 additions & 16 deletions src/util.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,6 @@ TupleN{T,N} = NTuple{N,T}
end
end

# Base gives up on tuples for promote_eltype... (TODO can we improve Base?)
@generated function promote_tuple_eltype(::Union{T,Type{T}}) where T <: Tuple
t = Union{}
for i = 1:length(T.parameters)
tmp = T.parameters[i]
if tmp <: Vararg
tmp = tmp.parameters[1]
end
t = :(promote_type($t, $tmp))
end
return quote
@_inline_meta
$t
end
end

# The ::Tuple variants exist to make sure that anything that calls with a tuple
# instead of a Tuple gets through to the constructor, so the user gets a nice
# error message
Expand Down
9 changes: 9 additions & 0 deletions test/SArray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,3 +122,12 @@
@test @inferred(promote_type(SMatrix{2,3,Float32,6}, SMatrix{2,3,Complex{Float64},6})) === SMatrix{2,3,Complex{Float64},6}
end
end

# It useed to be a problem that element promotion didn't work for newly defined types
struct _TestFloat64 <: Real
x::Float64
end
Base.promote_rule(::Type{Float64}, ::Type{_TestFloat64}) = _TestFloat64
@testset "promotion with new types" begin
@test eltype(SArray{Tuple{2}}((1.0, _TestFloat64(1.0)))) === _TestFloat64
end
4 changes: 2 additions & 2 deletions test/SHermitianCompact.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,13 +219,13 @@ fill3(x) = fill(3, x)
@testset "transpose/adjoint" begin
a = Hermitian([[rand(Complex{Int}) for i = 1 : 2, j = 1 : 2] for row = 1 : 3, col = 1 : 3])
if VERSION < v"1.2.0-"
@test transpose(SHermitianCompact{3}(a)) == transpose(a)
@test_broken transpose(SHermitianCompact{3}(a)) == transpose(a)
else
# FIXME: This `transpose(a)` crashes in v1.2.0-rc1.
# See https://github.com/JuliaLang/julia/issues/32222
@test_skip transpose(SHermitianCompact{3}(a)) == transpose(a)
end
@test adjoint(SHermitianCompact{3}(a)) == adjoint(a)
@test_broken adjoint(SHermitianCompact{3}(a)) == adjoint(a)

b = Hermitian([rand(Complex{Int}) for i = 1 : 3, j = 1 : 3])
@test adjoint(SHermitianCompact{3}(b)) == adjoint(b)
Expand Down