Opened 2 years ago
Closed 22 months ago
#26639 closed enhancement (fixed)
geometry/cone.py: use generator expressions and futureproof range() iterator
Reported by:  mjo  Owned by:  

Priority:  major  Milestone:  sage8.7 
Component:  geometry  Keywords:  
Cc:  novoselt, chapoton  Merged in:  
Authors:  Michael Orlitzky  Reviewers:  Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  7b51289 (Commits)  Commit:  7b512897c453982d4a21ac0ed74b60c49c00e8ad 
Dependencies:  Stopgaps: 
Description
We have a lot of lists that don't need to be lists in geometry/cone.py
, potentially slowing things down and wasting a bit of memory. This is especially prevalent in my own doctests (oops).
Two things should help:
 Use generator expressions instead of list comprehensions when we're merely iterating over things.
 Use
xrange
type iterators instead ofrange()
lists. This is complicated only by the fact thatxrange()
isn't futureproof with respect to python3.x.
Change History (9)
comment:1 Changed 2 years ago by
 Branch set to u/mjo/ticket/26639
 Cc novoselt chapoton added
 Commit set to 474b3356c7a55ed34bf8525747b6dc37a82d746a
 Status changed from new to needs_review
 Summary changed from geometry/cones.py: use generator expressions and futureproof range() iterator to geometry/cone.py: use generator expressions and futureproof range() iterator
comment:2 Changed 2 years ago by
By the way, I should warn the reviewer: in the following hunk,
@@ 5519,17 +5524,16 @@ class ConvexRationalPolyhedralCone(IntegralRayCollection, sage: L = ToricLattice(m*n) sage: M1 = MatrixSpace(F, m, m) sage: M2 = MatrixSpace(F, n, n)  sage: LL_K1 = [ M1(x.list())  ....: for x in K1.dual().lyapunov_like_basis() ]  sage: LL_K2 = [ M2(x.list()) for x in K2.lyapunov_like_basis() ]  sage: tps = [ s.tensor_product(x) for x in LL_K1 for s in LL_K2 ] + sage: tps = ( M2(s.list()).tensor_product(M1(x.list())) + ....: for x in K1.dual().lyapunov_like_basis() + ....: for s in K2.lyapunov_like_basis() ) sage: W = VectorSpace(F, (m**2)*(n**2))  sage: expected = span(F, [ W(x.list()) for x in tps ])  sage: pi_cone = Cone([g.list() for g in pi_gens], + sage: expected = span(F, ( W(x.list()) for x in tps )) + sage: pi_cone = Cone((g.list() for g in pi_gens), ....: lattice=L, ....: check=False) sage: LL_pi = pi_cone.lyapunov_like_basis()  sage: actual = span(F, [ W(x.list()) for x in LL_pi ]) + sage: actual = span(F, ( W(x.list()) for x in LL_pi )) sage: actual == expected True """
I did initially try the simpler change, to make LL_K1
and LL_K2
use generator expressions individually. But, that broke the test in a way that I don't understand. I don't like mysteries. Before merging this, we should know why that breaks, so that we can be sure it doesn't affect any of the other changes I made.
comment:3 Changed 2 years ago by
Rather use from six.moves import range
Beware of the risk of conflict with the other ticket about the very same file.
comment:4 Changed 2 years ago by
 Commit changed from 474b3356c7a55ed34bf8525747b6dc37a82d746a to 7b512897c453982d4a21ac0ed74b60c49c00e8ad
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
65f4f7b  py3: fix doctests in geometry/cone

e63b0ce  Trac #26639: use futureproof range() iterator in geometry/cone.py

5fa22d5  Trac #26639: add two missing set_random_seed() calls to geometry/cone.py.

7b51289  Trac #26639: use PEP 289 generator expressions in geometry/cone.py.

comment:5 Changed 2 years ago by
 Dependencies set to 26636
Thanks, I rebased onto your branch and force pushed.
comment:6 Changed 2 years ago by
 Dependencies changed from 26636 to #26636
comment:7 Changed 2 years ago by
To address my previous concern, the failure in comment 2 was due to an implementation detail that we need to be aware of. When I write,
LL_K1 = ( M1(x.list()) for x in K1.dual().lyapunov_like_basis() ) LL_K2 = ( M2(x.list()) for x in K2.lyapunov_like_basis() ) tps_gen = ( s.tensor_product(x) for x in LL_K1 for s in LL_K2 )
where LL_K1
and LL_K2
are generators (as opposed to lists) python will loop through the inner iterator multiple times for every item in the outer iterator. That doesn't work: the inner iterator is exhausted the first time around, and returns nothing for outer iterations 2, 3, etc.
So the situations we need to watch out for are:
 Don't return an iterator anywhere that we used to return a list (in case anyone is using the result in a nested loop).
 Don't turn two lists into generator expressions if they're later going to be used in a nested loop.
I've looked through the rest of the changes and don't think I made the same mistake anywhere else.
comment:8 Changed 22 months ago by
 Dependencies #26636 deleted
 Milestone changed from sage8.5 to sage8.7
 Reviewers set to Frédéric Chapoton
 Status changed from needs_review to positive_review
ok, let it be
comment:9 Changed 22 months ago by
 Branch changed from u/mjo/ticket/26639 to 7b512897c453982d4a21ac0ed74b60c49c00e8ad
 Resolution set to fixed
 Status changed from positive_review to closed
Frédéric, please make sure I'm not doing something stupid with the
range
import.