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: | 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: |
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