JuliaFolds2 / ChunkSplitters.jl

Simple chunk splitters for parallel loop executions
MIT License
40 stars 5 forks source link

Empty range instead of nothing when getchunk returns #38

Closed lmiq closed 2 months ago

lmiq commented 3 months ago

Currently, the getchunk function returns nothing if the iterator is empty:

julia> c = ChunkSplitters.chunks(2:1; n=1)
ChunkSplitters.Chunk{UnitRange{Int64}, ChunkSplitters.FixedCount}(2:1, 0, 0, :batch)

julia> getchunk(c, 1) # returns `nothing`

(jl_FoCmmm) pkg> st
Status `/tmp/jl_FoCmmm/Project.toml`
  [ae650224] ChunkSplitters v2.4.2

This can cause type-instabilities in situations where everything is in fact iterable. For example (now running with the development branch, with the https://github.com/JuliaFolds2/ChunkSplitters.jl/pull/37 PR merged already):

julia> using ChunkSplitters

julia> function mwe_size(ichunk=2, size=2, l=10)
           xs = collect(1:l)
           ys = collect(1:l)
           cx = getchunk(xs, ichunk; size=size, split=:batch)
           cy = getchunk(ys, ichunk; size=size, split=:batch)
           return Iterators.zip(cx, cy)
       end
mwe_size (generic function with 4 methods)

julia> @inferred mwe_size()
ERROR: return type Base.Iterators.Zip{Tuple{UnitRange{Int64}, UnitRange{Int64}}} does not match inferred return type Base.Iterators.Zip
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] top-level scope
   @ REPL[6]:1

(jl_1pA1nE) pkg> st
Status `/tmp/jl_1pA1nE/Project.toml`
  [ae650224] ChunkSplitters v2.4.3-DEV `~/.julia/dev/ChunkSplitters`

To solve this issue, we need to return, instead of nothing, an empty iterator. For :batch, return 0:-1, for :scatter, return 0:1:-1.

With this PR, we get:

julia> function mwe_size(ichunk=2, size=2, l=10)
           xs = collect(1:l)
           ys = collect(1:l)
           cx = getchunk(xs, ichunk; size=size, split=:batch)
           cy = getchunk(ys, ichunk; size=size, split=:batch)
           return Iterators.zip(cx, cy)
       end
mwe_size (generic function with 4 methods)

julia> @inferred mwe_size()
zip(3:4, 3:4)

It is a bit less elegant to return an arbitrary empty iterator rather than nothing, but that does not seem to justify the possibility of propagating a type instability.

lmiq commented 3 months ago

I have ran the OhMyThreads tests with this, and everything is fine there.

lmiq commented 3 months ago

@carstenbauer and/or @MasonProtter

Let me know if you see any issue with this PR, when possible. It is simple, the question is mostly about if you see any conceptual problem related to the change.