search

JuliaFolds2 / ChunkSplitters.jl

Splitting collections into chunks
https://juliafolds2.github.io/ChunkSplitters.jl/
MIT License
50 stars 7 forks source link

Unit range for batch #37

Closed lmiq closed 4 months ago

lmiq commented 4 months ago

This PR tries to solve the performance problem associated with returning a StepRange from the :batch splitting option.

See: https://github.com/JuliaFolds2/ChunkSplitters.jl/issues/35

What I do here is make the split type, :batch or :scatter a type-parameter in all except the top-level calls of chunks and getchunk. And with that return different types of ranges for :batch or :scatter, because having a UnitRange is important for the performance if the computation is performed on views and are very tight.

Seems to me that this is at the end an overall improvement.

codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 94.33962% with 3 lines in your changes missing coverage. Please review.

Project coverage is 91.16%. Comparing base (959f9db) to head (3d2dedb).

Files Patch % Lines
src/ChunkSplitters.jl 94.33% 3 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 #37 +/- ## ========================================== - Coverage 94.01% 91.16% -2.86% ========================================== Files 2 2 Lines 234 249 +15 ========================================== + Hits 220 227 +7 - Misses 14 22 +8 ```

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

lmiq commented 4 months ago

@carstenbauer and @MasonProtter

This may affect the performance (for better, I hope) of OhMyThreads.jl, so I would prefer you both to take a look at it before merging.

Thanks.

lmiq commented 4 months ago

I've now ran all the examples of OhMyThreads and did not find any meaningful performance difference with this PR.

Also ran the OhMyThreads tests, an all pass.

Tortar commented 4 months ago

Thanks for the work @lmiq, I'm not one of the mantainers but to me the change seems good :+1:

lmiq commented 4 months ago

I'll just wait for the input of one of Carlsten or Mason, because they are maintaing OhMyThreads and probably it is through it that this package is being used the most. I don't want to introduce problems there.

carstenbauer commented 4 months ago

I've skimmed the changes and they seem fine to me.