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: | sage-7.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 Python3-style 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 follow-up: ↓ 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