JuliaFolds2 / ChunkSplitters.jl

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

default n,size to nothing, to avoid type-instability #33

Closed lmiq closed 5 months ago

lmiq commented 5 months ago

I found that the introduction of the size option created a potentially type-instability in the construction of Chunk, because the chunks function did not specialize to the Constraint parameter. To fix that, I made the defaults of n and size to be nothing, such that the final Constraint type of Chunk is known from their types.

Before:

julia> @inferred chunks(1:7; n=4)
ERROR: return type ChunkSplitters.Chunk{UnitRange{Int64}, ChunkSplitters.FixedCount} does not match inferred return type ChunkSplitters.Chunk{UnitRange{Int64}}
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] top-level scope
   @ REPL[2]:1

Now:

julia> @inferred chunks(1:7; n=4)
ChunkSplitters.Chunk{UnitRange{Int64}, ChunkSplitters.FixedCount}(1:7, 4, 0, :batch)
lmiq commented 5 months ago

@carstenbauer

If you want to comment, please let me know. I think this is an important change. I was getting quite important type instabilities in trivial examples because of this.

Otherwise I'll just merge it asap.

codecov-commenter commented 5 months ago

Codecov Report

Attention: Patch coverage is 93.10345% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 94.39%. Comparing base (9cf0250) to head (f1be5e9).

Files Patch % Lines
src/ChunkSplitters.jl 93.10% 2 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #33 +/- ## ========================================== + Coverage 93.21% 94.39% +1.18% ========================================== Files 2 2 Lines 221 232 +11 ========================================== + Hits 206 219 +13 + Misses 15 13 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.