Opened 2 years ago

Closed 2 years ago

#29190 closed enhancement (fixed)

Dual for combinatorial polyhedron

Reported by: gh-kliem Owned by:
Priority: major Milestone: sage-9.1
Component: geometry Keywords: polar, dual, combinatorial polyhedron
Cc: jipilab, gh-LaisRast Merged in:
Authors: Jonathan Kliem Reviewers: Laith Rastanawi
Report Upstream: N/A Work issues:
Branch: 73cf39f (Commits, GitHub, GitLab) Commit: 73cf39f5228f42fec3ad2a401ac9e6f535887d7d
Dependencies: Stopgaps:

Status badges

Description (last modified by gh-kliem)

We implement a method of CombinatorialPolyhedron that obtains the polar/dual. We name it dual and create an alias polar to be consistent with Polyhedron_base.

This is obtained by simply copying and interchanging the bitrepresentation of facets and vertices. The object is purely combinatorial and does not have vertex or facet names. A ValueError is raised for the unbounded case.

Along the way we implement a __copy__ method for ListOfFaces and allow initializing CombinatorialPolyhedron from a tuple consisting of two ListOfFaces.

Change History (10)

comment:1 Changed 2 years ago by gh-kliem

  • Branch set to public/29190
  • Commit set to 5078b6e7cd68c0eb9a3570fbef1beb9cab241297
  • Status changed from new to needs_review

New commits:

​bc51430copy for ListOfFaces
​5078b6eadd polar for combinatorial polyhedron

comment:2 Changed 2 years ago by gh-LaisRast

  • Status changed from needs_review to needs_work
  • Maybe add #28608 as a dependency here and change the code accordingly.
  • In the documentation of CombinatorialPolyhedron. add to far_face the option when data is a tuple consisting of facets and vertices (of class ListOfFaces?).
  • A typo: 
    -       * or a tuple consting of facets and vertices as two
+       * or a tuple consisting of facets and vertices as two

comment:3 Changed 2 years ago by gh-kliem

  • Dependencies set to #28608

comment:4 Changed 2 years ago by gh-kliem

I'm just going to wait until #28608 is merged, otherwise it's a pain to review.

comment:5 Changed 2 years ago by gh-kliem

  • Branch changed from public/29190 to public/29190-reb
  • Commit changed from 5078b6e7cd68c0eb9a3570fbef1beb9cab241297 to 8beef7ca7c82e837629544b404eb2e47699caf56
  • Dependencies #28608 deleted
  • Status changed from needs_work to needs_review

New commits:

​328c16dcopy for ListOfFaces
​8beef7cadd polar for combinatorial polyhedron

comment:6 Changed 2 years ago by gh-LaisRast

  • Status changed from needs_review to needs_work
  • I would suggest renaming the method name to dual (since this is purely combinatorial) and put polar as an alias to dual to be consistent with Polyhedron_base.
  • Add 
            .. SEEALSO::

            :meth:`~sage.geometry.polyhedron.base.Polyhedron_base.polar`.

Otherwise, the ticket is good to go.

comment:7 Changed 2 years ago by git

  • Commit changed from 8beef7ca7c82e837629544b404eb2e47699caf56 to 73cf39f5228f42fec3ad2a401ac9e6f535887d7d

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​23b87edcopy for ListOfFaces
​309150badd polar for combinatorial polyhedron
​586bc82renamed polar to dual
​73cf39ffixing doctest

comment:8 Changed 2 years ago by gh-kliem

  • Description modified (diff)
  • Reviewers set to Laith Rastanawi
  • Status changed from needs_work to needs_review
  • Summary changed from Polar for combinatorial polyhedron to Dual for combinatorial polyhedron

comment:9 Changed 2 years ago by gh-LaisRast

  • Status changed from needs_review to positive_review

I think it is good to go.

comment:10 Changed 2 years ago by vbraun

  • Branch changed from public/29190-reb to 73cf39f5228f42fec3ad2a401ac9e6f535887d7d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.