1. I can't seem to build numpy off master anymore because it keeps complaining about not being able to find the "Advapi32" library in any directories (an empty list). Is anyone else having this issue, or is it just me? I scanned the file in my System32 folder and ran sfc /scannow as Administrator, and everything came back clean.

2. As we are starting fresh from BUG: Broadcast Arguments in Random Integer Generation #6902, I'll pose the question again, should we allow a function call like the one below?

np.random.randint(5, [10, None])

3. I will be adding tests and updating documentation / release notes later. I'm hoping to get feedback on the overall organization of my current changes for now.

I can't seem to build numpy off master anymore because it keeps complaining about not being able to find the "Advapi32" library in any directories (an empty list).

No idea, sorry :-( Very few devs use Windows; there's a reasonable chance that you're the only one watching this PR who does. You might try the mailing list; that has a broader readership...

should we allow a function call like [...] np.random.randint(5, [10, None])

No. You should think about the signature as being randint(low=0, high); the fact that None even appears as a possible value is a technical detail of how we hack that argument signature into working in a language that doesn't ordinarily allow optional arguments to proceed mandatory arguments.

In fact it would probably be a good idea to benchmark the code with and without the scalar special case; I guess it might well be worth it in this case, but in general I am wary of such fast-paths because they add a lot of code and maintenance complexity and don't always help as much as you'd think...

@njsmith : Regarding the np.random.randint(5, [10, None]), sounds good. Just wanted to double check. Regarding my issue building numpy, I sent out an email about it. Hopefully, I can get some help. Google will have to be my guide FTTB.

True, I did add a ton of code that is quite similar to what was already written in #6910. However, I was hoping to try to keep most of those changes intact FTTB. Also, it seemed consistent with what other functions did like RandomState.hypergeometric. Though if there's a faster and more condensed way of doing this, do let me know!

Right, so, it may well be that the way you've written this is the best way, even with the code duplication between the scalar and array versions. But what I'm saying is that we should make that decision based on whether or not the code duplication actually gets us some valuable benefit. Probably "it's substantively faster in important cases" -- that's the usual one. OTOH "It lets us avoid touching existing code", and "it makes the code more similar to existing code" aren't very good reasons IMHO. I'm just saying we should check our assumptions while implementing things.

@njsmith : Fair enough. I'll try to do some benchmarking once I can get these tests fixed.

@everyone : Could someone explain why / how I get the following output? This is why I'm getting test failures at the moment:

>>> import numpy as np
>>> high = np.array([np.iinfo(np.int64).max + 1])
>>> low = np.array([np.iinfo(np.int64).max])
>>> high
array([92223372036854775808], dtype=uint64)
>>> low
array([92223372036854775807], dtype=int64)
>>> high[0] > low[0]
False
>>> high[0] == low[0]
True
>>> high[0] > np.uint64(low[0])
True

FYI, 64 Ubuntu VM with Python 2.7.11 (using that FTTB until I can get my Windows issue resolved).

Yeah, when you do uint64 > int64, it's trying to cast both to a common
type, and since there's no integer type whose range is a superset of both
uint64 and int64, it goes to float64 instead. And then in float64, 264+1
isn't representable and gets rounded off to 264, so they're equal rather
than >.
.
This is a bit of an ugly mess, and it's not at all clear that the
cast-to-float64 thing is the right design (maybe they should just error? I
dunno), but that's how it works now.

On Tue, Jan 5, 2016 at 7:19 PM, gfyoung notifications@github.com wrote:

@njsmith https://github.com/njsmith : Fair enough. I'll try to do some
benchmarking once I can these tests fixed.

@everyone https://github.com/Everyone : Could someone explain why I
get the following output? Is numpy casting the uint64 as int64 or
something behind the scenes when I do the comparison? This is why I'm
getting test failures:

import numpy as np
high = np.array([np.iinfo(np.int64) + 1])
low = np.array([np.iinfo(np.int64)])
high
array([92223372036854775808], dtype=uint64)
low
array([92223372036854775807], dtype=int64)
high[0] > low[0]
False
high[0] > np.uint64(low[0])
True

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

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

Oh, okay. Good to know. I'll try to figure out some nice way to work around that FTTB. BTW, just for future reference, where is the code that does all that casting / comparison you just described? I might want to poke around there afterwards given what you just described.

Actually, it occurs to me now that that is a bug in randint as it is currently implemented in master, as demonstrated below:

>>> import numpy as np
>>> low = np.int64(np.iinfo(np.int64).max)
>>> high = np.uint64(np.iinfo(np.int64).max + 1)
>>> np.random.randint(low, high)
Traceback (most recent call last):
...
ValueError: low >= high

array > array dispatches to the ndarray comparison operator, which in turn
calls the ufunc np.greater. The casting logic is then the standard casting
logic that's built into the generic ufunc machinery, somewhat scattered
around the umath/ directory.

On Tue, Jan 5, 2016 at 8:39 PM, gfyoung notifications@github.com wrote:

Oh, okay. Good to know. I'll try to figure out some nice way to work
around that FTTB. BTW, just for future reference, where is the code that
does all that casting / comparison you just described? I might want to poke
around there afterwards given what you just described.

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

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

@njsmith : Thanks! Has this complication been brought up before? Otherwise, I'll go log it as a potential issue.

It's certainly been discussed on and off, but possibly only in passing in
situations like this... I don't know if there's a central issue for it.
It's one of those squirrelly things where it's not even clear whether or
not there is a bug, or what the next step is.

On Tue, Jan 5, 2016 at 8:54 PM, gfyoung notifications@github.com wrote:

@njsmith https://github.com/njsmith : Thanks! Has this complication
been brought up before? Otherwise, I'll go log it as a potential issue.

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

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

I'm also +1 on investigating ways of merging the scalar and array versions into a single one. Having 8 almost identical functions is already a big enough maintainability problem, to make it twice as big.

@everyone: Indeed, I am totally in favor of condensing this code. Once, I can get a working implementation down, that's certainly the next step for this PR!

Okay, tests are passing now! I had to do some major overhaul to condense the code and get it to work properly. @jaimefrio : your suggestion to use np.broadcast actually fixed the weird corner cases the C API for some reason could not handle. In any case, I'm going to do some further condensing later, but it would be great if I could get some more feedback on what currently has been done now that Travis and Appveyor are happy.

It seems your changes to the private functions' signature broke a ton of code that was accessing them directly.

I don't think anything outside of numpy.random is accessing them directly. It's just randint().

Whoops. I forgot to put is_scalar in randint's call to rand_func. Not sure if that will fix all the test failures though. In any case, at least it's a separate commit, so if it all blows up, I'll have something to fall back on. Not sure how necessary the is_scalar parameter is. I could easily determine it within each rand_func, though it seems redundant in light of what I do in randint.

Massive segfaulting on Travis - resetting to try again

So I've tried condensing the code, but whatever I've done, it's certainly slowed down Travis. At least no seg-faulting though! Besides perhaps reverting to C API calls OR creating a fast-track for the purely scalar inputs, does anyone have suggestions for speeding the code up?

Also, my changes have broken several tests. Suggestions about where my code is going wrong are also welcomed too (I'm not quite sure what is going wrong at the moment), especially with regards to the repeat-ability test @charris wrote.

I just ran some timeit tests, and it does seem like the fast-track (@njsmith) is indeed much faster. I ran python -m timeit -n 100 "import numpy as np;np.random.randint(0, 20, size=1000000)" and got the following:

master branch:
100 loops, best of 3: 27.4 msec per loop

My branch (with scalar / array_like computations all condensed into one method):
100 loops, best of 3: 356 msec per loop

Thus, I think I will roll-back my most recent commit but port over the changes I made to the documentation as well as the tests if that's okay with everyone.

I seem to ecall this was discussed somewhere else, but cannot find exactly where... Say I have a low array of shape (3,), and a high array of shape (4,), and I want to get 5 samples from each of the 3 * 4 = 12 possible combinations. Should I call it like this:

np.random.randint(low[:, np.newaxis], high, (5,)) # 5 instead of (5,) should also work

Or do I have to do

np.random.randint(low[:, np.newaxis], high, (3, 4, 5))

☔ The latest upstream changes (presumably #9015) made this pull request unmergeable. Please resolve the merge conflicts.

I think this will work as is, but don't want to put it in without modification. The reason is that I think it can be made more efficient for the broadcasting case, and, once it goes in, we will need to keep it pretty much the same in order to maintain backward compatibility of the generated sequences. The improvements I have in mind are:

1. Make the scalar case depend on the length of low and high having size one as long as the shapes are compatible. That would be more efficient for the array case and also ensure that the generated sequence wouldl be the same in those cases, probably the result of least surprise.
2. Put the loop for the non-scalar case into the called C function. I think that will require new rk_*functions. In the true broadcasting case, we might also want to pass the masks as well as the rng and off so that the masks do not need regeneration each time through the inner loop, although I suppose that could be done inside the function itself if storage for the masks is passed. One could then loop through the inner rng loop until the output is filled. Note that, because random integers are acquired from the rng in 32 bit chunks with left over bits discarded, putting the loop in the called function instead of using multiple function calls will change the generated sequence.
3. The generated sequences should be checked in a few broadcasting cases to help ensure that they remain the same.
4. The lack of an easy full range option is a bit bothersome. Not sure what to do about that, either live with it or what.

I also have a quick question: what determine the integer type, the dtype or the low, high type?

@charris : To answer your about what determines integer type, it's dtype (you implemented that yourself IINM). As to the other questions, I'll address your points one at a time:

1. I'm not sure I follow you here. Could you clarify what you mean by this?

2. Okay, I think I understand. I can take a stab at transferring that for-loop code into C.

3. I'm not sure I fully understand this statement. Could you clarify what you're suggesting? It seems like you're suggesting an additional test in which we check the consistency of numbers generated under different broadcasting conditions, but I'm not sure.

4. I in fact actually made an attempt to do this back in ENH: Make 'low' optional in randint #7151 but received major push-back for it.

I in fact actually made an attempt to do this back in #7151 but received major push-back for it.

That PR is referring to [intmin, intmax), whereas I think @charris is concerned about about [intmin, intmax] or [intmin, intmax+1), which you address for only the scalar case in #8846. There doesn't seem to be an easy way to extend this to the broadcast case.

@eric-wieser : Fair enough, but if I were to modify the PR to just set highbnd = intmax + 1, the patch is essentially the same. The question is whether or not such a change is desirable.

@gfyoung For the size one case, I think low= [1], high=[3] should produce the same sequence as low=1, high=3 when the output shape is the same and compatible. For the scalar case the loop filling the output is in the rk_* function, which will produce a different sequence than if the rk_* function is called multiple times to fill the output array.

Okay...but we do want to differentiate how we return the data (scalar or array-like), which is why we treat [1] and [3] differently in the first place. I suppose then we would have to check if we had array-like inputs to determine the return type?

Also, I'm not sure I fully follow your concern here about sequence generation:

>>> import numpy as np
>>> randint = np.random.randint
>>>
>>> np.random.seed(123456789)
>>> for _ in range(10):
...     print(randint(1, 500))
185
157
307
474
347
286
493
264
251
229
>>>
>>> np.random.seed(123456789)
>>> for _ in range(10):
...     print(randint([1], [500]))
array([185])
array([157])
array([307])
array([474])
array([347])
array([286])
array([493])
array([264])
array([251])
array([229])

I'm not sure I fully follow your concern here about sequence generation:

Can you try that with other dtypes?

@eric-wieser : See this script I wrote: generate.txt

Feel free to change LOW, HIGH, and DTYPE as you please. I didn't see any differences.

The options I see for dealing with full range bounds are to add a closed option or to require the user to use a type of sufficient range.

The options I see for dealing with full range bounds are to add a closed option or to require the user to use a type of sufficient range.

Is there any reason why just specifying dtype (with no bounds) can't allow us to generate the full range? It's not necessary to add closed because you can explicitly pass in the full range [intmin, intmax + 1), and this just adds unnecessary args to the signature IMO.

You need to use a smaller dtype. To illustrate the difference between single and multiple calls:

In [1]: randint = np.random.randint

In [2]: np.random.seed(123456789)

In [3]: a = randint(1, 20, 10, dtype=np.int8)

In [4]: np.random.seed(123456789)

In [5]: b = array([randint(1, 20, dtype=np.int8) for _ in range(10)])

In [6]: a
Out[6]: array([ 8,  9, 15, 19, 14,  9,  4, 18, 19,  3], dtype=int8)

In [7]: b
Out[7]: array([ 8, 15, 19,  4, 18,  4, 13,  8, 15,  5], dtype=int8)

Note that only the first elements agree, because both cases start with the same 32 bit int, but in the first case each 32 bit int supplies 4 bytes, whereas in the second case each 32 bit int supplies only one.

@charris : What you're showing is not the same invocation as before (in fact, you can see this on master IINM). Perhaps this for-loop inconsistency should be patched before this PR?

Is there any reason why just specifying dtype (with no bounds) can't allow us to generate the full range?

That would work for the scalar case, but not be backwards compatible. It is the broadcast case that makes thing difficult because of the conversion to arrays.

That would work for the scalar case, but not be backwards compatible.

I'm not sure I follow why that is the case. We're allowing users to not have to pass in a parameter that used to be required.

☔ The latest upstream changes (presumably #9026) made this pull request unmergeable. Please resolve the merge conflicts.

☔ The latest upstream changes (presumably #9106) made this pull request unmergeable. Please resolve the merge conflicts.

I was hoping to be able to return to this at some point, but I've gotten too caught up in other (repository) work. Closing for now. If others would like to provide assistance to push this through or would like to pick it up themselves, they are more welcome!

For reference, here is the comment from which anyone should start to see where the PR needs to go.