Opened 14 months ago
Closed 14 months ago
#31959 closed enhancement (fixed)
PolyhedronFace: Make it a subclass of ConvexSet_closed
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  geometry  Keywords:  
Cc:  ghkliem, 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: 
Description (last modified by )
... 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 ifambient
is only a free module).
Change History (27)
comment:1 Changed 14 months ago by
 Branch set to u/mkoeppe/polyhedronface__make_it_a_subclass_of_convexset_closed
comment:2 Changed 14 months ago by
 Commit set to 8d77b3e67c27e24ccae8e529eddb4beece0e8a30
comment:3 Changed 14 months ago by
 Cc tscrim added
 Description modified (diff)
 Status changed from new to needs_review
comment:4 Changed 14 months ago by
 Commit changed from 8d77b3e67c27e24ccae8e529eddb4beece0e8a30 to e66448ea0b3de1286595ef4af19fe95c5d9ef18e
comment:5 Changed 14 months ago by
 Description modified (diff)
comment:6 Changed 14 months ago by
 Description modified (diff)
comment:7 Changed 14 months ago by
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 14 months ago by
 Commit changed from e66448ea0b3de1286595ef4af19fe95c5d9ef18e to 1f7826d757f80ba7bae0aa64fb34c57206b2ad8f
Branch pushed to git repo; I updated commit sha1. New commits:
1f7826d  ConvexSet_base._test_contains: Expand and add doctests

comment:9 Changed 14 months ago by
 Commit changed from 1f7826d757f80ba7bae0aa64fb34c57206b2ad8f to 669cea3814762b1441bfcd777a31465df912039a
Branch pushed to git repo; I updated commit sha1. New commits:
142fb46  ConvexSet_base.codimension: Add doctest for alias 'codim'

30ebaf5  ConvexSet_base.ambient_dim, ambient_dimension: Fix doc

fcf2a32  fix coverage

6bef52b  ConvexSet_base._test_convex_set: Run the testsuite of relint

1448835  RelativeInterior.ambient, ambient_vector_space, is_universe: New

8ff6457  Polyhedron_base.interior: Handle the empty polyhedron correctly

148016f  Polyhedron_base.product: Add doctest for alias 'cartesian_product'

669cea3  Merge #31919

comment:10 Changed 14 months ago by
 Commit changed from 669cea3814762b1441bfcd777a31465df912039a to 2b1d108f1c5c788af1699943d8c79a60bffbf570
comment:11 Changed 14 months ago by
 Commit changed from 2b1d108f1c5c788af1699943d8c79a60bffbf570 to 7323b10d6c628c32adfeb62025879f909f707c61
Branch pushed to git repo; I updated commit sha1. New commits:
7323b10  ConvexSet_base._test_contains: Only test extension to AA for exact base rings

comment:12 Changed 14 months ago by
 Commit changed from 7323b10d6c628c32adfeb62025879f909f707c61 to 94e68582fde0f5bb8b2c57e5f7aa56dbaa5e825a
Branch pushed to git repo; I updated commit sha1. New commits:
94e6858  RelativeInterior.ambient, ambient_vector_space, is_universe: New

comment:13 Changed 14 months ago by
 Commit changed from 94e68582fde0f5bb8b2c57e5f7aa56dbaa5e825a to 0c9bc945a59ffbf38c59d3679036bf4c90a516fd
Branch pushed to git repo; I updated commit sha1. New commits:
0c9bc94  ConvexSet_base: Add default implementations of ambient, ambient_dim; add doctests

comment:14 Changed 14 months ago by
 Commit changed from 0c9bc945a59ffbf38c59d3679036bf4c90a516fd to ce91e44231c897ad00c4d838d2e4f081afcc6ff9
Branch pushed to git repo; I updated commit sha1. New commits:
ce91e44  src/sage/geometry/relative_interior.py: Fix doctest output

comment:16 Changed 14 months ago by
 Commit changed from ce91e44231c897ad00c4d838d2e4f081afcc6ff9 to afa67ab5f370cf53e9c39229d1c83b871f04ba6a
Branch pushed to git repo; I updated commit sha1. New commits:
afa67ab  ambient_vector_space docstring: Fix bad blocks

comment:17 Changed 14 months ago by
 Commit changed from afa67ab5f370cf53e9c39229d1c83b871f04ba6a to f0e7c5864eeea17190b0657accf9534fcafa0c89
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f0e7c58  ambient_vector_space docstring: Fix bad blocks

comment:18 Changed 14 months ago by
 Commit changed from f0e7c5864eeea17190b0657accf9534fcafa0c89 to 200d967982e2d4c600658354ef80a15f1ed0ccd8
Branch pushed to git repo; I updated commit sha1. New commits:
200d967  ConvexSet_base.ambient doctest: Actually test the method

comment:20 Changed 14 months ago by
Thank you!
comment:21 Changed 14 months ago by
 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 14 months ago by
 Commit changed from 200d967982e2d4c600658354ef80a15f1ed0ccd8 to f02ca284d4c7b886f5b185db5e6b6d6a8bc4a039
Branch pushed to git repo; I updated commit sha1. New commits:
f02ca28  src/sage/geometry/polyhedron/face.py: Remove unused import

comment:23 Changed 14 months ago by
 Status changed from needs_work to needs_review
comment:24 Changed 14 months ago by
 Status changed from needs_review to positive_review
comment:25 Changed 14 months ago by
It was positive before and the unused import was the only complaint.
comment:26 Changed 14 months ago by
Thanks!
comment:27 Changed 14 months ago by
 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
Branch pushed to git repo; I updated commit sha1. New commits:
PolyhedronFace.is_relatively_open, is_compact: New, add doctests