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

{Polyhedron, ConvexRationalPolyhedralCone}.{interior, relative_interior} #31916

Closed
mkoeppe opened this issue Jun 6, 2021 · 36 comments
Closed

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Jun 6, 2021

We introduce new methods relative_interior and interior, which (in nontrivial cases) return a simple object with a __contains__ method.

Then one can write x in P.relative_interior() instead of P.relative_interior_contains(x).

This will also simplify #31660.

CC: @kliem @yuan-zhou @jplab @tscrim @novoselt @orlitzky

Component: geometry

Author: Matthias Koeppe

Branch/Commit: fa4c2d2

Reviewer: Jonathan Kliem

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

@mkoeppe mkoeppe added this to the sage-9.4 milestone Jun 6, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 6, 2021

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 6, 2021

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 6, 2021

Commit: e9c670c

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 6, 2021

New commits:

e9c670csage.geometry.polyhedron.relint, Polyhedron_base.relative_interior, Polyhedron_base.interior: New

@mkoeppe mkoeppe changed the title Polyhedron.relative_interior {Polyhedron, ConvexRationalPolyhedralCone}.{interior, relative_interior} Jun 6, 2021
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2021

Changed commit from e9c670c to b8bfe20

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2021

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

9df2104sage.geometry.relative_interior: Move here from sage.geometry.polyhedron.relint
b8bfe20ConvexRationalPolyhedralCone: Add methods interior, relative_interior

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2021

Changed commit from b8bfe20 to 6869673

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2021

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

6869673relative_interior: Fix for dimension 0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2021

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

021d073RelativeInterior: Add documentation, tests, comparison methods, method relative_interior

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2021

Changed commit from 6869673 to 021d073

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2021

Changed commit from 021d073 to 8f38e04

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2021

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

8f38e04ConvexRationalPolyhedralCone.interior, relative_interior: Add doctests

@kliem
Copy link
Contributor

kliem commented Jun 7, 2021

Reviewer: Jonathan Kliem

@kliem
Copy link
Contributor

kliem commented Jun 7, 2021

comment:9

There are various small things:

The interior of a non-full-dimensional cone should be a cone, not a polyhedron.

Why do you implement relative_interior for relative interiors but not interior?

Why is closure defined for relative interiors, but not for polyhedra or cones? The problem is, that P.relative_interior().closure() will not always work.

Comparison will raise AttributeErrors, when other is the universe.

@kliem
Copy link
Contributor

kliem commented Jun 7, 2021

comment:10

#31919 solves the problem with closure.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 7, 2021

Changed commit from 8f38e04 to 5c089ec

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 7, 2021

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

5f9c852RelativeInterior.interior: New
5c089ecRelativeInterior.__eq__, __ne__: Handle comparisons with objects of other types

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 7, 2021

comment:12

Replying to @kliem:

The interior of a non-full-dimensional cone should be a cone, not a polyhedron.

Can't do - a ConvexRationalPolyhedralCone always contains 0

Why do you implement relative_interior for relative interiors but not interior?

Done

Why is closure defined for relative interiors, but not for polyhedra or cones? The problem is, that P.relative_interior().closure() will not always work.

For this ticket, it's just the accessor to the original polytope, not a more general API. See #31919 (ABC for convex sets), where I am adding such methods.

Comparison will raise AttributeErrors, when other is the universe.

Done

@kliem
Copy link
Contributor

kliem commented Jun 7, 2021

comment:14

LGTM.

@kliem
Copy link
Contributor

kliem commented Jun 7, 2021

comment:15

Replying to @mkoeppe:

Replying to @kliem:

The interior of a non-full-dimensional cone should be a cone, not a polyhedron.

Can't do - a ConvexRationalPolyhedralCone always contains 0

Right.

Why do you implement relative_interior for relative interiors but not interior?

Done

Why is closure defined for relative interiors, but not for polyhedra or cones? The problem is, that P.relative_interior().closure() will not always work.

For this ticket, it's just the accessor to the original polytope, not a more general API. See #31919 (ABC for convex sets), where I am adding such methods.

As noted above, I later discovered this on #31919 and it's fine to add it later on.

Comparison will raise AttributeErrors, when other is the universe.

Done

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 7, 2021

comment:16

Thanks! But I think I need to revise relative_interior to handle polyhedra that are already relatively open

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 7, 2021

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

86ce301Polyhedron_base.is_relatively_open: New; fix relative_interior for affine subspaces

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 7, 2021

Changed commit from 5c089ec to 86ce301

@kliem
Copy link
Contributor

kliem commented Jun 7, 2021

comment:19

I was locking for a boundary case, but yes, if a polyhedron is it's affine hull, then it either empty or the universe it it's affine hull.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 7, 2021

Changed commit from 86ce301 to 216cb81

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 7, 2021

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

216cb81ConvexRationalPolyhedralCone.is_relatively_open: New, fix ConvexRationalPolyhedralCone.relative_interior for linear subspaces

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 7, 2021

comment:21

Sorry, here's the same fix for cones

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 7, 2021

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

44cde1esrc/doc/en/reference/discrete_geometry/index.rst: Add sage/geometry/relative_interior

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 7, 2021

Changed commit from 216cb81 to 44cde1e

@kliem
Copy link
Contributor

kliem commented Jun 7, 2021

comment:23

Is there a reason for the emptylines:

+    def is_relatively_open(self):
+
+        r"""
+        Return whether ``self`` is relatively open.
+class RelativeInterior(SageObject):
+
+    r"""

Surround top-level function and class definitions with two blank lines.

+# ****************************************************************************
+
+from sage.structure.sage_object import SageObject
+
+class RelativeInterior(SageObject):

I'm not sure, if it also applies to the import.

Really tiny: Does a docstring ending with an example contain an empty line in the end? (Doesn't need to be fixed, I just noticed.)

Maybe for another ticket: Replace the ppl imports in cone.py by explicity imports to help pyflakes.

Otherwise it's a positive review.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 7, 2021

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

fa4c2d2Whitespace fixes

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 7, 2021

Changed commit from 44cde1e to fa4c2d2

@kliem
Copy link
Contributor

kliem commented Jun 8, 2021

comment:25

LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 8, 2021

comment:26

Thanks!

@vbraun
Copy link
Member

vbraun commented Jun 29, 2021

Changed branch from u/mkoeppe/polyhedron_relative_interior to fa4c2d2

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