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: edge_graph -> vertex_graph #28603

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

CombinatorialPolyhedron: edge_graph -> vertex_graph #28603

kliem opened this issue Oct 15, 2019 · 16 comments

Comments

@kliem
Copy link
Contributor

kliem commented Oct 15, 2019

In order to make CombinatorialPolyhedron more consistent with Polyhedron, we replace edge_graph by vertex_graph.

In case of of unbounded polyhedra this might make a difference, as unbounded 1-faces are considered for edge_graph but not for vertex_graph.

For now we keep edge_graph and add a deprecation warning.

CC: @jplab @LaisRast

Component: geometry

Keywords: polytopes, combinatorial polyhedron

Author: Jonathan Kliem

Branch/Commit: ecb7986

Reviewer: Laith Rastanawi

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

@kliem kliem added this to the sage-9.0 milestone Oct 15, 2019
@kliem

This comment has been minimized.

@kliem
Copy link
Contributor Author

kliem commented Oct 15, 2019

Branch: public/28603

@kliem
Copy link
Contributor Author

kliem commented Oct 15, 2019

New commits:

a005e47added vertex_graph, deprecated edge_graph

@kliem
Copy link
Contributor Author

kliem commented Oct 15, 2019

Commit: a005e47

@LaisRast
Copy link

comment:4

It is good to go.

@LaisRast
Copy link

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:

96346faimproved deprecation warning

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 15, 2019

Changed commit from a005e47 to 96346fa

@jplab
Copy link

jplab commented Oct 16, 2019

comment:7

Could the sentence also include what function the use should use instead?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 16, 2019

Changed commit from 96346fa to 4b83abe

@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:

4b83abedeprecation warning gives new name

@kliem
Copy link
Contributor Author

kliem commented Oct 16, 2019

comment:9

Replying to @jplab:

Could the sentence also include what function the use should use instead?

Sure.

The deprecation warning doesn't show in normal use. When you run the doctest manually it won't show. If you pack it in

sage: def foo(C):
sage:     return C.edge_graph()

it is printed when calling foo.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 16, 2019

Changed commit from 4b83abe to ecb7986

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 16, 2019

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

ecb7986changed stacklevel to 3, so deprecation warning shows during normal usage

@LaisRast
Copy link

comment:11

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

@vbraun
Copy link
Member

vbraun commented Oct 21, 2019

Changed branch from public/28603 to ecb7986

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