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

CombinatorialPolyhedron: ridge_graph -> facet_graph #28604

Closed
kliem opened this issue Oct 15, 2019 · 21 comments
Closed

CombinatorialPolyhedron: ridge_graph -> facet_graph #28604

kliem opened this issue Oct 15, 2019 · 21 comments

Comments

@kliem
Copy link
Contributor

kliem commented Oct 15, 2019

In order to make CombinatorialPolyhedron more consistent with Polyhedron, we replace ridge_graph by facet_graph.

In case of of the half space this makes a difference, as there is a facet, but no ridges.

For now we keep ridge_graph and add a deprecation warning.

While we are at it, we fix some bugs:

  • CombinatorialPolyhedron.ridges(names=False, add_equalities=True) ignores add_equalities=True now.

    Equalities do not carry indices and can only be added with names=True.

  • facet_graph is now aware of the following:
    a ridge in ridges is a tuple of indices with names=False and a tuple of tuples with names=True and add_equalities=True,

    e.g. (1,2) vs. (('my_vertex_1',), ('my_vertex_2',)).

  • ridges now appends equations at the end. This is consistent with FaceIterator.Hrepresentation.

Depends on #28603

CC: @jplab @LaisRast

Component: geometry

Keywords: polytopes, combinatorial polyhedron

Author: Jonathan Kliem

Branch/Commit: 9d93a14

Reviewer: Laith Rastanawi

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

@kliem kliem added this to the sage-9.0 milestone Oct 15, 2019
@kliem
Copy link
Contributor Author

kliem commented Oct 15, 2019

Author: Jonathan Kliem

@kliem
Copy link
Contributor Author

kliem commented Oct 15, 2019

Branch: public/28604

@kliem
Copy link
Contributor Author

kliem commented Oct 15, 2019

New commits:

a005e47added vertex_graph, deprecated edge_graph
8654f9fridge_graph -> facet_graph; fixed bug in ridges with arguments

@kliem
Copy link
Contributor Author

kliem commented Oct 15, 2019

Commit: 8654f9f

@kliem

This comment has been minimized.

@LaisRast
Copy link
Contributor

comment:2

facet_graph does not work for non-full-dimensional polyhedra:

    sage: C = CombinatorialPolyhedron(polytopes.hypersimplex(5,2)); C
    A 4-dimensional combinatorial polyhedron with 10 facets
    sage: C.facet_graph()
    Graph on 20 vertices

The problem arises since each facet in C.ridges(names=True, add_equalities=True) is represented as (equation, inequality), while in C.face_iter(C.dimension() - 1, dual=False)), each facet is represented as (inequality, equation).

facet_graph does not work when names=False:

    sage: C = CombinatorialPolyhedron(polytopes.cube()); C
    A 3-dimensional combinatorial polyhedron with 6 facets
    sage: C.facet_graph(names=False)
    Graph on 12 vertices

The problem arises sincefacet.Hrepr(names=False) returns a tuple, for example (5,). While in C.ridges(names=False), each facet is represented by a number.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 15, 2019

Changed commit from 8654f9f to 597afd8

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 15, 2019

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

c54886bbux fix regarding elements and 1-element tuples
597afd8ridges append eualities at the end

@kliem

This comment has been minimized.

@kliem

This comment has been minimized.

@LaisRast
Copy link
Contributor

comment:6

After the changes, I think that the ticket is good and thus I will put it on "positive review".

@LaisRast
Copy link
Contributor

Reviewer: Laith Rastanawi

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 15, 2019

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

3a6f44fimproved deprecation warning

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 15, 2019

Changed commit from 597afd8 to 3a6f44f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 16, 2019

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

bcb754fdeprecation warning gives new name

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 16, 2019

Changed commit from 3a6f44f to bcb754f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 16, 2019

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

9d93a14changed stacklevel to 3, so deprecation warning shows during normal usage

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 16, 2019

Changed commit from bcb754f to 9d93a14

@LaisRast
Copy link
Contributor

comment:12

The deprecation warning is now printed out. So I will put it back on "positive review".

@kliem

This comment has been minimized.

@vbraun
Copy link
Member

vbraun commented Oct 21, 2019

Changed branch from public/28604 to 9d93a14

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

3 participants