bpo-40282: Allow random.getrandbits(0) by pitrou · Pull Request #19539 · python/cpython · GitHub

https://bugs.python.org/issue40282

Note I've enhanced this test a bit to check that all bits take both values 0 and 1 accross 100 draws.

We can use (n-1) now.

Suggested change

      
                    k = n.bit_length()  # don't use (n-1) here because n can be 1
          
                    k = (n-1).bit_length()

I think we want to ensure that the values produced for a given seed don't change, so we probably can't change this anymore.

Maybe use assert?

This is the same error as in the other implementation (the one without getrandbits).

Seems this check causes a performance regression.

Is there any situation where a user could specifically encounter the above ValueError from the public methods? If not, I think an assertion would make more sense here; seeing as it's a private member that's only being used from within the random module. IMO, the same applies to _randbelow_without_getrandbits().

Also, how significant is the performance regression @serhiy-storchaka?

This makes shuffle() about 6% slower:

python -m timeit -r11 -s 'from random import shuffle' -s 's=list(range(1000))' 'shuffle(s)'

random.Random has a surprising API. The base class is Mersenne Twister which implements getrandbits(). Inheritance is non-trivial. Does your change work if a subclass implements _randbelow()? What if it implements random()? See Random.__init_subclass__().

_randbelow() is not designed for overriding. If there is a problem with this, it can be made private (__randbelow()).

If the subclass implements random() but not getrandbits(), its methods will use random() instead of getrandbits().

Co-Authored-By: Serhiy Storchaka <storchaka@gmail.com>

Co-Authored-By: Victor Stinner <vstinner@python.org>

I rebased and applied two trivial changes from reviewing. Will merge if CI is green. Thanks everyone!

random.Random has a surprising API. The base class is Mersenne Twister which implements getrandbits(). Inheritance is non-trivial. Does your change work if a subclass implements _randbelow()? What if it implements random()? See Random.init_subclass().

I created bpo-40346: "Redesign random.Random class inheritance".

* master: (1985 commits) bpo-40179: Fix translation of #elif in Argument Clinic (pythonGH-19364) bpo-35967: Skip test with `uname -p` on Android (pythonGH-19577) bpo-40257: Improve help for the typing module (pythonGH-19546) Fix two typos in multiprocessing (pythonGH-19571) bpo-40286: Use random.randbytes() in tests (pythonGH-19575) bpo-40286: Makes simpler the relation between randbytes() and getrandbits() (pythonGH-19574) bpo-39894: Route calls from pathlib.Path.samefile() to os.stat() via the path accessor (pythonGH-18836) bpo-39897: Remove needless `Path(self.parent)` call, which makes `is_mount()` misbehave in `Path` subclasses. (pythonGH-18839) bpo-40282: Allow random.getrandbits(0) (pythonGH-19539) bpo-40302: UTF-32 encoder SWAB4() macro use a|b rather than a+b (pythonGH-19572) bpo-40302: Replace PY_INT64_T with int64_t (pythonGH-19573) bpo-40286: Add randbytes() method to random.Random (pythonGH-19527) bpo-39901: Move `pathlib.Path.owner()` and `group()` implementations into the path accessor. (pythonGH-18844) bpo-40300: Allow empty logging.Formatter.default_msec_format. (pythonGH-19551) bpo-40302: Add pycore_byteswap.h header file (pythonGH-19552) bpo-40287: Fix SpooledTemporaryFile.seek() return value (pythonGH-19540) Minor modernization and readability improvement to the tokenizer example (pythonGH-19558) bpo-40294: Fix _asyncio when module is loaded/unloaded multiple times (pythonGH-19542) Fix parameter names in assertIn() docs (pythonGH-18829) bpo-39793: use the same domain on make_msgid tests (python#18698) ...

pitrou requested a review from rhettinger as a code owner April 15, 2020 08:24

the-knights-who-say-ni added the CLA signed label Apr 15, 2020

bedevere-bot added the awaiting core review label Apr 15, 2020

pitrou requested a review from mdickinson April 15, 2020 08:24

pitrou commented Apr 15, 2020

View reviewed changes

serhiy-storchaka reviewed Apr 15, 2020

View reviewed changes

Comment thread Doc/library/random.rst Outdated

Comment thread Lib/random.py Outdated

Comment thread Lib/random.py Outdated

serhiy-storchaka reviewed Apr 15, 2020

View reviewed changes

Comment thread Modules/_randommodule.c Outdated

vstinner reviewed Apr 15, 2020

View reviewed changes

Comment thread Lib/test/test_random.py Outdated

Comment thread Doc/library/random.rst Outdated

Comment thread Doc/library/random.rst Outdated

rhettinger approved these changes Apr 15, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Apr 15, 2020

rhettinger reviewed Apr 15, 2020

View reviewed changes

Comment thread Lib/random.py Outdated

pitrou and others added 6 commits April 17, 2020 18:22

bpo-40282: Allow random.getrandbits(0)

f572b53

Update Doc/library/random.rst

3153f1a

Co-Authored-By: Serhiy Storchaka <storchaka@gmail.com>

Ensure _randbelow implementations raise on 0 input.

9cac35f

s/positive/non-negative/

b01be91

Revert changes to choice()

e7c2e99

Update Lib/test/test_random.py

5e02ef8

Co-Authored-By: Victor Stinner <vstinner@python.org>

pitrou force-pushed the bpo40282-getrandbits0 branch from 9ccb7bc to 3b86eb8 Compare April 17, 2020 16:26

Apply review comments

c4673f0

pitrou force-pushed the bpo40282-getrandbits0 branch from 3b86eb8 to c4673f0 Compare April 17, 2020 17:11

pitrou merged commit 75a3378 into python:master Apr 17, 2020

bedevere-bot removed the awaiting merge label Apr 17, 2020

pitrou deleted the bpo40282-getrandbits0 branch April 17, 2020 17:32

	k = n.bit_length() # don't use (n-1) here because n can be 1
	k = (n-1).bit_length()

bpo-40282: Allow random.getrandbits(0) by pitrou · Pull Request #19539 · python/cpython · GitHub

Repository navigation

bpo-40282: Allow random.getrandbits(0) - #19539

bpo-40282: Allow random.getrandbits(0)#19539pitrou merged 7 commits intopython:masterpython/cpython:masterfrom pitrou:bpo40282-getrandbits0pitrou/cpython:bpo40282-getrandbits0Copy head branch name to clipboard

Conversation

pitrou commented Apr 15, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

pitrou Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

pitrou Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

aeros Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rhettinger commented Apr 15, 2020

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka commented Apr 15, 2020

Uh oh!

Uh oh!

pitrou commented Apr 17, 2020

Uh oh!

vstinner commented Apr 20, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

bpo-40282: Allow random.getrandbits(0)#19539
pitrou merged 7 commits into
python:masterfrom
pitrou:bpo40282-getrandbits0

pitrou commented Apr 15, 2020 •
edited by bedevere-bot
Loading

aeros Apr 15, 2020 •
edited
Loading

vstinner commented Apr 20, 2020 •
edited by bedevere-bot
Loading