classification
Title: add "Do not supply 'int' argument" to random.shuffle docstring
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.3, Python 3.4, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: eric.araujo, ezio.melotti, georg.brandl, michael.driscoll, orsenthil, python-dev, rhettinger, sandro.tosi, smichr, tim.peters
Priority: low Keywords: easy, patch

Created on 2012-05-27 15:44 by smichr, last changed 2013-10-06 06:44 by smichr. This issue is now closed.

Files
File name Uploaded Description Edit
shuffle.patch michael.driscoll, 2012-06-11 19:49 shuffle method docstring patch review
14927.diff orsenthil, 2013-09-16 01:31 review
14927-2.diff orsenthil, 2013-09-16 02:07 review
Messages (24)
msg161710 - (view) Author: Christopher Smith (smichr) Date: 2012-05-27 15:44
In randrange there is the note, "Do not supply the 'int' argument." That could probably be added to shuffle, too.
msg161760 - (view) Author: Christopher Smith (smichr) Date: 2012-05-28 07:40
----
> nosy: +rhettinger
> title: add not about int to shuffle -> add "Do not supply int argument" to random.shuffle docstring
> versions: +Python 2.7, Python 3.3
>

Thanks. I couldn't even figure out what my own subject meant! (I see
that the "e" was missing from "note"; yours is much better.

Chris
msg162638 - (view) Author: Michael Driscoll (michael.driscoll) * Date: 2012-06-11 19:49
I added the extra information to the docstring for the shuffle method and attached a patch.
msg162639 - (view) Author: Christopher Smith (smichr) Date: 2012-06-11 19:50
On Tue, Jun 12, 2012 at 1:34 AM, Michael Driscoll
<report@bugs.python.org> wrote:
>
> Michael Driscoll <mike@pythonlibrary.org> added the comment:
>
> I added the extra information to the docstring for the shuffle method and attached a patch.

Thanks Michael (and Python team)!

Chris
msg197513 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-09-12 05:57
New changeset 82bdd5fc7a71 by Senthil Kumaran in branch '2.7':
Improve the docstring of random.shuffle. Inform users not to provide int arg.
http://hg.python.org/cpython/rev/82bdd5fc7a71

New changeset 4782faf29480 by Senthil Kumaran in branch '3.3':
Improve the docstring of random.shuffle. Inform users not to provide int arg.
http://hg.python.org/cpython/rev/4782faf29480

New changeset 15096b93ae5a by Senthil Kumaran in branch 'default':
merge from 3.3
http://hg.python.org/cpython/rev/15096b93ae5a
msg197514 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2013-09-12 05:58
This is fixed in all versions. Thank you!
msg197753 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2013-09-15 07:06
I would propose a leading underscore for these methods; they should make it clear to the user that the parameter is meant to be "private".

