classification
Title: add random.pop()
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: Julian, ezio.melotti, john.feuerstein, mark.dickinson, rhettinger
Priority: normal Keywords: patch

Created on 2011-09-09 00:11 by john.feuerstein, last changed 2011-09-09 23:45 by rhettinger. This issue is now closed.

Files
File name Uploaded Description Edit
random_pop.diff john.feuerstein, 2011-09-09 00:10 add random.pop() review
Messages (6)
msg143748 - (view) Author: John Feuerstein (john.feuerstein) * Date: 2011-09-09 00:10
This patch against current hg tip (72314:92842e347d98) adds random.pop():

    pop(self, seq) method of Random instance
        Remove and return a random element from a non-empty sequence.

Includes test case.
msg143753 - (view) Author: Julian Berman (Julian) * Date: 2011-09-09 03:30
Considering this is pretty easily written more or less as

    r = range(20)
    r.pop(random.randrange(0, len(r)))

is it really worth adding?
msg143759 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2011-09-09 12:04
> is it really worth adding?

Probably not.  For improvements along these lines, I'd rather see random.choice support sets:

>>> random.choice({1, 2})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Library/Frameworks/Python.framework/Versions/7.1/lib/python2.7/random.py", line 274, in choice
    return seq[int(self.random() * len(seq))]  # raises IndexError if seq is empty
TypeError: 'set' object does not support indexing

But that's another (rejected) issue; see issue 7522.
msg143760 - (view) Author: John Feuerstein (john.feuerstein) * Date: 2011-09-09 13:41
> r.pop(random.randrange(0, len(r)))

So seq[random.randrange(0, len(seq))] suddenly makes random.choice(seq) obsolete? There is no functional reasoning here, it's convenience for a common operation.

> Not all the sequences have a .pop() method.

Not all sequences support indexing or item assignment either:

>>> random.choice({1, 2})
TypeError: 'set' object does not support indexing
>>> random.shuffle("abc")
TypeError: 'str' object does not support item assignment
...

> I'd rather see random.choice support sets: ...
> But that's another (rejected) issue; see issue 7522.

So the implicit requirement for random.choice() is support for indexing.
The implicit requirement for random.shuffle() is support for indexing and item assignment. The implicit requirement for random.pop() is support for seq.pop()...

I don't think random's convenience functions should validate (or even worse, convert) input. It's actually great that they are thin wrappers without magic, resulting in the same behaviour as if done without them.

> self.assertIn(pop([25, 75]), [25, 75])
> These should also verify that the element is not in the list anymore.
> Also other test with different (and possibly wrong) input types should be added.

This is true for many convenience functions in random. I've considered doing that at first and came to the conclusion that the tests for random should test the behaviour specific to random, not that of the underlying functionality?

So seq[random] should test for random behaviour, not that indexing of seq itself works correctly. Similarly, seq.pop(random) should test for random behaviour and not for a working seq.pop()?

All we would do here is duplicate tests not related to random.

Please correct me if I'm wrong. I'm glad that this started a discussion.
msg143762 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-09-09 14:04
>> Not all the sequences have a .pop() method.
> Not all sequences support indexing or item assignment either:

All the sequences support indexing and len(), but not assignment (e.g. tuples and strings are sequences, but they are immutable).  So talking about sequences there is incorrect (see also http://docs.python.org/py3k/glossary.html#term-sequence ).

> So seq[random] should test for random behaviour, not that indexing of
> seq itself works correctly. Similarly, seq.pop(random) should test for
> random behaviour and not for a working seq.pop()?

The test should check the behavior of the function, and in this case it includes both getting a random element and removing it.  The test doesn't have to check that seq.pop() is working fine (there are other tests for that) but that it's actually called and that it pops the right element from the input sequence (and not e.g. from a copy of the sequence that might have been created at some point).
In general it's always better to have a test more than one less.

Regarding the function itself, I'm not sure about its usefulness.  While I've used random.choice several times, I never felt the need of popping the elements.  One use case I might think of is picking all the elements of a sequence until there are no more left, without risking to pick the same element twice.  The same result can be achieved by shuffling the sequence and iterate over the elements though (that doesn't actually leave you with an empty sequence, but that's probably just a unimportant side-effect).
msg143765 - (view) Author: John Feuerstein (john.feuerstein) * Date: 2011-09-09 16:30
> The test doesn't have to check that seq.pop() is working fine (there are other tests for that) but that it's actually called and that it pops the right element from the input sequence (and not e.g. from a copy of the sequence that might have been created at some point).

I agree, that makes sense.

> One use case I might think of is picking all the elements of a sequence until there are no more left, without risking to pick the same element twice.  The same result can be achieved by shuffling the sequence and iterate over the elements though (that doesn't actually leave you with an empty sequence, but that's probably just a unimportant side-effect).

One problem with shuffling in this use case is that you lose the otherwise original order of the remaining elements, so there is no way to return from "pop random elements" to "pop elements in order" (without working on a shallow copy). However, I would agree that this is rather uncommon.

Feel free to close this as "wont fix", after all it is trivial for the user to randomly pop elements out of a sequence himself (see above).

There might be other use cases. If nothing else, there's at least issue 12941 to reference now.

Thanks!
History
Date User Action Args
2011-09-09 23:45:06rhettingersetassignee: rhettinger
2011-09-09 19:51:49terry.reedysetstatus: open -> closed
resolution: rejected
stage: patch review -> resolved
2011-09-09 16:30:20john.feuersteinsetmessages: + msg143765
2011-09-09 14:04:25ezio.melottisetmessages: + msg143762
2011-09-09 13:41:16john.feuersteinsetmessages: + msg143760
2011-09-09 12:04:54mark.dickinsonsetnosy: + mark.dickinson
messages: + msg143759
2011-09-09 03:30:28Juliansetnosy: + Julian
messages: + msg143753
2011-09-09 00:50:51ezio.melottisetnosy: + rhettinger, ezio.melotti
stage: patch review

versions: + Python 3.3, - Python 3.4
2011-09-09 00:11:00john.feuersteincreate