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

Implement facets method for Polyhedron #27974

Closed
jplab opened this issue Jun 12, 2019 · 30 comments
Closed

Implement facets method for Polyhedron #27974

jplab opened this issue Jun 12, 2019 · 30 comments

Comments

@jplab
Copy link

jplab commented Jun 12, 2019

It is often practical to get directly the facets of a polyhedron object without having to compute or to know the dimension of the object.

This ticket implements the following shortcut:

sage: c = polytopes.hypercube(4)
sage: dim = c.dimension()
sage: c.faces(dim-1)
(A 3-dimensional face of a Polyhedron in ZZ^4 defined as the convex hull of 8 vertices,
 A 3-dimensional face of a Polyhedron in ZZ^4 defined as the convex hull of 8 vertices,
 A 3-dimensional face of a Polyhedron in ZZ^4 defined as the convex hull of 8 vertices,
 A 3-dimensional face of a Polyhedron in ZZ^4 defined as the convex hull of 8 vertices,
 A 3-dimensional face of a Polyhedron in ZZ^4 defined as the convex hull of 8 vertices,
 A 3-dimensional face of a Polyhedron in ZZ^4 defined as the convex hull of 8 vertices,
 A 3-dimensional face of a Polyhedron in ZZ^4 defined as the convex hull of 8 vertices,
 A 3-dimensional face of a Polyhedron in ZZ^4 defined as the convex hull of 8 vertices)

Now becomes:

sage: c = polytopes.hypercube(4)
sage: c.facets()
(A 3-dimensional face of a Polyhedron in ZZ^4 defined as the convex hull of 8 vertices,
 A 3-dimensional face of a Polyhedron in ZZ^4 defined as the convex hull of 8 vertices,
 A 3-dimensional face of a Polyhedron in ZZ^4 defined as the convex hull of 8 vertices,
 A 3-dimensional face of a Polyhedron in ZZ^4 defined as the convex hull of 8 vertices,
 A 3-dimensional face of a Polyhedron in ZZ^4 defined as the convex hull of 8 vertices,
 A 3-dimensional face of a Polyhedron in ZZ^4 defined as the convex hull of 8 vertices,
 A 3-dimensional face of a Polyhedron in ZZ^4 defined as the convex hull of 8 vertices,
 A 3-dimensional face of a Polyhedron in ZZ^4 defined as the convex hull of 8 vertices)

CC: @jplab @LaisRast @kliem @sophiasage

Component: geometry

Keywords: polytopes, facets, days100

Author: Sophia Elia

Branch/Commit: 743c4c7

Reviewer: Jean-Philippe Labbé, Frédéric Chapoton

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

@jplab jplab added this to the sage-8.8 milestone Jun 12, 2019
@embray
Copy link
Contributor

embray commented Jun 14, 2019

comment:1

As the Sage-8.8 release milestone is pending, we should delete the sage-8.8 milestone for tickets that are not actively being worked on or that still require significant work to move forward. If you feel that this ticket should be included in the next Sage release at the soonest please set its milestone to the next release milestone (sage-8.9).

@embray embray removed this from the sage-8.8 milestone Jun 14, 2019
@sophiasage
Copy link
Mannequin

sophiasage mannequin commented Jul 23, 2019

Commit: 8dc7dd5

@sophiasage
Copy link
Mannequin

sophiasage mannequin commented Jul 23, 2019

New commits:

8dc7dd5implement facets

@sophiasage
Copy link
Mannequin

sophiasage mannequin commented Jul 23, 2019

Author: Sophia Elia

@sophiasage
Copy link
Mannequin

sophiasage mannequin commented Jul 23, 2019

Branch: public/27974

@sophiasage
Copy link
Mannequin

sophiasage mannequin commented Jul 23, 2019

Changed keywords from polytopes, facets to polytopes, facets, days100

@sophiasage sophiasage mannequin added the s: needs review label Jul 23, 2019
@fchapoton
Copy link
Contributor

comment:4

Suggestion: add cross-references, in the new method:

        .. SEEALSO:: :meth:`faces`

and the same in the other direction

@jplab
Copy link
Author

jplab commented Jul 23, 2019

comment:5

Suggestion: in the files base.py, face.py, and library.py, perhaps it would be nice to change the tests involving faces(a_number_here) where the intented thing is really .facets.

This way, the function is advertized in the documentation of functions, and also well-tested.

There are sufficient tests with faces that are not facets not to have this a regression. Further, it still uses faces anyways. So no loss in strength of testing...

One comment about the bot result: The pyflake line is taken care of in #27993.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 24, 2019

Changed commit from 8dc7dd5 to 76a87d7

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 24, 2019

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

76a87d7added cross-references, changed faces examples to facets where appropriate

@jplab
Copy link
Author

jplab commented Jul 24, 2019

comment:7

LGTM, pyflakes errors are taken care of in #27993.

@jplab
Copy link
Author

jplab commented Jul 24, 2019

Reviewer: Jean-Philippe Labbé

@jplab jplab added this to the sage-8.9 milestone Jul 24, 2019
@vbraun
Copy link
Member

vbraun commented Jul 28, 2019

comment:8

Merge conflict

@jplab
Copy link
Author

jplab commented Jul 29, 2019

comment:9

Please remove the insertion of spaces in the banner.

The conflict is probably from the library somewhere...

@fchapoton
Copy link
Contributor

comment:10

Looks good and the patchbot is green.

One detail needs to be fixed :

There is a seealso in facets referring to facets. It should refer to faces instead.

Once done, you can set to positive review on my behalf.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 27, 2019

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

e03f2eefixed seealso typo

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 27, 2019

Changed commit from 76a87d7 to e03f2ee

@vbraun
Copy link
Member

vbraun commented Aug 27, 2019

comment:13

Merge conflict, please merge in the next beta

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 5, 2019

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

3362dc1implement facets
1ab0c3frebased facets on sage8.9.beta9
e3621b8fixed seealso typo

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 5, 2019

Changed commit from e03f2ee to e3621b8

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 5, 2019

Changed commit from e3621b8 to e3177be

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 5, 2019

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

e3177bebanner fix

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 5, 2019

Changed commit from e3177be to f72960d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 5, 2019

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

f72960dbanner fix again

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 5, 2019

Changed commit from f72960d to 743c4c7

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 5, 2019

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

743c4c7added facets method to quickref

@sophiasage sophiasage mannequin added s: needs review and removed s: needs work labels Sep 5, 2019
@fchapoton
Copy link
Contributor

comment:20

ok, good to go

@fchapoton
Copy link
Contributor

Changed reviewer from Jean-Philippe Labbé to Jean-Philippe Labbé, Frédéric chapoton

@jplab
Copy link
Author

jplab commented Sep 9, 2019

Changed reviewer from Jean-Philippe Labbé, Frédéric chapoton to Jean-Philippe Labbé, Frédéric Chapoton

@vbraun
Copy link
Member

vbraun commented Sep 9, 2019

Changed branch from public/27974 to 743c4c7

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