Skip to content

np.random.randint no longer supports non-native byteorder #13159

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

Closed
QuLogic opened this issue Mar 19, 2019 · 8 comments · Fixed by #13655
Closed

np.random.randint no longer supports non-native byteorder #13159

QuLogic opened this issue Mar 19, 2019 · 8 comments · Fixed by #13655

Comments

@QuLogic
Copy link
Contributor

QuLogic commented Mar 19, 2019

This used to work in NumPy 1.15:

Reproducing code example:

$ conda create -n np115 python=3.6 numpy=1.15
$ conda activate np115
$ conda list numpy
# packages in environment at /var/container/conda/envs/np115:
#
# Name                    Version                   Build  Channel
numpy                     1.15.4          py36h8b7e671_1002    conda-forge
>>> import numpy as np

>>> np.random.seed(0)
>>> print(np.random.randint(0, 200, size=10, dtype='<i4'))
[172  47 117 192  67 195 103   9  21  36]

>>> np.random.seed(0)
>>>print(np.random.randint(0, 200, size=10, dtype='>i4'))
[172  47 117 192  67 195 103   9  21  36]

vs.

$ conda create np116 python=3 numpy=1.16
$ conda activate np116
$ conda list numpy
# packages in environment at /var/container/conda/envs/np116:
#
# Name                    Version                   Build  Channel
numpy                     1.16.2           py36h8b7e671_1    conda-forge

Error message:

Traceback (most recent call last):
  File "mtrand.pyx", line 975, in mtrand.RandomState.randint
KeyError: dtype('>i4')

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "mtrand.pyx", line 977, in mtrand.RandomState.randint
TypeError: Unsupported dtype ">i4" for randint

Numpy/Python version information:

Python 3.6.7; NumPy 1.15.4 vs 1.16.2

@QuLogic
Copy link
Contributor Author

QuLogic commented Mar 19, 2019

Probably from #11324; cc @eric-wieser.

@bashtage
Copy link
Contributor

I think your example is proof that it silently ignored the byte order, not that it "worked". The current implementation is more honest in stating that nonnative byte order is not supported.

For example,

np.random.seed(0)
a = np.random.randint(256,dtype='u1',size=4)
np.random.seed(0)
b = np.random.randint(256**4, dtype='u4', size=1)
np.random.seed(0)
c = np.random.randint(256**4, dtype='>u4', size=1)
np.random.seed(0)
d = np.random.randint(256**4, dtype='<u4', size=1)
print(a.view('<u4'), b, c, a.view('>u4'), d)

[2357136044] [2357136044] [2357136044] [2886369164] [2357136044]

It is worked, then the final 2 values would be identical.

@eric-wieser
Copy link
Member

@bashtage is right, the order was ignored

>>> np.random.randint(0, 200, size=10, dtype='>i4').dtype.byteorder
'='
>>> np.random.randint(0, 200, size=10, dtype='<i4').dtype.byteorder
'='

However, this seems pretty par-for-the-course for numpy:

>>> np.add(1, 2, dtype='>i4').dtype.byteorder
'='
>>> np.add(1, 2, dtype='<i4').dtype.byteorder
'='

I think a patch that explicitly discards the byteorder (via newbyteorder('=')) before doing the dictionary lookup would be fine.

@eric-wieser eric-wieser added this to the 1.16.3 release milestone Mar 23, 2019
@QuLogic
Copy link
Contributor Author

QuLogic commented Mar 24, 2019

It is worked, then the final 2 values would be identical.

@bashtage you are correct that byteorder is ignored, but this check is not correct. That is not the behaviour that's desired, nor is it consistent with other creation functions. Specifying byteorder does not change values, it changes storage. It's clearer to compare the result of .tobytes:

In [2]: np.ones(3, dtype='>i4').tobytes()
Out[2]: b'\x00\x00\x00\x01\x00\x00\x00\x01\x00\x00\x00\x01'

In [3]: np.ones(3, dtype='<i4').tobytes()
Out[3]: b'\x01\x00\x00\x00\x01\x00\x00\x00\x01\x00\x00\x00'

vs

In [8]: np.random.seed(0); np.random.randint(200, size=10, dtype='>i4').tobytes()
Out[8]: b'\xac\x00\x00\x00/\x00\x00\x00u\x00\x00\x00\xc0\x00\x00\x00C\x00\x00\x00\xc3\x00\x00\x00g\x00\x00\x00\t\x00\x00\x00\x15\x00\x00\x00$\x00\x00\x00'

In [9]: np.random.seed(0); np.random.randint(200, size=10, dtype='<i4').tobytes()
Out[9]: b'\xac\x00\x00\x00/\x00\x00\x00u\x00\x00\x00\xc0\x00\x00\x00C\x00\x00\x00\xc3\x00\x00\x00g\x00\x00\x00\t\x00\x00\x00\x15\x00\x00\x00$\x00\x00\x00'

@charris charris modified the milestones: 1.16.3 release, 1.16.4 release Apr 22, 2019
@charris
Copy link
Member

charris commented May 27, 2019

Ping to everyone :)

@charris charris modified the milestones: 1.16.4 release, 1.16.5 release May 27, 2019
@bashtage
Copy link
Contributor

bashtage commented May 27, 2019 via email

@seberg
Copy link
Member

seberg commented May 27, 2019

We could argue that it should go through deprecation before going to raise (I think raise is probably slightly better then silent ignoring), but I am not sure this is a big enough issue to worry much. Of course it would also be good to allow non-native order.

@bashtage
Copy link
Contributor

This is a patch in mattip#36 that produces a FutureWarning on this. It isn't a bug in the randomgen branch and would have silently run without doing any byte swapping. IMO it is better to leave this to users rather than push a .byteswap down into the function. The warning says as much.

bashtage added a commit to bashtage/numpy that referenced this issue May 28, 2019
Warn that non-native byte order is not supported in randint and integers

closes numpy#13159
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants