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
random.choice: raise IndexError on empty sequence even when not using getrandbits internally #77384
Comments
from https://docs.python.org/3/library/random.html#random.choice: Return a random element from the non-empty sequence seq. If seq is empty, raises IndexError. Indeed:
>>> import random
>>> random.choice([])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/wolma/cpython/random.py", line 259, in choice
raise IndexError('Cannot choose from an empty sequence') from None
IndexError: Cannot choose from an empty sequence
but when not using getrandbits internally:
>>> class MyRandom(random.Random):
... def random(self):
... return super().random()
...
>>> my_random=MyRandom()
>>> my_random.choice([])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/wolma/cpython/random.py", line 257, in choice
i = self._randbelow(len(seq))
File "/home/wolma/cpython/random.py", line 245, in _randbelow
rem = maxsize % n
ZeroDivisionError: integer division or modulo by zero This is because the ValueError that random.choice tries to catch gets raised only in the getrandbits-dependent branch of Random._randbelow, but not in the branch using only Random.random (even though Random._randbelow suggests uniform behaviour. |
If you're going to tackle this problem, this should probably be solved for the general case of n <= 0 rather than just n == 0. In [1]: import random In [2]: class Random(random.Random): In [3]: r = Random() In [4]: r._randbelow(-1) # Should raise a ValueError But honestly, if someone is going to override |
FWIW, it was intended that the error checking (when required) occur at higher levels in the API rather than in the inner-most non-public utility function. Some calls to _randbelow cannot be zero or negative, so they shouldn't have to pay an penalty for the extra error check. A comment to this effect should be added but I don't think the design should be changed. |
I'll mull this over for while. There is some merit in getting the various components to fit together more uniformly. There's also the option of having choice() catch either a ZeroDivisionError or ValueError. |
@rhettinger: the reason the ValueError gets raised correctly in the getrandbits-dependent branch is because getrandbits itself does a n<=0 check (in C for random.Random, in Python for random.SystemRandom). So instead of thinking about the suggested check as something that impacts performance (which certainly it does), I would rather see it as adding something into that second branch that was forgotten and gave that branch a performance benefit over the other one, which it never deserved. |
@selik: it's true _randbelow doesn't work for negative numbers, but the difference is that both branches are affected, the docstring does not make any guarantees about it, and no public part of the random module is affected by this behavior. In addition, "fixing" _randbelow for negative input cannot be done without impacting performance of several public methods of random.Random so I don't think this should be done. |
The patch looks fine. Once a news blurb is added, it can go forward. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: