Opened 6 years ago
Closed 6 years ago
#21375 closed enhancement (fixed)
py3 get rid of xrange inside combinat
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage7.4 
Component:  python3  Keywords:  
Cc:  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  0a80b52 (Commits, GitHub, GitLab)  Commit:  0a80b52a77062d8d495a1b789d48e5c6d8e9f878 
Dependencies:  #21374  Stopgaps: 
Description (last modified by )
as a step towards python3
replace xrange by Python3style range, after
"from six.moves import range" (as fast as xrange)
here only in the combinat folder
 mechanical replacement of xrange by range
 insertion of the imports in the changed files
 fine tuning by hand so that tests pass
Change History (18)
comment:1 Changed 6 years ago by
 Branch set to u/chapoton/21375
 Commit set to 2ad4252e07617135601ee84fa13223916205112f
 Status changed from new to needs_review
comment:2 Changed 6 years ago by
In src/sage/combinat/affine_permutation.py.
Replace l=['x' for i in range(self.k+1)]
by l = ['x'] * (self.k+1)
.
>>> timeit.timeit("l=['x']*100") 0.8203661441802979 >>> timeit.timeit("l=['x' for i in xrange(100)]") 4.155416011810303
comment:3 followup: ↓ 5 Changed 6 years ago by
It is about 10x slower to use the new range:
sage: %timeit for x in range(500000): pass 100 loops, best of 3: 12.5 ms per loop sage: %timeit for x in xrange(500000): pass 100 loops, best of 3: 6.32 ms per loop sage: from builtins import range sage: %timeit for x in range(500000): pass 10 loops, best of 3: 111 ms per loop
So I am 1 for changing the xrange > range
now. All of these we should be able to do by running sed
on the code base when we switch to python3.
comment:4 Changed 6 years ago by
comment:5 in reply to: ↑ 3 Changed 6 years ago by
Why not using range
from six.moves
? It's comparable to xrange
.
sage: %timeit for x in range(500000): pass 100 loops, best of 3: 15.8 ms per loop sage: %timeit for x in xrange(500000): pass 100 loops, best of 3: 9.44 ms per loop sage: from six.moves import range sage: %timeit for x in range(500000): pass 100 loops, best of 3: 9.32 ms per loop
comment:6 Changed 6 years ago by
 Commit changed from 2ad4252e07617135601ee84fa13223916205112f to 3b94c9090ec97ca97575a2b48cbb4bc486f73fbc
comment:7 Changed 6 years ago by
ok, excellent suggestion, thanks. I just did that by replacing all the imports.
Our aim being to have code running both in py2 and py3, this is a much better solution.
comment:8 Changed 6 years ago by
 Commit changed from 3b94c9090ec97ca97575a2b48cbb4bc486f73fbc to bb6810f7b32a63a0d0a0dac904676e9048a1f7b4
Branch pushed to git repo; I updated commit sha1. New commits:
bb6810f  undo change of fibonacci_xrange

comment:9 Changed 6 years ago by
 Reviewers set to Travis Scrimshaw
When the patchbot comes back green or all tests pass, you can set a positive review.
@aapitzsch thanks for letting us know about six.moves.range
!
comment:10 Changed 6 years ago by
 Commit changed from bb6810f7b32a63a0d0a0dac904676e9048a1f7b4 to be7b3eaa0b5555e39fb997457fb4ec0df9a5d55e
Branch pushed to git repo; I updated commit sha1. New commits:
be7b3ea  trac 21375 fixing a few doctests

comment:11 Changed 6 years ago by
 Status changed from needs_review to positive_review
ok, bot is green, setting this patchbomb to positive review. There will surely be rebasing issues.
comment:13 Changed 6 years ago by
 Dependencies set to #21374
comment:14 Changed 6 years ago by
 Commit changed from be7b3eaa0b5555e39fb997457fb4ec0df9a5d55e to 0a80b52a77062d8d495a1b789d48e5c6d8e9f878
Branch pushed to git repo; I updated commit sha1. New commits:
0a80b52  Merge branch 'u/chapoton/21375' into 7.4.b4

comment:15 Changed 6 years ago by
 Status changed from needs_work to needs_review
merged with 7.4.b4 (easy merge, nothing strange)
comment:16 Changed 6 years ago by
 Description modified (diff)
comment:17 Changed 6 years ago by
 Status changed from needs_review to positive_review
comment:18 Changed 6 years ago by
 Branch changed from u/chapoton/21375 to 0a80b52a77062d8d495a1b789d48e5c6d8e9f878
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
get rid of xrange in combinat