Opened 5 months ago

Closed 3 months ago

Last modified 2 months ago

#29188 closed enhancement (fixed)

Move vertex facet graph to combinatorial polyhedron

Reported by: gh-kliem Owned by:
Priority: major Milestone: sage-9.1
Component: geometry Keywords: polyhedra, vertex facet graph, combinatorial polyhedron
Cc: jipilab, gh-LaisRast Merged in:
Authors: Jonathan Kliem Reviewers: Jean-Philippe Labbé
Report Upstream: N/A Work issues:
Branch: a36a211 (Commits) Commit: a36a21179ccb0e23792f312d75283db5697f2aed
Dependencies: Stopgaps:

Description

We move the method vertex_facet_graph from Polyhedron_base to CombinatorialPolyhedron.

Along the way we fix an bug, namely that vertex_facet_graph(labels=False) has the edges the wrong way::

sage: P = polytopes.cube()
sage: P.vertex_facet_graph().is_isomorphic(P.vertex_facet_graph(False))
False
sage: P.vertex_facet_graph().is_isomorphic(P.vertex_facet_graph(False).reverse())
True

Change History (11)

comment:1 Changed 5 months ago by gh-kliem

  • Branch set to public/29188
  • Commit set to 6dbf267cb51e5cc05354ce330427aedcdb43eb30
  • Status changed from new to needs_review

New commits:

630b8a8add vertex_facet_graph to combinatorial polyhedron
6dbf267use combinatorial polyhedron to obtain the vertex facet graph

comment:2 follow-up: Changed 5 months ago by jipilab

  • Reviewers set to Jean-Philippe Labbé
  • Status changed from needs_review to needs_work

There is a space missing in the inputs.

Then, I would just swap the two examples to show the default behavior first.

Could you make this sentence more clear:

If ``names`` is ``True`` but no names are provided,

I would suggest to put If names is True (the default) but no names....

comment:3 Changed 5 months ago by gh-kliem

  • Branch changed from public/29188 to public/29188-reb
  • Commit changed from 6dbf267cb51e5cc05354ce330427aedcdb43eb30 to 7bae9dc943a7bd12f7e6a03993ea5d319345db2a
  • Status changed from needs_work to needs_review

New commits:

9a62a52add vertex_facet_graph to combinatorial polyhedron
1e10ebduse combinatorial polyhedron to obtain the vertex facet graph
7bae9dcimproved doc

comment:4 Changed 5 months ago by git

  • Commit changed from 7bae9dc943a7bd12f7e6a03993ea5d319345db2a to d4679c7c6ae42d3aa7e133f029622434bec511fb

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

d4679c7removed unused import

comment:5 in reply to: ↑ 2 Changed 5 months ago by gh-kliem

  • Status changed from needs_review to needs_work

I'll just put it back to needs work until this is done.

Replying to jipilab:

There is a space missing in the inputs.

Then, I would just swap the two examples to show the default behavior first.

Could you make this sentence more clear:

If ``names`` is ``True`` but no names are provided,

I would suggest to put If names is True (the default) but no names....

comment:6 Changed 5 months ago by git

  • Commit changed from d4679c7c6ae42d3aa7e133f029622434bec511fb to a36a21179ccb0e23792f312d75283db5697f2aed

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

a36a211swapped examples

comment:7 Changed 5 months ago by gh-kliem

  • Status changed from needs_work to needs_review

comment:8 Changed 3 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.

comment:9 Changed 3 months ago by jipilab

  • Status changed from needs_review to positive_review

LGTM.

comment:10 Changed 3 months ago by vbraun

  • Branch changed from public/29188-reb to a36a21179ccb0e23792f312d75283db5697f2aed
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:11 Changed 2 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.1
Note: See TracTickets for help on using tickets.