New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added test to test_random.py testing Random.shuffle #60041
Comments
Random.shuffle does not have a test in test_random.py; the attached patch adds this test. In addition, I rewrote the documentation string for Random.shuffle, which apparently did not reflect recent changes in the code and was inconsistent with the definition of the method. This change is also part of this patch; I was not sure if this merited a separate issue, so I just included this here. On a related matter: in Random.shuffle there is a third optional argument which looks very odd to me: def shuffle(self, x, random=None, int=int):
.... Besides being confusing to a user typing help(shuffle), what the "int" argument does in shuffle is to convert a float to an integer. But one could pass any one-argument function in principle, and it would be then very hard to predict what shuffle would do... it would not "shuffle" any more in the traditional sense - not with a uniform probability distribution. In summary, I don't see how that argument could be useful, although the people who wrote the library must have had something in mind... if so it would be a good idea to document it. |
The patch seems to be missing. The int=int is probably some sort of micro-optimization and perhaps should be removed. |
Agree, this micro-optimization has no effect here. |
Sorry, here it is the patch. |
Comparing the execution time with and without the int=int argument of this command: amoura@amoura-laptop:~/cpython$ time ./python -c "from random import shuffle; lst=list(range(1000000)); shuffle(lst); print (len(lst))" I get with int=int: real 0m13.755s and without it: real 0m13.876s So it makes no difference in practice. On the other hand, removing this has a chance of braking existing code, if someone somewhere actually uses the third argument for something - I can't image what, but still... |
Third parameter (int) plays a role only in the presence of a second one (random). |
No, it always has an effect. It means that the name 'int' is bound in locals instead of being looked up via globals. That is what makes it a micro-optimization (LOAD_FAST vs LOAD_GLOBAL, if you do a dis on the two variants). |
Oh, I see what you are saying. The lookup of int is only done if random is not None. Yes, that is true. |
If the optimization is actually useful, it can be preserved by just putting 'int=int' (with an 'optimization' comment :) before the loop. |
The int=int still makes no difference, but if the second argument is set to random.random, we get a big speedup, regardless of whether the third argument is there: without int=int: amoura@amoura-laptop:~/cpython$ time ./python -c "import random; lst=list(range(1000000)); random.shuffle(lst,random.random); print (len(lst))" real 0m7.082s With int=int: amoura@amoura-laptop:~/cpython$ time ./python -c "import random; lst=list(range(1000000)); random.shuffle(lst,random.random); print (len(lst))" real 0m7.281s Without second argument: amoura@amoura-laptop:~/cpython$ time ./python -c "import random; lst=list(range(1000000)); random.shuffle(lst); print (len(lst))" real 0m13.783s This could be because of the many tests of whether the 2nd argument is None in the loop. |
This is because Random._randbelow (and therefore randrange, randint) is |
Yup. This is the result of simply eliminating the condition in the loop and just using the second argument (for the purposes of testing this only): amoura@amoura-laptop:~/cpython$ time ./python -c "import random; lst=list(range(1000000)); random.shuffle(lst,random.random); print (len(lst))" real 0m7.330s |
The patch look fine as-is and it can be applied in 3.4. (BTW, I miss having a Resolution status of Accepted, meaning that the patch passed review and is ready to apply). FWIW, I'll remove the int=int optimization in Py3.4. It doesn't provide much benefit anymore. |
I left a review on rietveld. FWIW these are the results of the tests using timeit: # with int=int # without int=int |
I am not sure that None as default should be documented. It's implementation details (as third "int" argument) and can be silently changed in future versions. |
New changeset 58776cc74e89 by Antoine Pitrou in branch 'default': |
I've committed the patch, thank you Alessandro. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: