-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Make partition support non-1-indexed vector. #40830
Changes from 6 commits
5bdd80b
e9ff809
97fa632
5eb64b1
af7e493
3ace109
d87b631
f1e8f17
df2afa7
b5b3bdd
8962a99
5c5316e
ad54565
31b4b51
980e4c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -1171,10 +1171,10 @@ function length(itr::PartitionIterator) | |||||
return cld(l, itr.n) | ||||||
end | ||||||
|
||||||
function iterate(itr::PartitionIterator{<:AbstractRange}, state=1) | ||||||
state > length(itr.c) && return nothing | ||||||
function iterate(itr::PartitionIterator{<:AbstractVector}, state = firstindex(itr.c)) | ||||||
state > lastindex(itr.c) && return nothing | ||||||
r = min(state + itr.n - 1, length(itr.c)) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
return @inbounds itr.c[state:r], r + 1 | ||||||
return @inbounds view(itr.c, state:r), r + 1 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this change doesn't make a difference, there's no need to change it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the view makes it consistent with the docstring There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am a bit hesitant about this, at least as part of this PR. Since this is supposed to just fix a bug can we remove it for now and perhaps propose that change in a separate PR? |
||||||
end | ||||||
|
||||||
function iterate(itr::PartitionIterator{<:AbstractArray}, state=1) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm, actually, no need. Because here it's using LinearIndexing rules, which always assume 1-based. |
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -786,3 +786,8 @@ end | |
@test b[i] == a[r[i]] | ||
end | ||
end | ||
|
||
@testset "proper patition for non-1-indexed vector" begin | ||
@test partition(OffsetArray(1:10,10), 5) |> collect == [1:5,6:10] # OffsetVector | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and also |
||
@test partition(IdOffsetRange(2:7,10), 5) |> collect == [12:16,17:17] # IdOffsetRange | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not clear with "invalid", if we add a dispatch for
PartitionIterator{<:AbstractRange}
, then everything is the same except for replacingview
withgetindex
.This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on 1.6.1
something changed in dev?
This comment was marked as resolved.
Sorry, something went wrong.