HasLength trait for some Flatten iterators #22691

mschauer · 2017-07-06T11:49:31Z

It is worthwhile to give some Flatten-ed iterators the trait HasLength, if the elements of the iterator which gets flattened are iteratables with type-static length, for example Tuples. This PR does this for iterators with elements of type Tuple and Number.

This functionality can then be extended by packages defining iterators with type-encoded length.
For example it allows https://github.com/JuliaArrays/StaticArrays.jl to flatten a Vector of SVector's efficiently.
Replaces #16680

nalimilan · 2017-07-06T11:59:55Z

base/iterators.jl

 iteratoreltype(::Type{Flatten{I}}) where {I} = _flatteneltype(I, iteratoreltype(I))
 _flatteneltype(I, ::HasEltype) = iteratoreltype(eltype(I))
 _flatteneltype(I, et) = EltypeUnknown()

+flatten_iteratorsize(::Union{HasShape, HasLength}, b::Type{T}) where {T<:Tuple} = HasLength()


Split lines at 92 characters. Here, you can do ::Type{<:Tuple} to save some space.

Sacha0 · 2017-07-06T20:25:00Z

base/iterators.jl

+flatten_iteratorsize(::Union{HasShape, HasLength}, b::Type{T}) where {T<:Tuple} = HasLength()
+flatten_iteratorsize(::Union{HasShape, HasLength}, b::Type{T}) where {T<:Number} = HasLength()
+flatten_iteratorsize(a, b) = SizeUnknown()
+flatten_length(f, ::Type{T}) where {T<:Tuple} = isleaftype(T) ? nfields(T)*length(f.it) : throw(ArgumentError("Cannot compute length of a tuple-type which is not a leaf-type"))


Line length here as well. Perhaps write this method definition long-form with an if-else?

Sacha0 · 2017-07-06T20:25:34Z

base/iterators.jl

+flatten_length(f, ::Type{T}) where {T<:Tuple} = isleaftype(T) ? nfields(T)*length(f.it) : throw(ArgumentError("Cannot compute length of a tuple-type which is not a leaf-type"))
+flatten_length(f, ::Type{T}) where {T<:Number} = length(f.it)
+length(f::Flatten{I}) where {I} = flatten_length(f, eltype(I))
+iteratorsize(::Type{Flatten{I}}) where {I} = isleaftype(eltype(I)) ? flatten_iteratorsize(iteratorsize(I), eltype(I)) : SizeUnknown() # eltype is always defined


Likewise here re. line length. Perhaps also better as a long-form method definition with an if-else? :)

Sacha0 · 2017-07-06T20:26:04Z

test/iterators.jl

@@ -347,7 +347,9 @@ end
 @test collect(flatten(Any[flatten(Any[1:2, 6:5]), flatten(Any[6:7, 8:9])])) == Any[1,2,6,7,8,9]
 @test collect(flatten(Any[2:1])) == Any[]
 @test eltype(flatten(UnitRange{Int8}[1:2, 3:4])) == Int8
+@test length(flatten(zip(1:3,4:6))) == 6


A space between the arguments to zip would aid visual parsing :).

Sacha0 · 2017-07-06T20:26:59Z

test/iterators.jl

 @test_throws ArgumentError collect(flatten(Any[]))
+@test_throws ArgumentError length(flatten(NTuple[(1,),()])) # #16680


Likewise here, a space following the comma between Tuples might make this easier to parse visually? :)

For future reference: https://en.wikipedia.org/wiki/Klempen

rfourquet · 2017-07-07T09:30:25Z

base/iterators.jl

-flatten_length(f, ::Type{T}) where {T<:Tuple} = isleaftype(T) ? nfields(T)*length(f.it) : throw(ArgumentError("Cannot compute length of a tuple-type which is not a leaf-type"))
+
+function flatten_length(f, ::Type{<:Tuple})
+    if !isleaftype(T)


T is not defined anymore, adding T::Type{<:Tuple} in the signature should work

Isn't function flatten_length(f, ::Type{T}) where T<:Tuple better?

