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:

Status badges

Description (last modified by chapoton)

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 chapoton

  • Branch set to u/chapoton/21375
  • Commit set to 2ad4252e07617135601ee84fa13223916205112f
  • Status changed from new to needs_review

New commits:

​2ad4252get rid of xrange in combinat

comment:2 Changed 6 years ago by aapitzsch

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: Changed 6 years ago by tscrim

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 chapoton

ok, I thought that it was a perfect replacement, but it is not (coded in Python instead of CPython, I guess)

then let us close this ticket and keep only #16081 and #16457

comment:5 in reply to: ↑ 3 Changed 6 years ago by aapitzsch

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 git

  • Commit changed from 2ad4252e07617135601ee84fa13223916205112f to 3b94c9090ec97ca97575a2b48cbb4bc486f73fbc

Branch pushed to git repo; I updated commit sha1. New commits:

​21ad08ausing range from six.moves (as fast as xrange)
​3b94c90trac 21375 reviewer suggestion

comment:7 Changed 6 years ago by chapoton

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 git

  • Commit changed from 3b94c9090ec97ca97575a2b48cbb4bc486f73fbc to bb6810f7b32a63a0d0a0dac904676e9048a1f7b4

Branch pushed to git repo; I updated commit sha1. New commits:

​bb6810fundo change of fibonacci_xrange

comment:9 Changed 6 years ago by tscrim

  • 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 git

  • Commit changed from bb6810f7b32a63a0d0a0dac904676e9048a1f7b4 to be7b3eaa0b5555e39fb997457fb4ec0df9a5d55e

Branch pushed to git repo; I updated commit sha1. New commits:

​be7b3eatrac 21375 fixing a few doctests

comment:11 Changed 6 years ago by chapoton

  • 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:12 Changed 6 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:13 Changed 6 years ago by aapitzsch

  • Dependencies set to #21374

comment:14 Changed 6 years ago by git

  • Commit changed from be7b3eaa0b5555e39fb997457fb4ec0df9a5d55e to 0a80b52a77062d8d495a1b789d48e5c6d8e9f878

Branch pushed to git repo; I updated commit sha1. New commits:

​0a80b52Merge branch 'u/chapoton/21375' into 7.4.b4

comment:15 Changed 6 years ago by chapoton

  • Status changed from needs_work to needs_review

merged with 7.4.b4 (easy merge, nothing strange)

comment:16 Changed 6 years ago by chapoton

  • Description modified (diff)

comment:17 Changed 6 years ago by tscrim

  • Status changed from needs_review to positive_review

comment:18 Changed 6 years ago by vbraun

  • Branch changed from u/chapoton/21375 to 0a80b52a77062d8d495a1b789d48e5c6d8e9f878
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.