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

Add a face truncation method to Polyhedron class #18128

Closed
jplab opened this issue Apr 6, 2015 · 56 comments
Closed

Add a face truncation method to Polyhedron class #18128

jplab opened this issue Apr 6, 2015 · 56 comments

Comments

@jplab
Copy link

jplab commented Apr 6, 2015

Currently, it is possible to do a truncation of a polytope via the method ".edge_truncation()".

See http://en.wikipedia.org/wiki/Truncation_%28geometry%29

I currently need the notion of edge-truncation, which is achieve by cutting the polytope along a "well-chosen" hyperplane whose normal vector lies in the normal cone of the edge. This edge truncation uses only one edge. Not all edges at once.

Further, one can define a face truncation similarly with the same code. I am implementing a method called ".face_truncation(face, normal_coefficient, cut_frac)" taking a face of the polytope, and two optional parameters to vary the angle of the cut.

This new method makes the old method ill-named. It should be renamed simply "truncation" or "complete_vertex_truncation".

While at it, I corrected a few writing conventions in the file.

CC: @vbraun @sagetrac-mhampton @mo271

Component: geometry

Keywords: face truncation, polytope

Author: Jean-Philippe Labbé

Branch/Commit: 55f9cf6

Reviewer: Vincent Delecroix, Frédéric Chapoton

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

@jplab jplab added this to the sage-6.6 milestone Apr 6, 2015
@jplab
Copy link
Author

jplab commented Apr 7, 2015

Branch: u/jipilab/ge_truncation

@jplab
Copy link
Author

jplab commented Apr 7, 2015

New commits:

2182d7eInitial commit: added a face truncation method
8bec9b2Added deprecation warning

@jplab

This comment has been minimized.

@jplab
Copy link
Author

jplab commented Apr 7, 2015

Commit: 8bec9b2

@jplab
Copy link
Author

jplab commented Apr 7, 2015

comment:4

All test passed on sage 6.6.b5. The tests do not take more than 0.1 sec more than before on my old laptop.

@jplab
Copy link
Author

jplab commented Apr 7, 2015

Author: Jean-Philippe Labbé

@vbraun
Copy link
Member

vbraun commented Apr 29, 2015

comment:6

Conflicts, can you merge in the latest beta?

@jplab
Copy link
Author

jplab commented Apr 29, 2015

comment:7

Sure, will do.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 29, 2015

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

d26a570Merge branch 'develop' into edge_truncation
b66b4edMerge branch sage6.7.beta3 into ticket18128

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 29, 2015

Changed commit from 8bec9b2 to b66b4ed

@jplab
Copy link
Author

jplab commented Apr 29, 2015

comment:9

I don't really know why there are two commits for the merge. Perhaps I typed something more by accident(!?)

@vbraun
Copy link
Member

vbraun commented Apr 30, 2015

comment:10

You checked in a conflict marker (the thing starting with <<<<<<< HEAD).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 30, 2015

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

0ba2629Solved forgotten conflict

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 30, 2015

Changed commit from b66b4ed to 0ba2629

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 7, 2015

Changed commit from 0ba2629 to eef9e96

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 7, 2015

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

cf567e8Merge branch 'sage-6.8.beta2' into edge_truncation
eef9e96Made test pass on 6.8.beta2

@fchapoton
Copy link
Contributor

comment:14

two failing doctests (should be easy to fix)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 25, 2015

Changed commit from eef9e96 to 6a555a5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 25, 2015

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

6a555a5Merge branch 'u/jipilab/ge_truncation' of trac.sagemath.org:sage into ticket18128

@jplab jplab modified the milestones: sage-7.0, sage-7.1 Feb 22, 2016
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2016

Changed commit from 09f517c to ee4ac08

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2016

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

ee4ac08Made tests pass

@jplab
Copy link
Author

jplab commented Feb 23, 2016

comment:34

I have difficulty decoding what is the test which fails on the bot.

@fchapoton
Copy link
Contributor

comment:35

The patchbot librae currently misbehaves. Look at the other bots instead.

@jplab
Copy link
Author

jplab commented Feb 28, 2016

comment:36

Okay, so the other bots seems to give a green flag...

Needs proper review again then!

@fchapoton
Copy link
Contributor

comment:37

does not apply..

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 6, 2017

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

ec17f2bMerge branch 'u/jipilab/ge_truncation' of trac.sagemath.org:sage into 18128

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 6, 2017

Changed commit from ee4ac08 to ec17f2b

@jplab
Copy link
Author

jplab commented Feb 6, 2017

comment:39

This is a rebased version on 7.6.b2, solved the conflicts.

All test passed on the three modified files. Let's see what the bot says.

@fchapoton
Copy link
Contributor

Changed reviewer from Vincent Delecroix to Vincent Delecroix, Frédéric Chapoton

@fchapoton
Copy link
Contributor

comment:40

Salut,

two remarks, otherwise this is good to go:

  1. in
+            normal_vector = sum([linear_coefficients[i]*normal_vectors[i] for i
+                                 in range(len(normal_vectors))])

there is no need for the [ ] just inside sum()

  1. similarly in
+        linear_evaluation = set([-normal_vector * (v.vector()) for v in
+            self.vertices()])

inside set()

Once done and checked, you can set a positive review on my behalf.

@fchapoton fchapoton modified the milestones: sage-7.1, sage-7.6 Feb 8, 2017
@jplab
Copy link
Author

jplab commented Feb 9, 2017

comment:41

Salut!

Replying to @fchapoton:

Salut,

two remarks, otherwise this is good to go:

  1. in
+            normal_vector = sum([linear_coefficients[i]*normal_vectors[i] for i
+                                 in range(len(normal_vectors))])

there is no need for the [ ] just inside sum()

  1. similarly in
+        linear_evaluation = set([-normal_vector * (v.vector()) for v in
+            self.vertices()])

inside set()

Just for my information, where is this feature coming from? python3, sage?

Once done and checked, you can set a positive review on my behalf.

Great! Thanks a lot!

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 9, 2017

Changed commit from ec17f2b to 55f9cf6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 9, 2017

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

55f9cf6Removed brackets in sum and set

@fchapoton
Copy link
Contributor

comment:44

Just for my information, where is this feature coming from? python3, sage?

This is just python. One can feed them an iterator, this avoids to build the list twice.

@jplab

This comment has been minimized.

@jplab
Copy link
Author

jplab commented Feb 9, 2017

comment:46

This is just python. One can feed them an iterator, this avoids to build the list twice.

Good to know! Thanks!

@vbraun
Copy link
Member

vbraun commented Feb 11, 2017

Changed branch from u/jipilab/ge_truncation to 55f9cf6

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