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

Handle chunking of empty collections properly #30

Closed
carstenbauer opened this issue Mar 17, 2024 · 3 comments
Closed

Handle chunking of empty collections properly #30

carstenbauer opened this issue Mar 17, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@carstenbauer
Copy link
Member

carstenbauer commented Mar 17, 2024

julia> using ChunkSplitters

julia> collect(chunks(11:10; n=6))
ERROR: ArgumentError: You must either indicate the desired number of chunks (n) or the target size of a chunk (size).
Stacktrace:
  [1] missing_input_err()
    @ ChunkSplitters ~/.julia/packages/ChunkSplitters/R5lnx/src/ChunkSplitters.jl:118
  [2] getchunk(itr::UnitRange{Int64}, ichunk::Int64; n::Int64, size::Int64, split::Symbol)
    @ ChunkSplitters ~/.julia/packages/ChunkSplitters/R5lnx/src/ChunkSplitters.jl:274
  [3] getchunk
    @ ~/.julia/packages/ChunkSplitters/R5lnx/src/ChunkSplitters.jl:273 [inlined]
  [4] getchunk
    @ ~/.julia/packages/ChunkSplitters/R5lnx/src/ChunkSplitters.jl:293 [inlined]
  [5] iterate
    @ ~/.julia/packages/ChunkSplitters/R5lnx/src/ChunkSplitters.jl:142 [inlined]
  [6] iterate
    @ ~/.julia/packages/ChunkSplitters/R5lnx/src/ChunkSplitters.jl:141 [inlined]
  [7] copyto!(dest::Vector{StepRange{Int64, Int64}}, src::ChunkSplitters.Chunk{UnitRange{Int64}, ChunkSplitters.FixedCount})
    @ Base ./abstractarray.jl:943
  [8] _collect
    @ ./array.jl:765 [inlined]
  [9] collect(itr::ChunkSplitters.Chunk{UnitRange{Int64}, ChunkSplitters.FixedCount})
    @ Base ./array.jl:759
 [10] top-level scope
    @ REPL[32]:1

The error message is clearly wrong and bad.

The fundamental question is what should happen in this case. We could (1) improve the error message or (2) return an empty Vector{StepRange{Int64, Int64}} (consistent with the finite case, e.g. collect(chunks(9:10; n=6))).

Came up in JuliaFolds2/OhMyThreads.jl#86.

@lmiq
Copy link
Collaborator

lmiq commented Mar 17, 2024

I definitely prefer returning the empty vector.

@lmiq
Copy link
Collaborator

lmiq commented Mar 20, 2024

To return the empty step range vector one must return nothing from iterate. Additionaly, it seems consistent to return nothing from getchunk. I have implemented this here:

https://github.com/JuliaFolds2/ChunkSplitters.jl/pull/31/files

One additional thing, which I found slightly inelegant, is that we need to compute the length of the iterator by:

length(c::Chunk{T,FixedSize}) where {T} = cld(length(c.itr), max(1, c.size))

because c.size can be zero and still be a valid, empty, iterator.

Thus, now, we get:

    @test getchunk(10:9, 1; n=2) === nothing
    @test getchunk(10:9, 1; size=2) === nothing
    @test collect(chunks(10:9; n=2)) == Vector{StepRange{Int,Int}}()
    @test collect(chunks(10:9; size=2)) == Vector{StepRange{Int,Int}}()
    @test collect(enumerate(chunks(10:9; n=2))) == Tuple{Int64,Vector{StepRange{Int,Int}}}[]
    @test collect(enumerate(chunks(10:9; size=2))) == Tuple{Int64,Vector{StepRange{Int,Int}}}[]

If you have any comment, let me know.

@lmiq
Copy link
Collaborator

lmiq commented Apr 1, 2024

fixed in v2.4.2

@lmiq lmiq closed this as completed Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants