Opened 20 months ago
Closed 20 months ago
#26035 closed enhancement (fixed)
py3: some details in polyhedron
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.4 
Component:  python3  Keywords:  
Cc:  jhpalmieri  Merged in:  
Authors:  Frédéric Chapoton, John Palmieri  Reviewers:  Frédéric Chapoton, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  b357154 (Commits)  Commit:  b357154f847d38a85f54423e8f24af6998802166 
Dependencies:  Stopgaps: 
Description
Change History (17)
comment:1 Changed 20 months ago by
 Branch set to u/chapoton/26035
 Cc jhpalmieri added
 Commit set to 5a6d66a77df336c30d9817e5d83e46dd31d64040
 Status changed from new to needs_review
comment:2 Changed 20 months ago by
 Commit changed from 5a6d66a77df336c30d9817e5d83e46dd31d64040 to 1264a7add22a1061c6b44501d20dac1a62b2214b
Branch pushed to git repo; I updated commit sha1. New commits:
1264a7a  one detail

comment:3 Changed 20 months ago by
 Reviewers set to John Palmieri
 Status changed from needs_review to positive_review
I don't know if this is the ideal fix, but it works for me.
comment:4 Changed 20 months ago by
 Status changed from positive_review to needs_info
Actually, I'm going to try another fix first:

src/sage/geometry/polyhedron/backend_cdd.py
diff git a/src/sage/geometry/polyhedron/backend_cdd.py b/src/sage/geometry/polyhedron/backend_cdd.py index 63fd020d06..5ecab3aa76 100644
a b class Polyhedron_cdd(Polyhedron_base): 218 218 cddout = cddout.splitlines() 219 219 220 220 def parse_indices(count, cdd_indices, cdd_indices_to_sage_indices=None): 221 cdd_indices = list(map(int, cdd_indices))221 cdd_indices = [int(_) for _ in cdd_indices] 222 222 if cdd_indices_to_sage_indices is None: 223 223 cdd_indices_to_sage_indices = {i:i1 for i in cdd_indices} 224 224 if count < 0: … … class Polyhedron_cdd(Polyhedron_base): 247 247 assert self.ambient_dim() == dimension  1, "Unexpected ambient dimension" 248 248 assert len(data) == count, "Unexpected number of lines" 249 249 for i, line in enumerate(data): 250 coefficients = list(map(self.base_ring(), line))250 coefficients = [self.base_ring()(x) for x in line] 251 251 if coefficients[0] != 0 and all(e == 0 for e in coefficients[1:]): 252 252 # cddlib sometimes includes an implicit plane at infinity: 1 0 0 ... 0 253 253 # We do not care about this entry.
comment:5 Changed 20 months ago by
This new version works for me and I prefer it: there is no reason to use map
if we want a list and can use list comprehension instead. So I propose we use this instead.
comment:6 Changed 20 months ago by
 Branch changed from u/chapoton/26035 to u/jhpalmieri/26035
comment:7 Changed 20 months ago by
 Commit changed from 1264a7add22a1061c6b44501d20dac1a62b2214b to e57d3dfd0a4a89660dc3b6e5c81ae52188189547
 Reviewers John Palmieri deleted
 Status changed from needs_info to needs_review
Of course I proposed the first change, too, so sorry about that. I was just trying to get the PDF docs to build, not thinking too clearly about the right way to do things.
New commits:
e57d3df  trac 26035: use list comprehension instead of list(map(...))

comment:8 Changed 20 months ago by
ok for list comprehension.
but please do not use underscore _
, but a normal variable name.
comment:9 Changed 20 months ago by
 Commit changed from e57d3dfd0a4a89660dc3b6e5c81ae52188189547 to 8c1c081c439e6f38e3f08fe8d95630014b10b1d2
Branch pushed to git repo; I updated commit sha1. New commits:
8c1c081  trac 26035: Don't use _ in list comprehension. Convert 'data' to a list

comment:10 Changed 20 months ago by
Okay, no _
. Also, in moving the patch from #25840 to here, the change data = list(data)
got moved to the wrong place. That's fixed, so the plot which was problematic before in Python 3 now works. If you want to test, it's this one:
sage: M = matrix([[0., 0.556793440452, 0.19694925177, 0.19694925177, 0.0805477263944, 0.385290876171, 0., 0.3852 ....: 90876171], [0.180913155536, 0., 0.160212955043, 0.160212955043, 0., 0.0990170516545, 0.766360424875, 0.099017 ....: 0516545],[0.338261212718, 0, 0, 0.338261212718, 0.672816364803, 0.171502564281, 0, 0.171502564281]]) sage: L = RootSystem(["E",8]).ambient_space() sage: L.plot(roots="all", reflection_hyperplanes=False, projection=lambda v: M*vector(v), labels=False)
comment:11 Changed 20 months ago by
good for me if the bot is morally green
comment:12 Changed 20 months ago by
 Reviewers set to Frédéric Chapoton
 Status changed from needs_review to positive_review
Great, thanks (bot was green).
comment:13 Changed 20 months ago by
Quick little microoptimization: Instead of
coefficients = [self.base_ring()(x) for x in line]
which repeatedly called the base_ring()
, I would move that outside of the bigger for
loop. However, you can do that on a later ticket. :)
comment:14 Changed 20 months ago by
 Commit changed from 8c1c081c439e6f38e3f08fe8d95630014b10b1d2 to b357154f847d38a85f54423e8f24af6998802166
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
b357154  trac 26035: minor optimization

comment:15 Changed 20 months ago by
We can deal with that now.
comment:16 Changed 20 months ago by
 Reviewers changed from Frédéric Chapoton to Frédéric Chapoton, Travis Scrimshaw
 Status changed from needs_review to positive_review
Thank you.
comment:17 Changed 20 months ago by
 Branch changed from u/jhpalmieri/26035 to b357154f847d38a85f54423e8f24af6998802166
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
some py3 details in polyhedron/