From f2a869491c86a98f1e9b416a62f0833a10658a38 Mon Sep 17 00:00:00 2001 From: Matthias Goergens Date: Mon, 8 Aug 2022 11:25:54 +0800 Subject: [PATCH 1/6] Fix corner case in _randbelow_with_getrandbits In random._randbelow_with_getrandbits we are generating a random integer in [0,n). We do that by repeatedly drawing a random number, r, in [0,2**k) until we hit an r < n. That's a great strategy. It's not only very simple, but even with worst case input, we only need to draw 2 random numbers on average, before we hit the jackpot. However, the code is a bit overly cautious. When n is a power of 2, we ask `getrandbits` to sample from [0,2*n]. The code justifies that strange behaviour with a comment expressing fear of the special case n==1. That fear is unfounded: the obvious code works just fine for n==1. Fixing this not only makes the code simpler to understand, it also guarantees that the important special case of n being a power of two now has a guaranteed worst case runtime of only a single call to getrandbits; instead of an expected runtime of two calls with no worst case bound. Existing tests already cover this code and the special cases of interest. So we don't need any new tests. This minor performance bug was introduced in 0515661314c4e5b9235e07b2c46b8f456c7fadc3 as far as I can tell. But I can't really tell what the original author was thinking. --- Lib/random.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/random.py b/Lib/random.py index 76627309e288b9..f82803715b479a 100644 --- a/Lib/random.py +++ b/Lib/random.py @@ -239,7 +239,7 @@ def _randbelow_with_getrandbits(self, n): "Return a random int in the range [0,n). Defined for n > 0." getrandbits = self.getrandbits - k = n.bit_length() # don't use (n-1) here because n can be 1 + k = (n-1).bit_length() r = getrandbits(k) # 0 <= r < 2**k while r >= n: r = getrandbits(k) From cf6e90791ac6879864879e3ca389a13407e4b580 Mon Sep 17 00:00:00 2001 From: Matthias Goergens Date: Mon, 8 Aug 2022 12:42:33 +0800 Subject: [PATCH 2/6] Revert "Fix corner case in _randbelow_with_getrandbits" This reverts commit f2a869491c86a98f1e9b416a62f0833a10658a38. --- Lib/random.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/random.py b/Lib/random.py index f82803715b479a..76627309e288b9 100644 --- a/Lib/random.py +++ b/Lib/random.py @@ -239,7 +239,7 @@ def _randbelow_with_getrandbits(self, n): "Return a random int in the range [0,n). Defined for n > 0." getrandbits = self.getrandbits - k = (n-1).bit_length() + k = n.bit_length() # don't use (n-1) here because n can be 1 r = getrandbits(k) # 0 <= r < 2**k while r >= n: r = getrandbits(k) From 4e917295077159bf3a0c83b1298dec3896568d40 Mon Sep 17 00:00:00 2001 From: Matthias Goergens Date: Mon, 8 Aug 2022 12:45:53 +0800 Subject: [PATCH 3/6] Explain why we can't have nice things --- Lib/random.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Lib/random.py b/Lib/random.py index 76627309e288b9..63e2a8a3043efc 100644 --- a/Lib/random.py +++ b/Lib/random.py @@ -239,7 +239,13 @@ def _randbelow_with_getrandbits(self, n): "Return a random int in the range [0,n). Defined for n > 0." getrandbits = self.getrandbits - k = n.bit_length() # don't use (n-1) here because n can be 1 + # For efficiency, we'd like to use k = (n-1).bit_length() here, but + # before Python 3.9, `getrandbits()` did not accept 0 as an argument. + # That's why using `n-1` was a problem when `n` was 1. + # + # Now we are stuck with this version, because we want to reproduce + # results after explicitly setting a seed even between releases. + k = n.bit_length() r = getrandbits(k) # 0 <= r < 2**k while r >= n: r = getrandbits(k) From e72a4469ef150c5a6053fbdf03204d06dc14d16a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20G=C3=B6rgens?= Date: Mon, 8 Aug 2022 18:55:23 +0800 Subject: [PATCH 4/6] Remove explanatory comment For efficiency, we'd like to use k = (n-1).bit_length() here, but before Python 3.9, `getrandbits()` did not accept 0 as an argument. That's why using `n-1` was a problem when `n` was 1. Now we are stuck with this version, because we want to reproduce results after explicitly setting a seed even between releases. --- Lib/random.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/Lib/random.py b/Lib/random.py index 63e2a8a3043efc..c70294ee0cbf08 100644 --- a/Lib/random.py +++ b/Lib/random.py @@ -239,12 +239,6 @@ def _randbelow_with_getrandbits(self, n): "Return a random int in the range [0,n). Defined for n > 0." getrandbits = self.getrandbits - # For efficiency, we'd like to use k = (n-1).bit_length() here, but - # before Python 3.9, `getrandbits()` did not accept 0 as an argument. - # That's why using `n-1` was a problem when `n` was 1. - # - # Now we are stuck with this version, because we want to reproduce - # results after explicitly setting a seed even between releases. k = n.bit_length() r = getrandbits(k) # 0 <= r < 2**k while r >= n: From 7d8b8048ba9e3eace84e5e142fac24683ecbc2e5 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Mon, 8 Aug 2022 10:57:33 +0000 Subject: [PATCH 5/6] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Documentation/2022-08-08-10-57-32.gh-issue-13485.xtWoE8.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Documentation/2022-08-08-10-57-32.gh-issue-13485.xtWoE8.rst diff --git a/Misc/NEWS.d/next/Documentation/2022-08-08-10-57-32.gh-issue-13485.xtWoE8.rst b/Misc/NEWS.d/next/Documentation/2022-08-08-10-57-32.gh-issue-13485.xtWoE8.rst new file mode 100644 index 00000000000000..18ac9a5daa992a --- /dev/null +++ b/Misc/NEWS.d/next/Documentation/2022-08-08-10-57-32.gh-issue-13485.xtWoE8.rst @@ -0,0 +1 @@ +Remove obsolete comment in _randbelow_with_getrandbits From 1c47666bcd1b39b1efe24a081daf5140e043f584 Mon Sep 17 00:00:00 2001 From: Raymond Hettinger Date: Mon, 8 Aug 2022 16:48:38 -0500 Subject: [PATCH 6/6] Delete 2022-08-08-10-57-32.gh-issue-13485.xtWoE8.rst Comment changes don't warrant a comment. --- .../Documentation/2022-08-08-10-57-32.gh-issue-13485.xtWoE8.rst | 1 - 1 file changed, 1 deletion(-) delete mode 100644 Misc/NEWS.d/next/Documentation/2022-08-08-10-57-32.gh-issue-13485.xtWoE8.rst diff --git a/Misc/NEWS.d/next/Documentation/2022-08-08-10-57-32.gh-issue-13485.xtWoE8.rst b/Misc/NEWS.d/next/Documentation/2022-08-08-10-57-32.gh-issue-13485.xtWoE8.rst deleted file mode 100644 index 18ac9a5daa992a..00000000000000 --- a/Misc/NEWS.d/next/Documentation/2022-08-08-10-57-32.gh-issue-13485.xtWoE8.rst +++ /dev/null @@ -1 +0,0 @@ -Remove obsolete comment in _randbelow_with_getrandbits