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

Array.chunkBySize does not return final chunk in some cases #503

Closed

Conversation

PatrickMcDonald
Copy link
Contributor

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

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
@forki
Copy link
Contributor

forki commented Jun 16, 2015

home

@latkin
Copy link
Contributor

latkin commented Jun 16, 2015

👍 though I don't think this will make it for VS 2015 😭

@forki
Copy link
Contributor

forki commented Jun 17, 2015

I can confirm:

Ok, passed 100 tests.

@vasily-kirichenko
Copy link
Contributor

@latkin so we will have a function with a major bug in VS 2015 release? It's nonsense.

@mexx
Copy link
Contributor

mexx commented Jun 17, 2015

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?

@forki
Copy link
Contributor

forki commented Jun 17, 2015

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.
If we release the current Array.chunkBySize we will see all kinds of strange NRE and data loss in the wild.

@PatrickMcDonald
Copy link
Contributor Author

The complete list of cases failing for chunkSize <= 10 is:

chunkSize: [| array lengths |]
3: [|4|]
4: [|6; 9|]
5: [|6; 8; 12; 16|]
6: [|8; 10; 15; 20; 25|]
7: [|8; 10; 12; 15; 18; 24; 30; 36|]
8: [|10; 12; 14; 18; 21; 28; 35; 42; 49|]
9: [|10; 12; 14; 16; 21; 24; 28; 32; 40; 48; 56; 64|]
10: [|12; 14; 16; 18; 21; 24; 27; 32; 36; 45; 54; 63; 72; 81|]

I think I agree with the idea that if this cannot be fixed for 4.0, then it should be dropped altogether.

@latkin
Copy link
Contributor

latkin commented Jun 18, 2015

The pattern of bad cases (black dots) vs good cases (green dots) is actually kind of interesting. The blue line is chunkSize = sqrt arrayLength

image

@forki
Copy link
Contributor

forki commented Jun 18, 2015

When you think that you are dealing with a small bug:

small

@latkin
Copy link
Contributor

latkin commented Jun 19, 2015

Luckily, I was too pessimistic in my estimate of the approval bar. This was just in time to still get a 👍 from shiproom.

@forki
Copy link
Contributor

forki commented Jun 19, 2015

Yay.
On Jun 19, 2015 21:07, "Lincoln Atkinson" notifications@github.com wrote:

Luckily, I was too pessimistic in my estimate of the approval bar. This
was just in time to still get a [image: 👍] from shiproom.


Reply to this email directly or view it on GitHub
#503 (comment)
.

@forki
Copy link
Contributor

forki commented Jun 19, 2015

Btw I now have nearly 100 property based tests (makes 10000 tests) and didn't spot a second bug yet.

@vasily-kirichenko
Copy link
Contributor

Greate news.

@latkin latkin closed this in 2f5d047 Jun 19, 2015
@latkin
Copy link
Contributor

latkin commented Jun 19, 2015

@forki good to hear, and thanks again for the timely discovery

@latkin latkin added the fixed label Jun 19, 2015
@PatrickMcDonald
Copy link
Contributor Author

Yes indeed, thanks @forki for the property test goodness! And thanks @latkin for the superb expectation management :D

@mariusschulz
Copy link

Great to see this fixed in time!

@PatrickMcDonald PatrickMcDonald deleted the fix-chunkBySize branch July 6, 2015 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants