The arguments of randint() and random_integers() do not broadcast.

(This is either a request for an enhancement, or a bug report--take your pick.)

Unlike most of the other distributions implemented in the RandomState class, randint and random_integers do not broadcast their arguments.

For example, hypergeometric broadcasts:

In [87]: rng = np.random.RandomState()

In [88]: rng.hypergeometric(10, [10, 20, 30], 10)
Out[88]: array([6, 2, 1])

But randint only accepts scalar integers:

In [89]: rng.randint(10, [10, 100])
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-89-b14fbd2d38cb> in <module>()
----> 1 rng.randint(10, [10, 100])

mtrand.pyx in mtrand.RandomState.randint (numpy/random/mtrand/mtrand.c:9556)()

TypeError: an integer is required

I might add that it is no better (if not worse) with random_integers:

>>> import numpy as np
>>> np.random.random_integers(10, [10, 100])
...
TypeError: can only concatenate list (not "int") to list

Fixing this issue should not be so bad to implement, as there already is a template for hypergeometric and a slew of other functions. The slightly tricky part will be the calling of random_integers from randint (see the master branch). But overall, a minor hitch.

@WarrenWeckesser

@WarrenWeckesser I'm going to close this, see the discussion at http://thread.gmane.org/gmane.comp.python.numeric.general/62195. If you can make a good case for the functionality, please open a new issue.

I ran into the problem while trying to make the arguments to the rvs method of the distributions in scipy.stats properly broadcast their arguments. If the status of this is "won't fix--too difficult", then so be it. It can be worked around.

I don't really understand the argument against supporting broadcasting for random integer generation. It's useful functionality and the semantics are natural and improve consistency with the rest of numpy. If no-one feels inspired to put in the leg work right at the moment then that's fine, of course, but shouldn't we leave this open in case someone does?

(Possibly even with the "easy fix" label)

@njsmith

@njsmith : Could we raise this topic in the discussion as a separate thread? Seems like this is a point of contention and with the major refactoring merged in, people could focus more on this functionality.

If people are in favor, my original PR (#6902) could be reopened, and I could give this another try (I have the original branch currently even with master).

@WarrenWeckesser

@WarrenWeckesser Which distributions require broadcasting support from randint()?

@gfyoung

@gfyoung It's fine to discuss it here on the original issue. Talking about 50% of the original request isn't a digression. If you have a functional branch with this feature implemented against master, go ahead and throw it up and we can see.

@njsmith I think the main complication is that there are multiple optional keywords to broadcast rather than a single set of always-present arguments. I'd hesitate to call that "easy fix" territory, but if @gfyoung already has it working, then yahtzee.

@rkern

@rkern: Currently, the _rvs method of the discrete distribution scipy.stats.randint simply passes its arguments to numpy's randint function: https://github.com/scipy/scipy/blob/master/scipy/stats/_discrete_distns.py#L649

scipy/scipy#5526 is a WIP pull request where I work around this by wrapping numpy's randint with numpy.vectorize.

Note that, despite the comments and the default value high=None in rvs, the method scipy.stats.randint.rvs currently doesn't allow the use of the default high value. That's either a bug, and it really should handle high=None, or it's a bug that the signature promises a default value for high which doesn't actually work, and the default should be removed. In the PR, I assume the latter.

In the PR, I assume the latter.

Actually, no. In the scipy PR, I do:

    if high is None:
        high = low
        low = 0

and then use broadcast_to and vectorize. But the PR is work-in-progress, and subject to change (when I get back to it).

@WarrenWeckesser

@WarrenWeckesser : Yikes! I was just reading the original scipy issue and your PR. That broadcasting issue is not pretty, especially given how generic the interface is.

@rkern : In light of #6910, my original PR, #6902 will need significant rewriting. I made the branch even with master so that I would have a clean slate to start with. I do agree that broadcasting all these optional arguments will not be easy. However, at least I only have to do it for randint (and not random_integers, which is what I did in my original PR).

@Everyone_Watching_This_Issue: While I do not have a working implementation going, my first thought was to follow in the footsteps of RandomState.hypergeometric, where discnmN_array_sc will correspond to every new function that @charris wrote, and then I can write slightly modified versions of those to obtain the discnmN_array equivalents. This will significantly enlarge the code-base (lots of copying/pasting), but it should keep everything from #6910 intact. Thoughts?

In any case, if there is a consensus to actually implement broadcasting in randint, could someone then reopen #6902?

I'd suggest starting fresh with a new PR instead of working off of #6902.

The main challenge I see is that the previous discussion settled on the requirement that high=2**64+1 must be supported as a valid value, and for broadcasting to work we need to put the arguments into arrays, but there is no integer array type that can hold the value 2**64+1.

One possible strategy for handling this while avoiding too much extra code complexity might be to add the closed=False option mentioned before, and have the setup logic look like:

if high is None:
    high = low
    low = 0
low = np.asarray(low, dtype=dtype)
# Make high into an object that can have "-= 1" applied to it, while retaining 2**64+1 as a valid input
if not isinstance(high, (np.ndarray, int)):
    high = np.array(high)
if not closed:
    high -= 1
    closed = True
high = np.asarray(high, dtype=dtype)
low, high = np.broadcast_arrays((low, high))
# Now low and high are definitely arrays of the requested dtype and broadcast shape,
# and they definitely specify a closed range, so the actual generation functions only
# need to handle this case

object would work just fine.

ENH: Allow randint to broadcast arguments

Just for the record, I just ran into this issue it is a weird inconsistency, that is currently making me use the almost certainly slower, and definitely less clear np.floor(np.random.uniform(low, high)). It would be great if the PR for this issue was merged.

xref #13163 . This is fixed in the randomgen branch.

Closing, the new API supports broadcasting and the endpoint kwarg (to allow low == high). The name has changed to integers, so the example becomes numpy.random.Generator().integers(10, [10, 100], endpoint=True). Please reopen if needed.

Navigation Menu

Repository navigation

The arguments of randint() and random_integers() do not broadcast. #6745

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

The arguments of randint() and random_integers() do not broadcast. #6745

Description

Activity

gfyoung commented on Dec 30, 2015

charris commented on Jan 3, 2016

WarrenWeckesser commented on Jan 3, 2016

njsmith commented on Jan 4, 2016

njsmith commented on Jan 4, 2016

gfyoung commented on Jan 4, 2016

rkern commented on Jan 4, 2016

rkern commented on Jan 4, 2016

WarrenWeckesser commented on Jan 4, 2016

WarrenWeckesser commented on Jan 4, 2016

gfyoung commented on Jan 4, 2016

rkern commented on Jan 4, 2016

njsmith commented on Jan 4, 2016

rkern commented on Jan 4, 2016

oscardssmith commented on Dec 10, 2017

bashtage commented on Apr 11, 2019

mattip commented on May 29, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions