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

out of bounds error on empty iterator indexing #59

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

lmiq
Copy link
Collaborator

@lmiq lmiq commented Mar 20, 2025

This PR aims to fix #58 by considering out-of-bounds indexing of the chunk iterator an error even if the iterator is empty.

Obs: Checked that all OhMyThreads tests passed.

Previously:

julia> chunks(Float64[]; n=2)[1]
0-element view(::Vector{Float64}, 0:-1) with eltype Float64

julia> chunks(0:-1; n=2)[1]
-1:-2

Now:

julia> chunks(Float64[]; n=2)[1]
ERROR: ArgumentError: chunk index out of bounds: 1 not in 1:0 (length=0)

julia> chunks(0:-1; n=2)[1]
ERROR: ArgumentError: chunk index out of bounds: 1 not in 1:0 (length=0)

@lmiq lmiq changed the title Out of range out of bounds error on empty iterator indexing Mar 20, 2025
@@ -103,8 +102,8 @@ Base.getindex(c::ViewChunks{T,C,S}, i::Int) where {T,C,S} = @view(c.collection[g

Base.eltype(::IndexChunks{T,C,Consecutive}) where {T,C} = UnitRange{Int}
Base.eltype(::IndexChunks{T,C,RoundRobin}) where {T,C} = StepRange{Int,Int}
Base.eltype(c::ViewChunks{T,C,Consecutive}) where {T,C} = typeof(c[firstindex(c)])
Base.eltype(c::ViewChunks{T,C,RoundRobin}) where {T,C} = typeof(c[firstindex(c)])
Base.eltype(c::ViewChunks{T,C,Consecutive}) where {T,C} = typeof(@view(c.collection[firstindex(c.collection):lastindex(c.collection)]))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These changes are important to allow eltype to return the proper types without having to call getchunkindices. firstindex and lastindex are considered part of the required interface.

@lmiq lmiq self-assigned this Mar 20, 2025
@lmiq lmiq requested a review from carstenbauer March 20, 2025 20:21
@lmiq
Copy link
Collaborator Author

lmiq commented Mar 20, 2025

@carstenbauer

Want to take a look? I think this is more consistent throwing an error for empty iterators and can be considered a bug-fix.

OhMyThreads.jl tests are not affected.

@carstenbauer
Copy link
Member

Haven't look at this in detail, but why are you throwing an ArgumentError rather than an BoundsError?

@lmiq
Copy link
Collaborator Author

lmiq commented Mar 21, 2025

Haven't look at this in detail, but why are you throwing an ArgumentError rather than an BoundsError?

The "historical" reason is that this error was reached when the chunks object was constructed, not only when indexing into it.

Now, we can turn that into a BoundsError, at the cost of having a less specific error message, but probably that's for the best. Updated it now.

@nhz2
Copy link

nhz2 commented Mar 24, 2025

Yes, this fixes #58
I also think the docstrings of chunks and index_chunks could be updated to indicate that the default minsize is 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

What should happen if length of collection is less than n?
3 participants