Sorry, I did not pay attention here

No problem!

@fredrikekre you mean better in style, or... in efficiency?

ok, I thought T::Type may be less efficient than ::Type{T} where T, but that it was not the case for T::Type{<:Tuple} which should be specialized for each different type...

rfourquet · 2017-07-07T09:36:01Z

base/iterators.jl

+    end
+    nfields(T)*length(f.it)
+end
+flatten_length(f, ::Type{T}) where {T<:Number} = length(f.it)


flatten_length(f, ::Type{<:Number}) = length(f.it) ? (just a suggestion!)

rfourquet · 2017-07-07T09:42:06Z

base/iterators.jl

+    nfields(T)*length(f.it)
+end
+flatten_length(f, ::Type{T}) where {T<:Number} = length(f.it)
+length(f::Flatten{I}) where {I} = flatten_length(f, eltype(I))


For non-(number, tuple), this will throw a MethodError right? why not a fall-ball flatten_length which throws a more informative message?

rfourquet · 2017-07-12T08:41:05Z

test/iterators.jl

 @test_throws ArgumentError collect(flatten(Any[]))
+@test_throws ArgumentError length(flatten(NTuple[(1,), ()])) # #16680
+@test_throws ArgumentError length(flatten([[1],[1]]))


rfourquet · 2017-07-12T08:52:24Z

base/iterators.jl

+end
+flatten_length(f, ::Type{<:Number}) = length(f.it)
+flatten_length(f, T) = throw(ArgumentError(
+            "Iterates of the argument to Flatten are not known to have constant length"))


Strange indentation, I think this would be better with only 4 spaces

"Iteratees" wouldn't be the correct word here?

and as you have a T, you might as well report it in the message, e.g. "Iteratees (of type $T) ...

Flatten is private no? could be better to use flatten.

Iteratees would sound funny, even if possibly correct

rfourquet · 2017-07-12T08:55:29Z

base/iterators.jl

+flatten_iteratorsize(a, b) = SizeUnknown()
+
+function flatten_length(f, ::Type{T}) where {T<:Tuple}
+    if !isleaftype(T)


Do you know if this check is statically resolved?

From experience: yes. Can we test that?

rfourquet · 2017-07-12T09:13:03Z

base/iterators.jl

+
+function iteratorsize(::Type{Flatten{I}}) where {I}
+    if isleaftype(eltype(I))
+        flatten_iteratorsize(iteratorsize(I), eltype(I)) # eltype is always defined


With this, you seem to prevent have HasLength for non-concrete numbers, e.g. Union{Int,UInt}[1, 0x2]. Why not move this test to flatten_iteratorsize for tuples, and having simply

iteratorsize(::Type{Flatten{I}}) where {I} = flatten_iteratorsize(iteratorsize(I), eltype(I))

? IOW, similar to what you did for flatten_length.

Yes, this would be better.

rfourquet · 2017-07-12T09:13:50Z

test/iterators.jl

@@ -347,7 +347,10 @@ end
 @test collect(flatten(Any[flatten(Any[1:2, 6:5]), flatten(Any[6:7, 8:9])])) == Any[1,2,6,7,8,9]
 @test collect(flatten(Any[2:1])) == Any[]
 @test eltype(flatten(UnitRange{Int8}[1:2, 3:4])) == Int8
+@test length(flatten(zip(1:3, 4:6))) == 6


you could add a test for numbers too

rfourquet · 2017-07-21T11:44:41Z

base/iterators.jl

+        throw(ArgumentError(
+            "Cannot compute length of a tuple-type which is not a leaf-type"))
+    end
+    nfields(T)*length(f.it)


it seems that nfields was deprecated it favor of fieldcount

this shouldn't have passed tests then. is this covered and running?

As far as I understand, this code path is tested. @mschauer do you know why this was not noticed by CI? (did the change happened after CI was run?)

I think CI picked up the change, but I did not pick it up (locally on my machine a test regarding output format of arrays is broken recently).

rfourquet · 2017-07-21T11:54:01Z

LGTM. Just one question: you are testing tuples with isleaftype, to handle the case where the eltype is an NTuple which doesn't have a fixed number of elements. Is there another case than NTuple which doesn't have a fixed number of elements? otherwise, you could dispatch on NTuple instead, and allow non-leaf-types tuples which have a fixed length (for e.g. flatten(Tuple{Integer}[(1,), (3,)])) to benefit from your work.

mschauer · 2017-07-23T13:56:10Z

Mixed tuples are not NTuples, for this reason I went with the test for leaftype. NTuples which are non-leaf-types could be covered additionally.

mschauer · 2017-07-23T20:16:22Z

Squashed and rebased.

tkelman · 2017-07-23T21:19:21Z

base/iterators.jl

+iteratorsize(::Type{Flatten{I}}) where {I} = flatten_iteratorsize(iteratorsize(I), eltype(I))
+
+function flatten_length(f, ::Type{T}) where {T<:Tuple}
+    if !isleaftype(T)


it's not very intuitive that isleaftype is the right predicate for this - are Vararg and nesting coupled to this?

rfourquet

I will merge tomorrow if no-one objects. Finer handling of tuples can be added later if needed. Thanks @mschauer !

tkelman · 2017-07-24T12:57:56Z

hold on at least until there's a good answer for the question above. is this correct even for Tuples with Vararg or nested tuples?

mschauer · 2017-07-25T19:53:30Z

it's not very intuitive that isleaftype is the right predicate for this - are Vararg and nesting coupled to this?

If a tuple type T is a leaftype, it can have instances t::T. For the instances
length(t) == nfields(typeof(t)) so length is type constant.

rfourquet · 2017-07-27T07:50:17Z

@tkelman good to go now W.R.T. the isleaftype question?

rfourquet · 2017-08-03T07:31:29Z

One CI failure is a timeout, the other seems unrelated.

mschauer mentioned this pull request Jul 6, 2017

Give Flatten-iterator HasLength() in some cases #16680

Closed

nalimilan reviewed Jul 6, 2017

View reviewed changes

Sacha0 reviewed Jul 6, 2017

View reviewed changes

ararslan added the collections Data structures holding multiple items, e.g. sets label Jul 6, 2017

rfourquet reviewed Jul 7, 2017

View reviewed changes

mschauer force-pushed the flatten2 branch from f3081d2 to e2d25fa Compare July 7, 2017 11:16

mschauer mentioned this pull request Jul 11, 2017

Iteratorsize for partition iterator #16552

Merged

rfourquet reviewed Jul 12, 2017

View reviewed changes

mschauer force-pushed the flatten2 branch from e2d25fa to dde472e Compare July 19, 2017 09:15

rfourquet reviewed Jul 21, 2017

View reviewed changes

HasLength trait for some Flatten iterators

30536b8

mschauer force-pushed the flatten2 branch from dde472e to 30536b8 Compare July 23, 2017 18:20

tkelman reviewed Jul 23, 2017

View reviewed changes

rfourquet approved these changes Jul 24, 2017

View reviewed changes

rfourquet merged commit 5e32423 into JuliaLang:master Aug 3, 2017

tkoolen mentioned this pull request Aug 24, 2017

HasLength for even more Flatten iterators #23431

Open

		@test_throws ArgumentError collect(flatten(Any[]))
		@test_throws ArgumentError length(flatten(NTuple[(1,),()])) # #16680

HasLength trait for some Flatten iterators #22691

HasLength trait for some Flatten iterators #22691

Conversation

mschauer commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfourquet commented Jul 21, 2017

mschauer commented Jul 23, 2017

mschauer commented Jul 23, 2017

Choose a reason for hiding this comment

rfourquet left a comment

Choose a reason for hiding this comment

tkelman commented Jul 24, 2017

mschauer commented Jul 25, 2017 • edited Loading

rfourquet commented Jul 27, 2017

rfourquet commented Aug 3, 2017

Sacha0 Jul 6, 2017 •

edited

Loading

mschauer commented Jul 25, 2017 •

edited

Loading