Opened 5 months ago

Closed 4 months ago

#31916 closed enhancement (fixed)

{Polyhedron, ConvexRationalPolyhedralCone}.{interior, relative_interior}

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.4
Component: geometry Keywords:
Cc: gh-kliem, yzh, jipilab, tscrim, novoselt, mjo Merged in:
Authors: Matthias Koeppe Reviewers: Jonathan Kliem
Report Upstream: N/A Work issues:
Branch: fa4c2d2 (Commits, GitHub, GitLab) Commit: fa4c2d238a8f16a61f02e7cfa89ba3f17d8f4980
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

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.

Change History (27)

comment:1 Changed 5 months ago by mkoeppe

  • Authors set to Matthias Koeppe

comment:2 Changed 5 months ago by mkoeppe

  • Branch set to u/mkoeppe/polyhedron_relative_interior

comment:3 Changed 5 months ago by mkoeppe

  • Cc novoselt mjo added
  • Commit set to e9c670c00a492f8f50f3e21e55895a0d3c1d55f2
  • Description modified (diff)
  • Summary changed from Polyhedron.relative_interior to {Polyhedron, ConvexRationalPolyhedralCone}.{interior, relative_interior}

New commits:

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

comment:4 Changed 5 months ago by git

  • Commit changed from e9c670c00a492f8f50f3e21e55895a0d3c1d55f2 to b8bfe200f6e698dec855394af9adaaa63c282abd

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

comment:5 Changed 5 months ago by git

  • Commit changed from b8bfe200f6e698dec855394af9adaaa63c282abd to 6869673199fb80af0a09caf039873e27d50d2741

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

6869673relative_interior: Fix for dimension 0

comment:6 Changed 5 months ago by git

  • Commit changed from 6869673199fb80af0a09caf039873e27d50d2741 to 021d073e758d37f0abdb2466e3dc59e3383ed05b

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

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

comment:7 Changed 5 months ago by git

  • Commit changed from 021d073e758d37f0abdb2466e3dc59e3383ed05b to 8f38e0475edfd5913bf25ede90162b04597db64c

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

8f38e04ConvexRationalPolyhedralCone.interior, relative_interior: Add doctests

comment:8 Changed 5 months ago by mkoeppe

  • Status changed from new to needs_review

comment:9 follow-up: Changed 5 months ago by gh-kliem

  • Reviewers set to Jonathan Kliem
  • Status changed from needs_review to needs_work

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.

comment:10 Changed 5 months ago by gh-kliem

#31919 solves the problem with closure.

comment:11 Changed 5 months ago by git

  • Commit changed from 8f38e0475edfd5913bf25ede90162b04597db64c to 5c089ec4926664d426e5d46735cb8c1a9b1d1017

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

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

comment:12 in reply to: ↑ 9 ; follow-up: Changed 5 months ago by mkoeppe

Replying to gh-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

comment:13 Changed 5 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:14 Changed 5 months ago by gh-kliem

  • Status changed from needs_review to positive_review

LGTM.

comment:15 in reply to: ↑ 12 Changed 5 months ago by gh-kliem

Replying to mkoeppe:

Replying to gh-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

comment:16 Changed 5 months ago by mkoeppe

  • Status changed from positive_review to needs_work

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

comment:17 Changed 5 months ago by git

  • Commit changed from 5c089ec4926664d426e5d46735cb8c1a9b1d1017 to 86ce301c02936f4b56f6ab821520e8542bd5fbc1

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

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

comment:18 Changed 5 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:19 Changed 5 months ago by gh-kliem

  • Status changed from needs_review to positive_review

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.

comment:20 Changed 5 months ago by git

  • Commit changed from 86ce301c02936f4b56f6ab821520e8542bd5fbc1 to 216cb811d16bdf1bedfb2d7e04339ed4387f7212
  • Status changed from positive_review to needs_review

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

comment:21 Changed 5 months ago by mkoeppe

Sorry, here's the same fix for cones

comment:22 Changed 5 months ago by git

  • Commit changed from 216cb811d16bdf1bedfb2d7e04339ed4387f7212 to 44cde1e3dbea9086e75f235c0f61164278e4e451

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

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

comment:23 Changed 5 months ago by gh-kliem

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.

comment:24 Changed 5 months ago by git

  • Commit changed from 44cde1e3dbea9086e75f235c0f61164278e4e451 to fa4c2d238a8f16a61f02e7cfa89ba3f17d8f4980

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

fa4c2d2Whitespace fixes

comment:25 Changed 5 months ago by gh-kliem

  • Status changed from needs_review to positive_review

LGTM.

comment:26 Changed 5 months ago by mkoeppe

Thanks!

comment:27 Changed 4 months ago by vbraun

  • Branch changed from u/mkoeppe/polyhedron_relative_interior to fa4c2d238a8f16a61f02e7cfa89ba3f17d8f4980
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.