PolyhedralComplex

[yuan-zhou]

Create (geometric) PolyhedralComplex, whose cells are Sage Polyhedra.

CC: @mkoeppe @jhpalmieri @jplab @tscrim

Component: geometry

Keywords: polyhedral complex

Author: Yuan Zhou

Branch/Commit: 32d34b7

Reviewer: John Palmieri, Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/31748

[yuan-zhou]

comment:40

Replying to @jhpalmieri:

Typo: "poyhedron" on about line 8.

Thanks for catching that!

I might change has_maximal_cell to is_maximal_cell. What do you think?

I once heard that self.is_xxx(other) means that "self is xxx of other". Therefore, in this case, I would think that cell.is_maximal_cell(polyhedralcomplex) and polyhedralcomplex.has_maximal_cell(cell). If this is not the convention in sage/homology, I'm happy to change the name to is_maximal_cell.

As far as refactoring goes, I see similar code but not identical code, so it would some work to reorganize things. That can be done on another ticket, in my opinion.

:)

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

f5be3fa

fix typo

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 0a40f1f to f5be3fa

[jhpalmieri]

comment:42

Replying to @yuan-zhou:

Replying to @jhpalmieri:

I might change has_maximal_cell to is_maximal_cell. What do you think?

I once heard that self.is_xxx(other) means that "self is xxx of other". Therefore, in this case, I would think that cell.is_maximal_cell(polyhedralcomplex) and polyhedralcomplex.has_maximal_cell(cell). If this is not the convention in sage/homology, I'm happy to change the name to is_maximal_cell.

In simplicial_complex.py, the methods is_face and is_shelling_order use is... the way I'm suggesting. It's common for is_xxx(self) to be used the way you say, but not universal, and absolutely not universal for is_xxx(self, x, y, z), with more arguments.

[tscrim]

comment:43

A few things to do before I would set a positive review:

• Add doctests for the NotImplementedError raising methods.
• Change has_maximal_cell to is_maximal_cell because that is what the docstring is saying. If this was has_maximal_cell, I would expect it not to take any other arguments.

• You can test plotting:

  sage: poset.plot(element_labels=d)                    # not tested

• Change:

  -TESTS on nonbounded polyhedral complex::
  +For a nonbounded polyhedral complex::

• Add a TestSuite(foo).run() to the __init__ doctests. (Not really necessary, but I like to see such a test there.)

[jhpalmieri]

comment:44

Also under discussion at #30400 and #31925: create a new directory sage/topology and move all files which deal with topological spaces there, leaving sage/homology for chain complexes and related tools. So perhaps this could be placed in sage/topology as well.

[mkoeppe]

comment:45

In light of #31925 I would actually suggest to move PolyhedralComplex to sage.geometry.polyhedral_complex -- where already similar modules exist: fan, triangulation, voronoi_diagram, hyperplane_arrangement.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

88fa298	Merge branch 'develop' of git://trac.sagemath.org/sage into t/31748/polyhedralcomplex
9e7e1fd	move PolyhedralComplex from sage.homology to sage.geometry

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from f5be3fa to 9e7e1fd

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

3188e74

revise according to ticket 31748 comment 43

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 9e7e1fd to 3188e74

[tscrim]

Reviewer: John Palmieri, Travis Scrimshaw

[tscrim]

Changed commit from 3188e74 to ee3f294

[tscrim]

comment:49

I made some reviewer changes to the documentation to standardize it a bit. If my changes are good, then I think we can set a positive review.

---

New commits:

ee3f294

Some reviewer changes to documentation.

[tscrim]

Changed branch from u/yzh/polyhedralcomplex to public/geometry/polyhedral_complex-31748

[fchapoton]

comment:50

there is a red patchbot plugin (pyflakes)

[jhpalmieri]

comment:51

Replying to @fchapoton:

there is a red patchbot plugin (pyflakes)

In particular:

src/sage/geometry/polyhedral_complex.py:117:1 'sage.structure.sage_object.SageObject' imported but unused
src/sage/geometry/polyhedral_complex.py:119:1 'sage.rings.rational_field.QQ' imported but unused
src/sage/geometry/polyhedral_complex.py:326:13 local variable 'cells' is assigned to but never used
src/sage/geometry/polyhedral_complex.py:967:13 local variable 'cells' is assigned to but never used

[jhpalmieri]

comment:52

These changes should fix those problems. Should I push a branch? What do you think of just calling self.cells() instead of cells = self.cells()?

diff --git a/src/sage/geometry/polyhedral_complex.py b/src/sage/geometry/polyhedral_complex.py
index 351bfe65b3..06dc60380c 100644
--- a/src/sage/geometry/polyhedral_complex.py
+++ b/src/sage/geometry/polyhedral_complex.py
@@ -114,9 +114,7 @@ from sage.topology.cell_complex import GenericCellComplex
 from sage.geometry.polyhedron.constructor import Polyhedron
 from sage.geometry.polyhedron.base import is_Polyhedron
 from sage.modules.free_module_element import vector
-from sage.structure.sage_object import SageObject
 from sage.rings.integer_ring import ZZ
-from sage.rings.rational_field import QQ
 from sage.graphs.graph import Graph
 from sage.combinat.posets.posets import Poset
 from sage.misc.misc import powerset
@@ -323,7 +321,7 @@ class PolyhedralComplex(GenericCellComplex):
         self._face_poset = None

         if maximality_check:
-            cells = self.cells()    # compute self._cells and self._face_poset
+            self.cells()    # compute self._cells and self._face_poset
             self._maximal_cells = cells_list_to_cells_dict(
                                       self._face_poset.maximal_elements())
         if face_to_face_check:
@@ -964,7 +962,7 @@ class PolyhedralComplex(GenericCellComplex):
             Finite poset containing 9 elements
         """
         if self._face_poset is None:
-            cells = self.cells()    # poset is obtained and cached in cells()
+            self.cells()    # poset is obtained and cached in cells()
         return self._face_poset

     def is_subcomplex(self, other):

[yuan-zhou]

comment:53

Thanks for the fixes! Calling self.cells() looks good to me.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

2f77bbc

solve the red patchbot plugin (pyflakes)

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from ee3f294 to 2f77bbc

[jhpalmieri]

comment:55

I'm happy. Positive review from everyone else?

[tscrim]

comment:56

LGTM.

[vbraun]

comment:57

On 32-bit:

**********************************************************************
File "src/sage/geometry/polyhedral_complex.py", line 1407, in sage.geometry.polyhedral_complex.PolyhedralComplex.relative_boundary_cells
Failed example:
    [p.vertices() for p in pc_lower_dim.relative_boundary_cells()]
Expected:
    [(A vertex at (0, 2),), (A vertex at (1, 2),)]
Got:
    [(A vertex at (1, 2),), (A vertex at (0, 2),)]
**********************************************************************
1 item had failures:
   1 of  17 in sage.geometry.polyhedral_complex.PolyhedralComplex.relative_boundary_cells
    [451 tests, 1 failure, 14.88 s]
----------------------------------------------------------------------
sage -t --long --random-seed=0 src/sage/geometry/polyhedral_complex.py  # 1 doctest failed
----------------------------------------------------------------------

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 2f77bbc to 32d34b7

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

79ba169	Merge branch 'public/geometry/polyhedral_complex-31748' of git://trac.sagemath.org/sage into public/geometry/polyhedral_complex-31748
32d34b7	Added sorted to doctest in polyhedral_complex.py for consistent output.

[tscrim]

comment:59

Trivial fix of added sorted to doctest.

[vbraun]

Changed branch from public/geometry/polyhedral_complex-31748 to 32d34b7