8000 ENH: Expand Argument Range of random_integers by gfyoung · Pull Request #6885 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

ENH: Expand Argument Range of random_integers #6885

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 28, 2015
Merged

ENH: Expand Argument Range of random_integers #6885

merged 1 commit into from
Dec 28, 2015

Conversation

gfyoung
Copy link
Contributor
@gfyoung gfyoung commented Dec 24, 2015

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
Copy link
Member
charris commented Dec 24, 2015

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

@gfyoung
Copy link
Contributor Author
gfyoung commented Dec 24, 2015

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
Copy link
Contributor Author
gfyoung commented Dec 24, 2015

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

@charris
Copy link
Member
charris commented Dec 24, 2015

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

@gfyoung
Copy link
Contributor Author
gfyoung commented Dec 24, 2015

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
charris reviewed Dec 28, 2015
View reviewed changes
numpy/random/mtrand/mtrand.pyx
rv = lo + <long>rk_interval(diff, self. internal_state)
array_data[i] = rv
return array
return self.random_integers(low, high-1, size)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space around -.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing.
8000

@charris
Copy link
Member
charris commented Dec 28, 2015

LGTM modulo a couple of nits.

@gfyoung
Copy link
Contributor Author
gfyoung commented Dec 28, 2015

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
Copy link
Contributor Author
gfyoung commented Dec 28, 2015

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

@charris
Copy link
Member
charris commented Dec 28, 2015
8000

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

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

@gfyoung
Copy link
Contributor Author
gfyoung commented Dec 28, 2015

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

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.
@gfyoung
Copy link
Contributor Author
gfyoung commented Dec 28, 2015

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
Copy link
Member
charris commented Dec 28, 2015

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
Copy link
Member
njsmith commented Dec 28, 2015

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
Copy link
Contributor Author
gfyoung commented Dec 28, 2015

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

@gfyoung
Copy link
Contributor Author
gfyoung commented Dec 28, 2015

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

charris added a commit that referenced this pull request Dec 28, 2015
@charris
Merge pull request #6885 from gfyoung/rand_integers_expand
efd5fbe 
ENH: Expand Argument Range of random_integers
@charris charris merged commit efd5fbe into numpy:master Dec 28, 2015
@charris
Copy link
Member
charris commented Dec 28, 2015

In it goes. Thanks @gfyoung .

This was referenced Dec 28, 2015
Closed
Closed
@gfyoung gfyoung deleted the rand_integers_expand branch December 28, 2015 22:39
@gfyoung gfyoung mentioned this pull request Dec 31, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
01 - Enhancement component: numpy.random
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gfyoung @charris @njsmith
0