Iteratorsize for partition iterator #16552
Conversation
test/functional.jl
Outdated
| @test v[2] == [3,4] | ||
| @test v[3] == [5] | ||
| let v1 = collect(Base.partition([1,2,3,4,5], 2)), v2 = collect(Base.partition(Base.flatten([1,2,3,4,5]), 2)) | ||
| @test v1[1] == v1[1] == [1,2] |
There was a problem hiding this comment.
presumably this should be v1[1] == v2[1] ?
|
The test failure after adressing @tkelman's spot is unrelated, previously passing test here https://ci.appveyor.com/project/JuliaLang/julia/build/1.0.2015 |
|
Personal assessment: this change is straight forward. |
|
Thanks for the bump, I agree this looks fine. I think all the iterator size business still needs more thorough interface documentation, right? |
|
Yes, I wrote https://gist.github.com/mschauer/c8d8b7eb5b455cb12ddc9bda15695203 but I would like to wait for feedback before writing more docs. See issue #15977 |
|
Bump. |
rfourquet
left a comment
rfourquet
left a comment
There was a problem hiding this comment.
LGTM! (modulo the few comments)
base/iterator.jl
Outdated
|
|
||
| eltype{T}(::Type{PartitionIterator{T}}) = Vector{eltype(T)} | ||
| partition_iteratorsize(::HasShape) = HasLength() | ||
| partition_iteratorsize{T}(isz::T) = isz |
There was a problem hiding this comment.
The type parameter can be omitted.
base/iterator.jl
Outdated
| eltype{T}(::Type{PartitionIterator{T}}) = Vector{eltype(T)} | ||
| partition_iteratorsize(::HasShape) = HasLength() | ||
| partition_iteratorsize{T}(isz::T) = isz | ||
| iteratorsize{T}(P::Type{PartitionIterator{T}}) = partition_iteratorsize(iteratorsize(T)) |
There was a problem hiding this comment.
the argument P can be omitted, and when rebasing update to the where syntax
test/functional.jl
Outdated
| @test v[1] == [1,2] | ||
| @test v[2] == [3,4] | ||
| @test v[3] == [5] | ||
| let v1 = collect(Base.partition([1,2,3,4,5], 2)), v2 = collect(Base.partition(Base.flatten([1,2,3,4,5]), 2)) |
|
If this is good to go, please merge, @rfourquet since you have some outstanding review comments. |
|
@rfourquet Let me make one amendment first, after #22691 |
| partition_iteratorsize(isz) = isz | ||
| function iteratorsize(::Type{PartitionIterator{T}}) where {T} | ||
| partition_iteratorsize(iteratorsize(T)) | ||
| end |
There was a problem hiding this comment.
out of curiosity: you switched to the long-form function syntax because of the where syntax?
|
Thanks @mschauer ! |
5 participants
Before, collect of a partition could fail because it assumed the existence of a length method via the iteratorsize fall back.