#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) Commit: b357154f847d38a85f54423e8f24af6998802166
Dependencies: Stopgaps:

Description


Change History (17)

comment:1 Changed 20 months ago by chapoton

  • Branch set to u/chapoton/26035
  • Cc jhpalmieri added
  • Commit set to 5a6d66a77df336c30d9817e5d83e46dd31d64040
  • Status changed from new to needs_review

New commits:

5a6d66asome py3 details in polyhedron/

comment:2 Changed 20 months ago by git

  • Commit changed from 5a6d66a77df336c30d9817e5d83e46dd31d64040 to 1264a7add22a1061c6b44501d20dac1a62b2214b

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

1264a7aone detail

comment:3 Changed 20 months ago by jhpalmieri

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

  • 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): 
    218218        cddout = cddout.splitlines()
    219219
    220220        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]
    222222            if cdd_indices_to_sage_indices is None:
    223223                cdd_indices_to_sage_indices = {i:i-1 for i in cdd_indices}
    224224            if count < 0:
    class Polyhedron_cdd(Polyhedron_base): 
    247247            assert self.ambient_dim() == dimension - 1, "Unexpected ambient dimension"
    248248            assert len(data) == count, "Unexpected number of lines"
    249249            for i, line in enumerate(data):
    250                 coefficients = list(map(self.base_ring(), line))
     250                coefficients = [self.base_ring()(x) for x in line]
    251251                if coefficients[0] != 0 and all(e == 0 for e in coefficients[1:]):
    252252                    # cddlib sometimes includes an implicit plane at infinity: 1 0 0 ... 0
    253253                    # We do not care about this entry.

comment:5 Changed 20 months ago by jhpalmieri

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 jhpalmieri

  • Branch changed from u/chapoton/26035 to u/jhpalmieri/26035

comment:7 Changed 20 months ago by jhpalmieri

  • Authors changed from Frédéric Chapoton to Frédéric Chapoton, John Palmieri
  • 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:

e57d3dftrac 26035: use list comprehension instead of list(map(...))

comment:8 Changed 20 months ago by chapoton

ok for list comprehension.

but please do not use underscore _, but a normal variable name.

comment:9 Changed 20 months ago by git

  • Commit changed from e57d3dfd0a4a89660dc3b6e5c81ae52188189547 to 8c1c081c439e6f38e3f08fe8d95630014b10b1d2

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

8c1c081trac 26035: Don't use _ in list comprehension. Convert 'data' to a list

comment:10 Changed 20 months ago by jhpalmieri

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 chapoton

good for me if the bot is morally green

comment:12 Changed 20 months ago by jhpalmieri

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

Great, thanks (bot was green).

Last edited 20 months ago by jhpalmieri (previous) (diff)

comment:13 Changed 20 months ago by tscrim

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 20 months ago by git

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

b357154trac 26035: minor optimization

comment:15 Changed 20 months ago by jhpalmieri

We can deal with that now.

comment:16 Changed 20 months ago by tscrim

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

  • Branch changed from u/jhpalmieri/26035 to b357154f847d38a85f54423e8f24af6998802166
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.