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.repeat bug with powers of 32 sizes #176

Closed
TheSeamau5 opened this Issue Feb 16, 2015 · 7 comments

Comments

Projects
None yet
4 participants
@TheSeamau5
Contributor

TheSeamau5 commented Feb 16, 2015

For some reason, the following code:

import Array 
import List
import Text (asText)

main = 
  asText <|
    (Array.fromList (List.repeat 32 -740239360)) == (Array.repeat 32 -740239360)

yields False.

You can convince yourself that the repeat function is using the same number.

This was found using the following elm-check property:

property2 "Repeat works the same on lists and arrays" 
          (\n a -> Array.fromList (List.repeat n a) == Array.repeat n a) 
          (int 0 100) anyInt

which yielded:

Repeat works the same on lists and arrays has failed with the following input: (32,-740239360)

@TheSeamau5 TheSeamau5 changed the title from Array.repeat bug to Array.repeat bug with multiples of 32 sizes Feb 16, 2015

@TheSeamau5 TheSeamau5 changed the title from Array.repeat bug with multiples of 32 sizes to Array.repeat bug with powers of 32 sizes Feb 16, 2015

@TheSeamau5

This comment has been minimized.

Show comment
Hide comment
@TheSeamau5

TheSeamau5 Feb 16, 2015

Contributor

This bug seems to affect calling repeat with a size that is a power of 32

For example, the following yields true:

import Array 
import List
import Text (asText)

main = 
  asText <|
    (Array.fromList (List.repeat 31 1)) == (Array.repeat 31 1)

so, does this:

import Array 
import List
import Text (asText)

main = 
  asText <|
    (Array.fromList (List.repeat 33 1)) == (Array.repeat 33 1)

But these do not:

import Array 
import List
import Text (asText)

main = 
  asText <|
    (Array.fromList (List.repeat 32 1)) == (Array.repeat 32 1)
import Array 
import List
import Text (asText)

main = 
  asText <|
    (Array.fromList (List.repeat 1024 1)) == (Array.repeat 1024 1)

But this one works:

import Array 
import List
import Text (asText)

main = 
  asText <|
    (Array.fromList (List.repeat 0 1)) == (Array.repeat 0 1)
Contributor

TheSeamau5 commented Feb 16, 2015

This bug seems to affect calling repeat with a size that is a power of 32

For example, the following yields true:

import Array 
import List
import Text (asText)

main = 
  asText <|
    (Array.fromList (List.repeat 31 1)) == (Array.repeat 31 1)

so, does this:

import Array 
import List
import Text (asText)

main = 
  asText <|
    (Array.fromList (List.repeat 33 1)) == (Array.repeat 33 1)

But these do not:

import Array 
import List
import Text (asText)

main = 
  asText <|
    (Array.fromList (List.repeat 32 1)) == (Array.repeat 32 1)
import Array 
import List
import Text (asText)

main = 
  asText <|
    (Array.fromList (List.repeat 1024 1)) == (Array.repeat 1024 1)

But this one works:

import Array 
import List
import Text (asText)

main = 
  asText <|
    (Array.fromList (List.repeat 0 1)) == (Array.repeat 0 1)
@mgold

This comment has been minimized.

Show comment
Hide comment
@mgold

mgold Feb 17, 2015

Contributor

Array equality seems to have some issues, and the same fix may resolve a host of problems. In particular, it seems to be comparing tree representations rather than the abstract flattened lists.

Contributor

mgold commented Feb 17, 2015

Array equality seems to have some issues, and the same fix may resolve a host of problems. In particular, it seems to be comparing tree representations rather than the abstract flattened lists.

@TheSeamau5

This comment has been minimized.

Show comment
Hide comment
@TheSeamau5

TheSeamau5 Feb 18, 2015

Contributor

Cool! Looking forward to the fix.

Contributor

TheSeamau5 commented Feb 18, 2015

Cool! Looking forward to the fix.

@mgold

This comment has been minimized.

Show comment
Hide comment
@mgold

mgold Feb 18, 2015

Contributor

I don't have a fix, but a proper implementation of array equality ought to fix both issues.

Contributor

mgold commented Feb 18, 2015

I don't have a fix, but a proper implementation of array equality ought to fix both issues.

@TheSeamau5

This comment has been minimized.

Show comment
Hide comment
@TheSeamau5

TheSeamau5 Feb 18, 2015

Contributor

Oh...ok... That doesn't matter. I'll still look forward to a fix.

Contributor

TheSeamau5 commented Feb 18, 2015

Oh...ok... That doesn't matter. I'll still look forward to a fix.

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Sep 8, 2015

Contributor

The bug is here is to do with the differences between how initialize creates an array, and how fromList generates an array. fromList will deal with the case where length % M == 0 without adding a parent node. initialize however adds a parent node.

E.g

Array.fromList <| List.repeat n 1

will generate

Object {ctor: "_Array", height: 1, table: Array[32], lengths: Array[32]}

whereas

Array.repeat n 1

will generate

Object {ctor: "_Array", height: 2, table: Array[1], lengths: Array[1]}

Whether or not they should generate equally-deep trees or not I don't know, but the fact they don't highlighted this issue.

The Utils.js file assumingly should be comparing Arrays element-by-element, so a fix in tree generation wouldn't be enough to close this issue.

Contributor

eeue56 commented Sep 8, 2015

The bug is here is to do with the differences between how initialize creates an array, and how fromList generates an array. fromList will deal with the case where length % M == 0 without adding a parent node. initialize however adds a parent node.

E.g

Array.fromList <| List.repeat n 1

will generate

Object {ctor: "_Array", height: 1, table: Array[32], lengths: Array[32]}

whereas

Array.repeat n 1

will generate

Object {ctor: "_Array", height: 2, table: Array[1], lengths: Array[1]}

Whether or not they should generate equally-deep trees or not I don't know, but the fact they don't highlighted this issue.

The Utils.js file assumingly should be comparing Arrays element-by-element, so a fix in tree generation wouldn't be enough to close this issue.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Sep 9, 2015

Member

It looks like the root issue here is using (==) on arrays. This is known to be weird on sets, dicts, and arrays where you do not actually want structural equality, you only care about the leafs.

If that is the root, close this and create a more focused issue about equality on arrays. If there are other relevant issues, please link them here and I will create a meta issue.

Member

evancz commented Sep 9, 2015

It looks like the root issue here is using (==) on arrays. This is known to be weird on sets, dicts, and arrays where you do not actually want structural equality, you only care about the leafs.

If that is the root, close this and create a more focused issue about equality on arrays. If there are other relevant issues, please link them here and I will create a meta issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment