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 stacking onto a face of a polyhedron #24847

Closed
jplab opened this issue Feb 26, 2018 · 34 comments
Closed

Implement stacking onto a face of a polyhedron #24847

jplab opened this issue Feb 26, 2018 · 34 comments

Comments

@jplab
Copy link
Contributor

jplab commented Feb 26, 2018

From https://www.csun.edu/~ctoth/Handbook/chap15.pdf:

Stacking onto a face of a polytope adds a vertex slightly outside of the polytope positioned over a point in the interior of the face.

This is the "polar" of the truncation operation.

Depends on #22572

CC: @videlec @mo271 @mkoeppe @fchapoton

Component: geometry

Keywords: days93, polytope

Author: Jean-Philippe Labbé

Branch/Commit: e5838ed

Reviewer: Moritz Firsching, Frédéric Chapoton

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

@jplab jplab added this to the sage-8.2 milestone Feb 26, 2018
@jplab
Copy link
Contributor Author

jplab commented Feb 27, 2018

comment:1

Here's a first working version.

I hesitated between different approaches. This is not the most flexible implementation, but at least a solid enough implementation.


New commits:

3bfb953first version of stacking

@jplab
Copy link
Contributor Author

jplab commented Feb 27, 2018

Dependencies: #22572

@jplab
Copy link
Contributor Author

jplab commented Feb 27, 2018

Commit: 3bfb953

@jplab

This comment has been minimized.

@jplab
Copy link
Contributor Author

jplab commented Feb 27, 2018

Branch: u/jipilab/24847

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 27, 2018

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

d5908a5Cleaned two old deprecation warnings

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 27, 2018

Changed commit from 3bfb953 to d5908a5

@jplab
Copy link
Contributor Author

jplab commented Mar 2, 2018

Author: Jean-Philippe Labbé

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 6, 2018

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

9677be7Merge branch '8.2.b7' into 24847

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 6, 2018

Changed commit from d5908a5 to 9677be7

@mo271
Copy link
Contributor

mo271 commented Apr 10, 2018

Changed branch from u/jipilab/24847 to u/moritz/24847

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 10, 2018

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

7f0af17pep

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 10, 2018

Changed commit from 9677be7 to 7f0af17

@mo271
Copy link
Contributor

mo271 commented Apr 10, 2018

comment:7

That's a nice addition.

One question: Should we give the user more control over the base ring of the resulting stacked polytope? At the moment it quietly changes base ring:

sage: C  = polytopes.cube()
sage: C.base_ring()
Integer Ring
sage: C.stack(C.faces(2)[2], 1/2).base_ring()
Rational Field
sage: C.stack(C.faces(2)[2], 0.5).base_ring()
Real Double Field

Also, in the dual method of face-truncation, there is an option linear_coefficients=None. Should there be an analogous option here?

@jplab
Copy link
Contributor Author

jplab commented Apr 10, 2018

comment:8

Replying to @mo271:

That's a nice addition.

Thanks! :)

One question: Should we give the user more control over the base ring of the resulting stacked polytope? At the moment it quietly changes base ring:

sage: C  = polytopes.cube()
sage: C.base_ring()
Integer Ring
sage: C.stack(C.faces(2)[2], 1/2).base_ring()
Rational Field
sage: C.stack(C.faces(2)[2], 0.5).base_ring()
Real Double Field

I do not think it is a good idea. The above is consistent with the Polyhedron constructor and also the current behavior for scaling (and many other methods...):

sage: C = polytopes.cube()
sage: 1/2*C
A 3-dimensional polyhedron in QQ^3 defined as the convex hull of 8 vertices
sage: 0.5*C
A 3-dimensional polyhedron in RDF^3 defined as the convex hull of 8 vertices

The user should be aware of the type of inputs that are given, and the constructed object is somehow expected to be in the fraction field of the ring. The base_ring of self can not be kept in general and if the user wants to keep the base ring, it should keep the input to be in the base ring...

I agree that it would be nice to make this step for the user, but it is a nightmare to implement in a uniform stable way... :-/

Also, in the dual method of face-truncation, there is an option linear_coefficients=None. Should there be an analogous option here?

I thought about it, but it leads to a lot of case analysis. At least, I did not find a simple way to do this. it seems to me that the dual equivalent has more "accessible" freedom than that one. Although I may be wrong.

@mo271
Copy link
Contributor

mo271 commented Apr 11, 2018

comment:9

ok, I agree with your reasoning above.

One more thing:

The stacking seems to work nicely for unbounded polyhedra if you stack on bounded faces of them:

sage: [fac] =  [_ for _ in P.faces(1) if _.as_polyhedron().is_compact()]
sage: P = Polyhedron(ieqs = [[-1,1,0], [1,0,-1], [1,0,1]])
sage: [fac] =  [_ for _ in P.faces(1) if _.as_polyhedron().is_compact()]
sage: P = Polyhedron(ieqs = [[-1,1,0], [1,0,-1], [1,0,1]]); P
A 2-dimensional polyhedron in QQ^2 defined as the convex hull of 2 vertices and 1 ray
sage: [fac] =  [_ for _ in P.faces(1) if _.as_polyhedron().is_compact()]
sage: SP = P.stack(fac); SP
A 2-dimensional polyhedron in QQ^2 defined as the convex hull of 3 vertices and 1 ray
sage: P.vertices(), SP.vertices()
((A vertex at (1, 1), A vertex at (1, -1)),
 (A vertex at (0, 0), A vertex at (1, -1), A vertex at (1, 1)))

Could you add this as an example?

What is the desired behavior when stacking on unbounded faces of unbounded polyhedra?

@mo271
Copy link
Contributor

mo271 commented Apr 11, 2018

Reviewer: Moritz Firsching

@jplab
Copy link
Contributor Author

jplab commented Apr 11, 2018

comment:11

The stacking seems to work nicely for unbounded polyhedra if you stack on bounded faces of them:

sage: [fac] =  [_ for _ in P.faces(1) if _.as_polyhedron().is_compact()]
sage: P = Polyhedron(ieqs = [[-1,1,0], [1,0,-1], [1,0,1]])
sage: [fac] =  [_ for _ in P.faces(1) if _.as_polyhedron().is_compact()]
sage: P = Polyhedron(ieqs = [[-1,1,0], [1,0,-1], [1,0,1]]); P
A 2-dimensional polyhedron in QQ^2 defined as the convex hull of 2 vertices and 1 ray
sage: [fac] =  [_ for _ in P.faces(1) if _.as_polyhedron().is_compact()]
sage: SP = P.stack(fac); SP
A 2-dimensional polyhedron in QQ^2 defined as the convex hull of 3 vertices and 1 ray
sage: P.vertices(), SP.vertices()
((A vertex at (1, 1), A vertex at (1, -1)),
 (A vertex at (0, 0), A vertex at (1, -1), A vertex at (1, 1)))

Could you retrieve the first P above? It seems to be missing.

Could you add this as an example?

Sure!

What is the desired behavior when stacking on unbounded faces of unbounded polyhedra?

If one takes "stacking" as adding a vertex "just outside" a face as a definition, then it is not very difficult to extend it to unbounded faces using .representative_point() for face.as_polyhedron() and pushing it out a little.

@jplab
Copy link
Contributor Author

jplab commented Apr 11, 2018

New commits:

664df99deal with unbounded faces

@jplab
Copy link
Contributor Author

jplab commented Apr 11, 2018

Changed commit from 7f0af17 to 664df99

@jplab
Copy link
Contributor Author

jplab commented Apr 11, 2018

Changed branch from u/moritz/24847 to public/stackpoly

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 11, 2018

Changed commit from 664df99 to 2a59f67

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 11, 2018

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

7f5e125fixed typos in polyhedra_quickref.rst
bd6c356LateX -> LaTeX in polytope_tikz.rst
b1ce45bSeveral other corrections
d526f70renamed tutorial files
d7896f8Merge branch 'develop' into 22572
597b802Merge branch sage8.2.rc1 into 22572
eee533aCorrections from review
b74ae85Made tests pass
b5470d8Merge branch tutorial into stackpoly
2a59f67Added stack to quickref

@jplab

This comment has been minimized.

@jplab
Copy link
Contributor Author

jplab commented Apr 11, 2018

comment:14

I made the function a bit more robust: checking if the face is a PolyhedronFace object, and fixed it so that it accepts unbounded faces.

@mo271: I think that the new examples show the possibilities properly.

Needs review again!

@fchapoton
Copy link
Contributor

comment:15

branch does not merge

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 11, 2018

Changed commit from 2a59f67 to e5838ed

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 11, 2018

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

e5838edMerge branch 'stackpoly' into 24847

@jplab jplab modified the milestones: sage-8.2, sage-8.3 Jul 11, 2018
@jplab
Copy link
Contributor Author

jplab commented Jul 11, 2018

comment:18

Now merges cleanly.

In the ticket, I also remove two old deprecation warnings...

@fchapoton
Copy link
Contributor

Changed reviewer from Moritz Firsching to Moritz Firsching, Frédéric Chapoton

@fchapoton
Copy link
Contributor

comment:20

ok, let it be

@jplab
Copy link
Contributor Author

jplab commented Jul 12, 2018

comment:21

Thanks for the review!

@videlec
Copy link
Contributor

videlec commented Aug 3, 2018

comment:22

update milestone 8.3 -> 8.4

@videlec videlec modified the milestones: sage-8.3, sage-8.4 Aug 3, 2018
@vbraun
Copy link
Member

vbraun commented Aug 5, 2018

Changed branch from public/stackpoly to e5838ed

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

5 participants