| 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)  |
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) *  |
Date: 2013-09-12 05:58 |
| This is fixed in all versions. Thank you! |
|
|
| msg197753 - (view) |
Author: Georg Brandl (georg.brandl) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) |
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) * |
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) |
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) * |
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 |
|
|