ENH: Expand Argument Range of random_integers by gfyoung · Pull Request #6885 · numpy/numpy

gfyoung · 2015-12-24T06:15:08Z

Redistributes the code between the randint and random_integers methods so that we can generate integers up to and including np.iinfo('l').max with random_integers, which previously would have caused an OverflowError.

Previous behavior:

>>> import numpy as np
>>> np.random.random_integers(np.iinfo('l').max, np.iinfo('l').max)
Traceback (most recent call last):
  File "<pyshell#1>", line 1, in <module>
    np.random.random_integers(np.iinfo('l').max, np.iinfo('l').max)
  File "mtrand.pyx", line 1446, in mtrand.RandomState.random_integers (numpy\random\mtrand\mtrand.c:12401)
  File "mtrand.pyx", line 942, in mtrand.RandomState.randint (numpy\random\mtrand\mtrand.c:9558)
OverflowError: Python int too large to convert to C long

New behavior:

>>> import numpy as np
>>> np.random.random_integers(np.iinfo('l').max, np.iinfo('l').max) # 32-bit Python
2147483647

(Optional, But Recommended) Depends on #6824

While the PR currently allows for the generation of up to and including np.iinfo('l').max, if #6824 is merged in, the PR will be revised to being able to generate random integers up to and including np.iinfo('ll').max.

charris · 2015-12-24T17:58:24Z

I believe it is currently np.iinfo('l').max. the 'l' is the C long.

gfyoung · 2015-12-24T18:46:25Z

Ah, yes, in light of the underlying code, semantically that is correct. However, it seems that np.iinfo('l').max equals np.iinfo(np.int).max, at least on the machine/VMs I have access to.

gfyoung · 2015-12-24T19:24:28Z

@charris: Updated with the wording fixes. Travis is still happy.

charris · 2015-12-24T19:48:15Z

Yes, it turns out that 'int' is the same as old python 2 'int', which was a long. The c int is 'intc'.

gfyoung · 2015-12-24T19:51:23Z

Fair enough, though I agree with your point that we should use np.iinfo('l') instead np.iinfo(np.int) to keep it consistent with the underlying code.

charris · 2015-12-28T18:55:52Z

numpy/random/mtrand/mtrand.pyx

Space around -.

Sure thing.

charris · 2015-12-28T19:10:29Z

LGTM modulo a couple of nits.

gfyoung · 2015-12-28T19:13:30Z

In light of the discussion in #6869, where does this PR stand in terms of being merged? As #6824 and #6869 will probably not be merged in for some time, once all of the smaller fixes have been made, can the changes in the PR go in as is? Or will additional work be needed?

gfyoung · 2015-12-28T20:07:50Z

Made changes, and Travis is happy. From your comments above, should be good to merge.

charris · 2015-12-28T21:45:43Z

Did you intend to include the _configtest.exe.manifest file?

This looks independent of your other PR, so will be good to go.

gfyoung · 2015-12-28T21:46:34Z

Yikes! No, did not intend to! Let me remove that.

Redistributes the code between the randint and random_integers methods so that we can generate integers up to and including np.iinfo('l').max with random_integers, which previously would have caused an OverflowError.

gfyoung · 2015-12-28T21:50:09Z

I think I must have accidentally did git add -A when I was making those small changes you recommended. Got to remember it's git commit -a -m <message> for this repo!

charris · 2015-12-28T21:54:30Z

I find it safer to do git commit -a without the message because I can verify the files being committed when the editor pops up.

njsmith · 2015-12-28T21:56:26Z

BTW the bar for adding things to .gitignore is pretty low -- if there's
stuff that would help, say, you and one other person if it were added, then
feel free to submit a PR :-)
(You might also want to read up on .git/info/exclude --
https://help.github.com/articles/ignoring-files/#explicit-repository-excludes
)

On Mon, Dec 28, 2015 at 1:50 PM, gfyoung notifications@github.com wrote:

I think I must have accidentally did git add -A when I was making those
small changes you recommended. Got to remember it's git commit -a -m
for this repo!

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

Nathaniel J. Smith -- http://vorpus.org

gfyoung · 2015-12-28T21:58:08Z

@charris : Ah, fair enough. Better safe than sorry.
@njsmith : Thanks for the info!

gfyoung · 2015-12-28T22:17:05Z

Travis gives a green light, so now the PR should be good to go.

ENH: Expand Argument Range of random_integers

charris · 2015-12-28T22:28:33Z

In it goes. Thanks @gfyoung .

charris added 01 - Enhancement component: numpy.random labels Dec 28, 2015

charris reviewed Dec 28, 2015
View reviewed changes

ENH: Allow random_integers to include the maximum np.iinfo('l').max

d7a68af

Redistributes the code between the randint and random_integers methods so that we can generate integers up to and including np.iinfo('l').max with random_integers, which previously would have caused an OverflowError.

charris added a commit that referenced this pull request Dec 28, 2015

Merge pull request #6885 from gfyoung/rand_integers_expand

efd5fbe

ENH: Expand Argument Range of random_integers

charris merged commit efd5fbe into numpy:master Dec 28, 2015

This was referenced Dec 28, 2015

Allow for larger int64s in random.randint #6824

Closed

Add dtypes for random number generation #6869

Closed

gfyoung deleted the rand_integers_expand branch December 28, 2015 22:39

gfyoung mentioned this pull request Dec 31, 2015

ENH: Add dtype argument to random.randint. #6910

Merged

Uh oh!

Conversation

gfyoung commented Dec 24, 2015

(Optional, But Recommended) Depends on #6824

Uh oh!

charris commented Dec 24, 2015

Uh oh!

gfyoung commented Dec 24, 2015

Uh oh!

gfyoung commented Dec 24, 2015

Uh oh!

charris commented Dec 24, 2015

Uh oh!

gfyoung commented Dec 24, 2015

Uh oh!

charris Dec 28, 2015

Choose a reason for hiding this comment

Uh oh!

gfyoung Dec 28, 2015

Choose a reason for hiding this comment

Uh oh!

charris commented Dec 28, 2015

Uh oh!

gfyoung commented Dec 28, 2015

Uh oh!

gfyoung commented Dec 28, 2015

Uh oh!

charris commented Dec 28, 2015

Uh oh!

gfyoung commented Dec 28, 2015

Uh oh!

gfyoung commented Dec 28, 2015

Uh oh!

charris commented Dec 28, 2015

Uh oh!

njsmith commented Dec 28, 2015

Uh oh!

gfyoung commented Dec 28, 2015

Uh oh!

gfyoung commented Dec 28, 2015

Uh oh!

charris commented Dec 28, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants