[issue33203] random.choice: raise IndexError on empty sequence even when not using getrandbits internally

[Raymond Hettinger]

Change by Raymond Hettinger :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33203] random.choice: raise IndexError on empty sequence even when not using getrandbits internally

[Raymond Hettinger]

Raymond Hettinger  added the comment:


New changeset e25af930a300c01aa043745058a8c7f6c32d89ae by Raymond Hettinger 
(Miss Islington (bot)) in branch '3.6':
bpo-33203: Ensure random.choice always raises IndexError on empty sequence 
(GH-6338) (GH-6388)
https://github.com/python/cpython/commit/e25af930a300c01aa043745058a8c7f6c32d89ae


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33203] random.choice: raise IndexError on empty sequence even when not using getrandbits internally

[Raymond Hettinger]

Raymond Hettinger  added the comment:


New changeset baf304e82e1b54dbeee6b78ddf168e33ed8d557a by Raymond Hettinger 
(Miss Islington (bot)) in branch '3.7':
bpo-33203: Ensure random.choice always raises IndexError on empty sequence 
(GH-6338) (GH-6387)
https://github.com/python/cpython/commit/baf304e82e1b54dbeee6b78ddf168e33ed8d557a


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33203] random.choice: raise IndexError on empty sequence even when not using getrandbits internally

[miss-islington]

Change by miss-islington :


--
pull_requests: +6097

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33203] random.choice: raise IndexError on empty sequence even when not using getrandbits internally

[miss-islington]

Change by miss-islington :


--
pull_requests: +6096

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33203] random.choice: raise IndexError on empty sequence even when not using getrandbits internally

[Raymond Hettinger]

Raymond Hettinger  added the comment:


New changeset 091e95e9004b794280ab35becec2c3e30dd5e96e by Raymond Hettinger 
(Wolfgang Maier) in branch 'master':
bpo-33203: Ensure random.choice always raises IndexError on empty sequence 
(GH-6338)
https://github.com/python/cpython/commit/091e95e9004b794280ab35becec2c3e30dd5e96e


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33203] random.choice: raise IndexError on empty sequence even when not using getrandbits internally

[Raymond Hettinger]

Raymond Hettinger  added the comment:

The patch looks fine.  Once a news blurb is added, it can go forward.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33203] random.choice: raise IndexError on empty sequence even when not using getrandbits internally

[Raymond Hettinger]

Change by Raymond Hettinger :


--
Removed message: https://bugs.python.org/msg314956

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33203] random.choice: raise IndexError on empty sequence even when not using getrandbits internally

[Raymond Hettinger]

Raymond Hettinger  added the comment:

I think this can go forward as-is.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33203] random.choice: raise IndexError on empty sequence even when not using getrandbits internally

[Wolfgang Maier]

Wolfgang Maier  added the comment:

@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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33203] random.choice: raise IndexError on empty sequence even when not using getrandbits internally

[Wolfgang Maier]

Wolfgang Maier  added the comment:

@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 I thought the real reason why _randbelow does not perform the check was that 
it would be redundant, which it isn't when only random is used.

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.
It is also worthwhile to remember that any performance impact of this will only 
hit subclasses of random.Random that define random, but not getrandbits. Your 
alternative idea of having random.choice catch (ValueError, ZeroDivisionError) 
would affect random.Random and all subclasses.
If a subclass defines getrandbits, it needs to perform that check anyway  and, 
thinking about this more, I suggest that the requirement for any user-provided 
getrandbits to raise ValueError on k <= 0 should actually be added to the 
getrandbits docs.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33203] random.choice: raise IndexError on empty sequence even when not using getrandbits internally

[Raymond Hettinger]

Raymond Hettinger  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33203] random.choice: raise IndexError on empty sequence even when not using getrandbits internally

[Raymond Hettinger]

Raymond Hettinger  added the comment:

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.

--
assignee:  -> rhettinger

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33203] random.choice: raise IndexError on empty sequence even when not using getrandbits internally

[Michael Selik]

Michael Selik  added the comment:

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):
   ...: def random(self):
   ...: return  super().random()
   ...:

In [3]: r = Random()

In [4]: r._randbelow(-1) # Should raise a ValueError
Out[4]: 0

But honestly, if someone is going to override ``random`` I think we can expect 
them to realize they're stepping into the deep end.

--
nosy: +selik

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33203] random.choice: raise IndexError on empty sequence even when not using getrandbits internally

[Wolfgang Maier]

Change by Wolfgang Maier :


--
keywords: +patch
pull_requests: +6050
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33203] random.choice: raise IndexError on empty sequence even when not using getrandbits internally

[Wolfgang Maier]

New submission from Wolfgang Maier :

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 "", line 1, in 
  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 "", line 1, in 
  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.

--
components: Library (Lib)
messages: 314787
nosy: rhettinger, serhiy.storchaka, wolma
priority: normal
severity: normal
status: open
title: random.choice: raise IndexError on empty sequence even when not using 
getrandbits internally
type: behavior
versions: Python 3.6, Python 3.7, Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com