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

Define promotion and conversion for CartesianIndices and LinearIndices #28084

Merged
merged 1 commit into from
Jul 13, 2018

Conversation

timholy
Copy link
Member

@timholy timholy commented Jul 12, 2018

Interesting that we hadn't yet noticed the lack of such fundamental methods. Porting packages is good!

@timholy timholy changed the title Define promotion and conversion for CartesianIndices Define promotion and conversion for CartesianIndices and LinearIndices Jul 12, 2018
base/indices.jl Outdated
@@ -357,6 +357,14 @@ LinearIndices(inds::NTuple{N,Union{<:Integer,AbstractUnitRange{<:Integer}}}) whe
LinearIndices(map(i->first(i):last(i), inds))
LinearIndices(A::Union{AbstractArray,SimpleVector}) = LinearIndices(axes(A))

promote_type(::Type{NTuple{N,R1}}, ::Type{NTuple{N,R2}}) where {N,R1<:AbstractUnitRange,R2<:AbstractUnitRange} =
NTuple{N,promote_type(R1,R2)}
Copy link
Member

Choose a reason for hiding this comment

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

What makes tuples of unit ranges special compared to other tuples to justify this? (Other than that this comes handy in this PR?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough. But it's not wrong either. However, I could hide it behind a private method if you really think that's better.

Copy link
Member Author

Choose a reason for hiding this comment

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

This method is running into some trouble too with ambiguity (really, typevar) tests. But I think it's a bug, see #28086.

Copy link
Member

Choose a reason for hiding this comment

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

I'd lean towards making this a private method, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. For fun I played with making it really general,

index 78a1954..f260cbf 100644
--- a/base/promotion.jl
+++ b/base/promotion.jl
@@ -236,6 +236,11 @@ promote_rule(::Type{Any}, ::Type{<:Any}) = Any
 promote_rule(::Type{<:Any}, ::Type{Any}) = Any
 promote_rule(::Type{Any}, ::Type{Any}) = Any
 
+function promote_rule(::Type{Tuple{R1,Vararg{R1,N}}}, ::Type{Tuple{R2,Vararg{R2,N}}}) where {R1,R2,N}
+    R = promote_type(R1, R2)
+    Tuple{R,Vararg{R,N}}
+end
+

Somewhat surprisingly, only one test fails: https://github.com/JuliaLang/julia/blob/master/test/abstractarray.jl#L653
But I'll make it a private method, because that really would be a scary change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants