-
Notifications
You must be signed in to change notification settings - Fork 794
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
Array.chunkBySize does not return final chunk in some cases #503
Conversation
An example case is Array.chunkBySize 3 [|1..4|], the code calculates the number of chunks to be 2, and then does not copy the final (partial) chunk as 4 mod 2 is 0. Similarly for Array.chunkBySize 5 [|1..12|], number of chunks is 3 and 12 % 3 = 0 Changing check to len % chunkSize fixes this. Fixes dotnet#501
👍 though I don't think this will make it for VS 2015 😭 |
I can confirm:
|
@latkin so we will have a function with a major bug in VS 2015 release? It's nonsense. |
Fully agree with @vasily-kirichenko, it makes no sense to publish an absolutely new function which is known to be buggy to the wild. If we can't make it correct, maybe it's better to remove it? |
at first i thought this is only a very strange edge case that nobody will ever notice, but then again chunkBySize is a function that people will use to distrubute workloads on different nodes. |
The complete list of cases failing for chunkSize <= 10 is:
I think I agree with the idea that if this cannot be fixed for 4.0, then it should be dropped altogether. |
Luckily, I was too pessimistic in my estimate of the approval bar. This was just in time to still get a 👍 from shiproom. |
Yay.
|
Btw I now have nearly 100 property based tests (makes 10000 tests) and didn't spot a second bug yet. |
Greate news. |
@forki good to hear, and thanks again for the timely discovery |
Great to see this fixed in time! |
An example case is Array.chunkBySize 3 [|1..4|], the code calculates the
number of chunks to be 2, and then does not copy the final (partial) chunk
as 4 mod 2 is 0. Similarly for Array.chunkBySize 5 [|1..12|], number of
chunks is 3 and 12 % 3 = 0
Changing check to len % chunkSize fixes this.
Fixes #501