Opened 4 months ago

Closed 4 months ago

#31959 closed enhancement (fixed)

PolyhedronFace: Make it a subclass of ConvexSet_closed

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.4
Component: geometry Keywords:
Cc: gh-kliem, jipilab, tscrim Merged in:
Authors: Matthias Koeppe Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: f02ca28 (Commits, GitHub, GitLab) Commit: f02ca284d4c7b886f5b185db5e6b6d6a8bc4a039
Dependencies: #31919 Stopgaps:

Status badges

Description (last modified by mkoeppe)

... we only need to be careful with the method ambient_dim, which is the dimension of the polyhedron, not of the space.

So in this ticket we add to the API defined by ConvexSet_base:

  • the method ambient, which is allowed to be a containing convex set, not necessarily a space,
  • the method ambient_vector_space, which is always a vector space (even if ambient is only a free module).

Change History (27)

comment:1 Changed 4 months ago by mkoeppe

  • Branch set to u/mkoeppe/polyhedronface__make_it_a_subclass_of_convexset_closed

comment:2 Changed 4 months ago by git

  • Commit set to 8d77b3e67c27e24ccae8e529eddb4beece0e8a30

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

​8d77b3ePolyhedronFace.is_relatively_open, is_compact: New, add doctests

comment:3 Changed 4 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Cc tscrim added
  • Description modified (diff)
  • Status changed from new to needs_review

comment:4 Changed 4 months ago by git

  • Commit changed from 8d77b3e67c27e24ccae8e529eddb4beece0e8a30 to e66448ea0b3de1286595ef4af19fe95c5d9ef18e

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

​6ba5d9cConvexSet_base.ambient_vector_space: New
​e66448ePolyhedronFace.contains, ConvexSet_base._test_contains: New

comment:5 Changed 4 months ago by mkoeppe

  • Description modified (diff)

comment:6 Changed 4 months ago by mkoeppe

  • Description modified (diff)

comment:7 Changed 4 months ago by tscrim

Every function needs a doctest, even if they are just @abstract_method stubs. (This annoys me too at times, but it is the current policy. Although it is good for showing how an implementation should (or does) work.) One in particular is ConvexSet_base._test_contains needs a doctest (since it was added in e66448e).

comment:8 Changed 4 months ago by git

  • Commit changed from e66448ea0b3de1286595ef4af19fe95c5d9ef18e to 1f7826d757f80ba7bae0aa64fb34c57206b2ad8f

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

​1f7826dConvexSet_base._test_contains: Expand and add doctests

comment:9 Changed 4 months ago by git

  • Commit changed from 1f7826d757f80ba7bae0aa64fb34c57206b2ad8f to 669cea3814762b1441bfcd777a31465df912039a

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

​142fb46ConvexSet_base.codimension: Add doctest for alias 'codim'
​30ebaf5ConvexSet_base.ambient_dim, ambient_dimension: Fix doc
​fcf2a32fix coverage
​6bef52bConvexSet_base._test_convex_set: Run the testsuite of relint
​1448835RelativeInterior.ambient, ambient_vector_space, is_universe: New
​8ff6457Polyhedron_base.interior: Handle the empty polyhedron correctly
​148016fPolyhedron_base.product: Add doctest for alias 'cartesian_product'
​669cea3Merge #31919

comment:10 Changed 4 months ago by git

  • Commit changed from 669cea3814762b1441bfcd777a31465df912039a to 2b1d108f1c5c788af1699943d8c79a60bffbf570

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​6ab5677RelativeInterior.is_universe: New
​c085d30Polyhedron_base.interior: Handle the empty polyhedron correctly
​686d0afPolyhedron_base.product: Add doctest for alias 'cartesian_product'
​2b1d108Merge #31919

comment:11 Changed 4 months ago by git

  • Commit changed from 2b1d108f1c5c788af1699943d8c79a60bffbf570 to 7323b10d6c628c32adfeb62025879f909f707c61

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

​7323b10ConvexSet_base._test_contains: Only test extension to AA for exact base rings

comment:12 Changed 4 months ago by git

  • Commit changed from 7323b10d6c628c32adfeb62025879f909f707c61 to 94e68582fde0f5bb8b2c57e5f7aa56dbaa5e825a

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

​94e6858RelativeInterior.ambient, ambient_vector_space, is_universe: New

comment:13 Changed 4 months ago by git

  • Commit changed from 94e68582fde0f5bb8b2c57e5f7aa56dbaa5e825a to 0c9bc945a59ffbf38c59d3679036bf4c90a516fd

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

​0c9bc94ConvexSet_base: Add default implementations of ambient, ambient_dim; add doctests

comment:14 Changed 4 months ago by git

  • Commit changed from 0c9bc945a59ffbf38c59d3679036bf4c90a516fd to ce91e44231c897ad00c4d838d2e4f081afcc6ff9

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

​ce91e44src/sage/geometry/relative_interior.py: Fix doctest output

comment:15 Changed 4 months ago by tscrim

  • Reviewers set to Travis Scrimshaw

Green bot => positive review

comment:16 Changed 4 months ago by git

  • Commit changed from ce91e44231c897ad00c4d838d2e4f081afcc6ff9 to afa67ab5f370cf53e9c39229d1c83b871f04ba6a

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

​afa67abambient_vector_space docstring: Fix bad blocks

comment:17 Changed 4 months ago by git

  • Commit changed from afa67ab5f370cf53e9c39229d1c83b871f04ba6a to f0e7c5864eeea17190b0657accf9534fcafa0c89

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​f0e7c58ambient_vector_space docstring: Fix bad blocks

comment:18 Changed 4 months ago by git

  • Commit changed from f0e7c5864eeea17190b0657accf9534fcafa0c89 to 200d967982e2d4c600658354ef80a15f1ed0ccd8

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

​200d967ConvexSet_base.ambient doctest: Actually test the method

comment:19 Changed 4 months ago by tscrim

  • Status changed from needs_review to positive_review

LGTM.

comment:20 Changed 4 months ago by mkoeppe

Thank you!

comment:21 Changed 4 months ago by gh-kliem

  • Status changed from positive_review to needs_work

Can you please remove the import of SageObject in face.py.

It's redundant now.

comment:22 Changed 4 months ago by git

  • Commit changed from 200d967982e2d4c600658354ef80a15f1ed0ccd8 to f02ca284d4c7b886f5b185db5e6b6d6a8bc4a039

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

​f02ca28src/sage/geometry/polyhedron/face.py: Remove unused import

comment:23 Changed 4 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:24 Changed 4 months ago by gh-kliem

  • Status changed from needs_review to positive_review

comment:25 Changed 4 months ago by gh-kliem

It was positive before and the unused import was the only complaint.

comment:26 Changed 4 months ago by mkoeppe

Thanks!

comment:27 Changed 4 months ago by vbraun

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