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

Polar of polytopes does not check if polytope is full-dimensional #28850

Closed
kliem opened this issue Dec 6, 2019 · 13 comments
Closed

Polar of polytopes does not check if polytope is full-dimensional #28850

kliem opened this issue Dec 6, 2019 · 13 comments

Comments

@kliem
Copy link
Contributor

kliem commented Dec 6, 2019

Currently, the polar of rational polyhedra does not check, whether the polyhedron is full-dimensional:

sage: P = polytopes.simplex(3, base_ring=QQ)
sage: P.polar()
A 4-dimensional polyhedron in QQ^4 defined as the convex hull of 4 vertices and 1 line

We fix this by adding an assertion

sage: P = polytopes.simplex(3, base_ring=QQ)
sage: P.polar()
Traceback (most recent call last):
...
AssertionError: must be full-dimensional

Also we add an extra keyword in_affine_span (default False). By this one can obtain the polar in its affine span (after translation as usual):

sage: P = polytopes.simplex(3, base_ring=QQ)
sage: P.polar(in_affine_span=True)
A 3-dimensional polyhedron in QQ^4 defined as the convex hull of 4 vertices

sage: point = Polyhedron([[0]])
sage: P = polytopes.cube().change_ring(QQ)
sage: (P*point).polar(in_affine_span=True) == P.polar()*point
True

This option seems reasonable and simplifies the current construction of barycentric subdivision.

We change the other message "Not a polytope." according to conventions to "not a polytope".

CC: @jplab @LaisRast

Component: geometry

Keywords: polar, polytopes

Author: Jonathan Kliem

Branch/Commit: 4ee9802

Reviewer: Travis Scrimshaw

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

@kliem kliem added this to the sage-9.0 milestone Dec 6, 2019
@kliem

This comment has been minimized.

@kliem
Copy link
Contributor Author

kliem commented Dec 6, 2019

Commit: 6b12213

@kliem
Copy link
Contributor Author

kliem commented Dec 6, 2019

Branch: public/28850

@kliem
Copy link
Contributor Author

kliem commented Dec 6, 2019

comment:2

I'm not exactly happy with the phrasing and the name in_affine_span.


New commits:

6b12213check whether polytope is full-dimensional before taking the polar; added a polar version in its affine span

@kliem
Copy link
Contributor Author

kliem commented Dec 6, 2019

comment:3

phrasing in the doctests

@kliem
Copy link
Contributor Author

kliem commented Dec 6, 2019

Changed keywords from none to polar, polytopes

@tscrim
Copy link
Collaborator

tscrim commented Dec 11, 2019

comment:5
Check that :trac:`28850` is fixed::

is overindented. Also it is not considered good practice to do an assert to check input but instead raise a ValueError or TypeError. Otherwise LGTM.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 11, 2019

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

4ee9802alignment in docs; AssertionError -> ValueError

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 11, 2019

Changed commit from 6b12213 to 4ee9802

@kliem
Copy link
Contributor Author

kliem commented Dec 11, 2019

comment:7

Ok, done. I also changed the test for compactness to give a ValueError.

Replying to @tscrim:

Check that :trac:`28850` is fixed::

is overindented. Also it is not considered good practice to do an assert to check input but instead raise a ValueError or TypeError. Otherwise LGTM.

@tscrim
Copy link
Collaborator

tscrim commented Dec 11, 2019

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Dec 11, 2019

comment:8

Thanks.

@vbraun
Copy link
Member

vbraun commented Dec 16, 2019

Changed branch from public/28850 to 4ee9802

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