Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move vertex facet graph to combinatorial polyhedron #29188

Closed
kliem opened this issue Feb 12, 2020 · 16 comments
Closed

Move vertex facet graph to combinatorial polyhedron #29188

kliem opened this issue Feb 12, 2020 · 16 comments

Comments

@kliem
Copy link
Contributor

kliem commented Feb 12, 2020

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

CC: @jplab @LaisRast

Component: geometry

Keywords: polyhedra, vertex facet graph, combinatorial polyhedron

Author: Jonathan Kliem

Branch/Commit: a36a211

Reviewer: Jean-Philippe Labbé

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

@kliem kliem added this to the sage-9.1 milestone Feb 12, 2020
@kliem
Copy link
Contributor Author

kliem commented Feb 12, 2020

New commits:

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

@kliem
Copy link
Contributor Author

kliem commented Feb 12, 2020

Branch: public/29188

@kliem
Copy link
Contributor Author

kliem commented Feb 12, 2020

Commit: 6dbf267

@jplab
Copy link

jplab commented Feb 14, 2020

Reviewer: Jean-Philippe Labbé

@jplab
Copy link

jplab commented Feb 14, 2020

comment:2

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....

@kliem
Copy link
Contributor Author

kliem commented Feb 14, 2020

New commits:

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

@kliem
Copy link
Contributor Author

kliem commented Feb 14, 2020

Changed branch from public/29188 to public/29188-reb

@kliem
Copy link
Contributor Author

kliem commented Feb 14, 2020

Changed commit from 6dbf267 to 7bae9dc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 19, 2020

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

d4679c7removed unused import

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 19, 2020

Changed commit from 7bae9dc to d4679c7

@kliem
Copy link
Contributor Author

kliem commented Feb 20, 2020

comment:5

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

Replying to @jplab:

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....

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 20, 2020

Changed commit from d4679c7 to a36a211

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 20, 2020

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

a36a211swapped examples

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 14, 2020

comment:8

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.

@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 Apr 14, 2020
@jplab
Copy link

jplab commented Apr 20, 2020

comment:9

LGTM.

@vbraun
Copy link
Member

vbraun commented Apr 23, 2020

Changed branch from public/29188-reb to a36a211

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants