Opened 4 years ago
Closed 4 years ago
#26035 closed enhancement (fixed)
py3: some details in polyhedron
Reported by: | chapoton | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.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, GitHub, GitLab) | Commit: | b357154f847d38a85f54423e8f24af6998802166 |
Dependencies: | Stopgaps: |
Description
Change History (17)
comment:1 Changed 4 years 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 4 years 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 4 years 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 4 years 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:i-1 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 4 years 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 4 years ago by
- Branch changed from u/chapoton/26035 to u/jhpalmieri/26035
comment:7 Changed 4 years 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 4 years ago by
ok for list comprehension.
but please do not use underscore _
, but a normal variable name.
comment:9 Changed 4 years 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 4 years 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 4 years ago by
good for me if the bot is morally green
comment:12 Changed 4 years 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 4 years ago by
Quick little micro-optimization: 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 4 years 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 4 years ago by
We can deal with that now.
comment:16 Changed 4 years 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 4 years 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/