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

PolyhedronFace: Make it a subclass of ConvexSet_closed #31959

Closed
mkoeppe opened this issue Jun 11, 2021 · 40 comments
Closed

PolyhedronFace: Make it a subclass of ConvexSet_closed #31959

mkoeppe opened this issue Jun 11, 2021 · 40 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Jun 11, 2021

... 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).

Depends on #31919

CC: @kliem @jplab @tscrim

Component: geometry

Author: Matthias Koeppe

Branch/Commit: f02ca28

Reviewer: Travis Scrimshaw

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

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

mkoeppe commented Jun 11, 2021

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2021

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

8d77b3ePolyhedronFace.is_relatively_open, is_compact: New, add doctests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2021

Commit: 8d77b3e

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 11, 2021

Author: Matthias Koeppe

@mkoeppe

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2021

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 11, 2021

Changed commit from 8d77b3e to e66448e

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Jun 12, 2021

comment:7

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).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2021

Changed commit from e66448e to 1f7826d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2021

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

1f7826dConvexSet_base._test_contains: Expand and add doctests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2021

Changed commit from 1f7826d to 669cea3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2021

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2021

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2021

Changed commit from 669cea3 to 2b1d108

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2021

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2021

Changed commit from 2b1d108 to 7323b10

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 16, 2021

Changed commit from 7323b10 to 94e6858

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 16, 2021

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

94e6858RelativeInterior.ambient, ambient_vector_space, is_universe: New

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 16, 2021

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 16, 2021

Changed commit from 94e6858 to 0c9bc94

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 17, 2021

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 17, 2021

Changed commit from 0c9bc94 to ce91e44

@tscrim
Copy link
Collaborator

tscrim commented Jun 17, 2021

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jun 17, 2021

comment:15

Green bot => positive review

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 17, 2021

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

afa67abambient_vector_space docstring: Fix bad blocks

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 17, 2021

Changed commit from ce91e44 to afa67ab

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 17, 2021

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

f0e7c58ambient_vector_space docstring: Fix bad blocks

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 17, 2021

Changed commit from afa67ab to f0e7c58

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 17, 2021

Changed commit from f0e7c58 to 200d967

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 17, 2021

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

200d967ConvexSet_base.ambient doctest: Actually test the method

@tscrim
Copy link
Collaborator

tscrim commented Jun 17, 2021

comment:19

LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 17, 2021

comment:20

Thank you!

@kliem
Copy link
Contributor

kliem commented Jun 18, 2021

comment:21

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

It's redundant now.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 18, 2021

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 18, 2021

Changed commit from 200d967 to f02ca28

@kliem
Copy link
Contributor

kliem commented Jun 19, 2021

comment:25

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 19, 2021

comment:26

Thanks!

@vbraun
Copy link
Member

vbraun commented Jun 29, 2021

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

4 participants