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

Make partition support non-1-indexed vector. #40830

Merged
merged 15 commits into from
May 21, 2021
Merged

Make partition support non-1-indexed vector. #40830

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5bdd80b
Create iterators.jl
N5N3 May 15, 2021
e9ff809
add test
N5N3 May 15, 2021
97fa632
Update iterators.jl
N5N3 May 15, 2021
5eb64b1
Update offsetarray.jl
N5N3 May 15, 2021
af7e493
Update iterators.jl
N5N3 May 15, 2021
3ace109
Update offsetarray.jl
N5N3 May 15, 2021
d87b631
Update iterators.jl
N5N3 May 15, 2021
f1e8f17
Update offsetarray.jl
N5N3 May 15, 2021
df2afa7
Update test/offsetarray.jl
N5N3 May 16, 2021
b5b3bdd
Update offsetarray.jl
N5N3 May 16, 2021
8962a99
Update offsetarray.jl
N5N3 May 16, 2021
5c5316e
Merge branch 'JuliaLang:master' into master
N5N3 May 20, 2021
ad54565
restore the dispatch for AbstractRange
N5N3 May 20, 2021
31b4b51
Merge branch 'JuliaLang:master' into master
N5N3 May 20, 2021
980e4c6
Update iterators.jl
N5N3 May 20, 2021
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
6 changes: 3 additions & 3 deletions base/iterators.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Member Author

@N5N3 N5N3 May 15, 2021 •
edited
Loading

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 replacing view with getindex.

This comment was marked as resolved.

Sign in to view

Copy link
Member Author

Choose a reason for hiding this comment

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

on 1.6.1

julia> UnitRange <: AbstractRange <: AbstractVector
true

something changed in dev?

This comment was marked as resolved.

Sign in to view

state > lastindex(itr.c) && return nothing
r = min(state + itr.n - 1, length(itr.c))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
r = min(state + itr.n - 1, length(itr.c))
r = min(state + itr.n - 1, lastindex(itr.c))

return @inbounds itr.c[state:r], r + 1
return @inbounds view(itr.c, state:r), r + 1
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the view makes it consistent with the docstring

Copy link
Member

Choose a reason for hiding this comment

The 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)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the AbstractArray method can be improved, too?

Copy link
Member

Choose a reason for hiding this comment

The 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.

Expand Down