With this line in the docstring, I would wonder "Why is the argument there in the first place?!" (because most people don't know about the optimization technique).
msg197847 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2013-09-16 00:06
> Georg Brandl added the comment:
>
> I would propose a leading underscore for these methods; they should make it clear to the user that the parameter is meant to be "private".

+1 to this proposal.
This style is present with randrange, randbelow and shuffle.

> With this line in the docstring, I would wonder "Why is the argument there in the first place?!" (because most people don't know about the optimization technique).

This style has been present since the introduction of these functions.
One attempt to clarify / cleanup shuffle here is: 29430:b80a22250e4f
Georg :  I am unaware of the optimization technique you refer to as
well, it will helpful if you could point to any resource.
msg197848 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2013-09-16 00:17
[Senthil Kumaran]
> I am unaware of the optimization technique you refer to as
> well, it will helpful if you could point to any resource.

It's an old trick since the very first Pythons:  global lookups are
much slower than local lookups (the difference between the LOAD_GLOBAL
and LOAD_FAST opcodes in CPython).  Putting:

     ..., _fast=slow, ...

in an argument list means we endure the slow lookup (of `slow`) only
once, when the function is first defined.  When the function is
_called_, that binding is available via the local (much faster lookup)
variable `_fast`.

Purely a speed trick, but can make a real difference in very heavily
used functions.

And it's a Good Idea to put a leading underscore on such arguments.
It's never intended that users pass values for them.
msg197849 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2013-09-16 00:36
Tim Peters added the comment:

>      ..., _fast=slow, ...
>
> in an argument list means we endure the slow lookup (of `slow`) only
> once, when the function is first defined.  When the function is
> _called_, that binding is available via the local (much faster lookup)
> variable `_fast`.
>
> Purely a speed trick, but can make a real difference in very heavily
> used functions.

Thanks for the answer, Tim. Interesting optimization.
msg197856 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2013-09-16 01:31
Attaching a patch after changing int=int to _int = int and improving the docstring. Please review the changes to the docstring and see if it will be helpful.
msg197859 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2013-09-16 01:44
I wouldn't add info about the optimization in the docstring.

In _randbelow() I think you missed a call to int().
For _randbelow(), all arguments after "int" are non-public ones.
(_randbelow as a private function wouldn't necessarily need the change, but it's good for consistency.)

Final decision should be made by Raymond.
msg197860 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2013-09-16 02:07
Thanks for catching the mistake at _randbelow. Updated patch to fix that and removed the explanation in the docstring. Not sure if _randbelow should changed (fully) or not at all. Leaving the change only with _int.
Will wait for Raymond's review.
msg199019 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2013-10-06 00:33
I think the "Do not supply the 'int' argument" covers it well enough.  This code has been around for a very long time and isn't causing any problems.
msg199020 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2013-10-06 00:37
Hi Raymond, Ezio provided some comments on improvements to the patch. Do you mind if Ezio or I take over task of improvement. Not cause problems != no need to improve.

TIA.
msg199021 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2013-10-06 00:42
I don't think there is an actual problem here to be solved.
msg199022 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2013-10-06 00:50
I'm old, but I liked the docs better when they didn't mention "the int argument" at all.  The "int=int" - or "_int=int" - argument is a CPython implementation detail.  It has nothing to do with the API.  And _of course_ users shouldn't mess with implementation details.  99.9+% will never notice the argument is there, and the fraction that do notice should infer that they shouldn't mess with it from that it's _not_ documented.
msg199023 - (view) Author: Christopher Smith (smichr) Date: 2013-10-06 03:51
I probably wouldn't have noticed it except I was working more intensely
with the different random methods and saw that randrange had the note about
not supplying the 'int' argument and shuffle (though it had the same sort
of argument) did *not* have the comment. So that raised the issue for me.
Proabably the best thing would be do remove the comment from randrange and
make sure that the not-to-mess-with args are made private with the
underscore.

/c
msg199025 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-10-06 04:35
New changeset b1e94e332ec8 by Raymond Hettinger in branch '2.7':
Issue 14927: Minor clean-up of function parameters in random().
http://hg.python.org/cpython/rev/b1e94e332ec8
msg199026 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2013-10-06 04:54
Py3.3: http://hg.python.org/cpython/rev/0899960835f5
Py3.4: http://hg.python.org/cpython/rev/8494d2c8ef54
msg199027 - (view) Author: Christopher Smith (smichr) Date: 2013-10-06 05:02
In 3.3 and 3.4 I would just change the shuffle arg from `int=int` to
`_int=int` and delete the comment in docstring regarding not supplying the
value. (In both you *removed* the argument and internally added `_int=int`.)

Note that (as far as I can see) in 3.3 you didn't remove the comment in the
docstring of shuffle like you did in 3.4

/c

On Sat, Oct 5, 2013 at 11:54 PM, Raymond Hettinger
<report@bugs.python.org>wrote:

>
> Raymond Hettinger added the comment:
>
> Py3.3: http://hg.python.org/cpython/rev/0899960835f5
> Py3.4: http://hg.python.org/cpython/rev/8494d2c8ef54
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue14927>
> _______________________________________
>
msg199029 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-10-06 05:12
New changeset 50ea4dccb03e by Raymond Hettinger in branch '3.3':
Issue 14927: Remove a docstring line that is no longer applicable.
http://hg.python.org/cpython/rev/50ea4dccb03e
msg199030 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2013-10-06 05:14
Christopher, this tracker item needs to die.  It is wasting everyone's time (and churning code) over nothing.

FYI, I moved the _int=int for shuffle inside the function because the assignment was outside of the inner loop, so we weren't getting any real benefit.
msg199032 - (view) Author: Christopher Smith (smichr) Date: 2013-10-06 06:44
On Sun, Oct 6, 2013 at 12:14 AM, Raymond Hettinger
<report@bugs.python.org>wrote:

>
> Raymond Hettinger added the comment:
>
> Christopher, this tracker item needs to die.  It is wasting everyone's
> time (and churning code) over nothing.
>
>
but it's not quite dead yet...

> FYI, I moved the _int=int for shuffle inside the function because the
> assignment was outside of the inner loop, so we weren't getting any real
> benefit.
>

but cf Tim's comment regarding the advantage of leaving it in the arg list
so that the lookup is fast:

[Senthil Kumaran]
> I am unaware of the optimization technique you refer to as
> well, it will helpful if you could point to any resource.

It's an old trick since the very first Pythons:  global lookups are
much slower than local lookups (the difference between the LOAD_GLOBAL
and LOAD_FAST opcodes in CPython).  Putting:

     ..., _fast=slow, ...

in an argument list means we endure the slow lookup (of `slow`) only
once, when the function is first defined.  When the function is
_called_, that binding is available via the local (much faster lookup)
variable `_fast`.

Purely a speed trick, but can make a real difference in very heavily
used functions.

----------

So by removing it from the arg list you have perhaps caused a regression in
performance of shuffle. Other than that, everything looks fine to me.

Thanks,
 Chris
History
Date User Action Args
2013-10-06 06:44:45smichrsetmessages: + msg199032
2013-10-06 05:14:44rhettingersetmessages: + msg199030
2013-10-06 05:12:01python-devsetmessages: + msg199029
2013-10-06 05:02:16smichrsetmessages: + msg199027
2013-10-06 04:54:02rhettingersetmessages: + msg199026
2013-10-06 04:35:03python-devsetmessages: + msg199025
2013-10-06 03:51:29smichrsetmessages: + msg199023
2013-10-06 00:50:13tim.peterssetmessages: + msg199022
2013-10-06 00:42:37rhettingersetmessages: + msg199021
2013-10-06 00:37:14orsenthilsetmessages: + msg199020
2013-10-06 00:34:00rhettingersetstatus: open -> closed

messages: + msg199019
2013-10-05 23:12:32ezio.melottisetnosy: + ezio.melotti

versions: + Python 3.4, - Python 3.2
2013-09-16 02:07:29orsenthilsetfiles: + 14927-2.diff

messages: + msg197860
2013-09-16 01:44:02georg.brandlsetstatus: pending -> open

messages: + msg197859
2013-09-16 01:31:22orsenthilsetstatus: closed -> pending
files: + 14927.diff
messages: + msg197856
2013-09-16 00:36:08orsenthilsetmessages: + msg197849
2013-09-16 00:17:23tim.peterssetnosy: + tim.peters
messages: + msg197848
2013-09-16 00:06:06orsenthilsetmessages: + msg197847
2013-09-15 07:06:21georg.brandlsetnosy: + georg.brandl
messages: + msg197753
2013-09-12 05:58:59orsenthilsetstatus: open -> closed

type: behavior

nosy: + orsenthil
messages: + msg197514
resolution: fixed
stage: needs patch -> resolved
2013-09-12 05:57:22python-devsetnosy: + python-dev
messages: + msg197513
2012-06-12 01:13:40rhettingersetpriority: normal -> low
2012-06-12 01:12:37rhettingersetassignee: rhettinger
2012-06-11 19:50:12smichrsetmessages: + msg162639
2012-06-11 19:49:02michael.driscollsetfiles: + shuffle.patch

nosy: + michael.driscoll
messages: + msg162638

keywords: + patch
2012-06-10 01:37:33eric.araujosetkeywords: + easy
nosy: + sandro.tosi

stage: needs patch
2012-06-01 18:05:17eric.araujosetnosy: + eric.araujo

title: add "Do not supply int argument" to random.shuffle docstring -> add "Do not supply 'int' argument" to random.shuffle docstring
2012-05-28 07:40:17smichrsetmessages: + msg161760
2012-05-28 06:29:32mark.dickinsonsetnosy: + rhettinger
title: add not about int to shuffle -> add "Do not supply int argument" to random.shuffle docstring

versions: + Python 2.7, Python 3.3
2012-05-27 15:44:08smichrcreate