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

Topological closure of manifold subsets, methods ManifoldSubset.closure, is_closed, declare_closed #31644

Closed
mkoeppe opened this issue Apr 10, 2021 · 59 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 10, 2021

We define a subclass of ManifoldSubset whose instances represents the topological closure of given subset in the manifold.

Subsets provide a method closure to construct it. When the subset is already closed, as detected by the new method is_closed, it just returns the input.

We also add a method declare_closed. It just sets up an open disjoint union with an open complement. (This is exactly what is_closed tests.)

The purpose of this is to build a connection of manifolds to cell complexes and convex polyhedra: In a separate ticket, we will define embedded submanifolds of euclidean spaces that arise as interiors of polyhedra or relative interiors of their faces.

Depends on #31763
Depends on #31798

CC: @egourgoulhon @tscrim @yuan-zhou @mjungmath

Component: manifolds

Author: Matthias Koeppe

Branch/Commit: 9abc617

Reviewer: Eric Gourgoulhon

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

@mkoeppe mkoeppe added this to the sage-9.3 milestone Apr 10, 2021
@mjungmath
Copy link

comment:1

That sounds extremely interesting!

Btw: Do you really mean cell complexes or rather simplicial complexes?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 11, 2021

comment:2

Simplicial is too narrow - that would not be enough (without triangulating) to represent general convex polyhedra and their boundary structure. So need something slightly more general.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 11, 2021

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 11, 2021

Author: Matthias Koeppe, ...

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 11, 2021

Commit: 331aa59

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 11, 2021

New commits:

331aa59sage.manifolds.closure_topological_submanifold, TopologicalSubmanifold.closure: New

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 11, 2021

comment:5

Here's a beginning.

Currently, is there already a way to get the image of the embedding of the submanifold in the ambient manifold as a "subset"?

@mjungmath
Copy link

comment:6

Replying to @mkoeppe:

New commits:

331aa59sage.manifolds.closure_topological_submanifold, TopologicalSubmanifold.closure: New

Looks already nice!

Replying to @mkoeppe:

Simplicial is too narrow - that would not be enough (without triangulating) to represent general convex polyhedra and their boundary structure. So need something slightly more general.

Right, the cube is already a counter-example.

Replying to @mkoeppe:

Here's a beginning.

Currently, is there already a way to get the image of the embedding of the submanifold in the ambient manifold as a "subset"?

No, I don't think so.

@egourgoulhon
Copy link
Member

comment:7

Replying to @mjungmath:

Replying to @mkoeppe:

Here's a beginning.

Currently, is there already a way to get the image of the embedding of the submanifold in the ambient manifold as a "subset"?

No, I don't think so.

I confirm; more generally, there is no such functionality for continuous maps.

@mjungmath
Copy link

comment:8

Speaking of boundary; I think we should slowly but surely consider to introduce manifolds with boundary...

@egourgoulhon
Copy link
Member

comment:9

Replying to @mjungmath:

Speaking of boundary; I think we should slowly but surely consider to introduce manifolds with boundary...

Indeed. There is even some demand for it: https://ask.sagemath.org/question/56532/.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 11, 2021

Dependencies: #31653

@mkoeppe mkoeppe changed the title Topological closure of embedded submanifolds Topological closure of manifold subsets, embedded submanifolds Apr 11, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 11, 2021

comment:11

Replying to @egourgoulhon:

Replying to @mjungmath:

Replying to @mkoeppe:

Currently, is there already a way to get the image of the embedding of the submanifold in the ambient manifold as a "subset"?

No, I don't think so.

I confirm; more generally, there is no such functionality for continuous maps.

OK, I have opened #31653 for this

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 11, 2021

Changed commit from 331aa59 to 0579188

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 11, 2021

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

0579188Fixup

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 11, 2021

Work Issues: redo on top of #31653

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Apr 11, 2021
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 29, 2021

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

26c7e56src/sage/manifolds/continuous_map_image.py: Update doctests
3e273bbTopologicalSubmanifold.as_subset: New
9726d36Docstring work
19762aeImageManifoldSubset: New parameter domain_subset, use it in ContinuousMap.image
964f9f7src/sage/manifolds/continuous_map.py: Update copyright
e711215src/sage/manifolds/continuous_map_image.py: Add tests
0f3e36dLink in documentation of sage.manifolds.continuous_map_image
218117bsage.manifolds.closure_topological_submanifold, TopologicalSubmanifold.closure: New
3b3662eFixup
0666faeChange to ClosureOfManifoldSubset

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 29, 2021

Changed commit from 0579188 to 0666fae

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 30, 2021

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

426e087ManifoldSubset.declare_{sub,super}set: New
72294f0ManifoldSubset.closure: New

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 30, 2021

Changed commit from 0666fae to 72294f0

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 30, 2021

Changed author from Matthias Koeppe, ... to Matthias Koeppe

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 30, 2021

Changed work issues from redo on top of #31653 to none

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 12, 2021

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

ac53984src/sage/manifolds/subsets/__init__.py: New

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 12, 2021

Changed commit from 9259a2c to ac53984

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 19, 2021

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

21739desrc/sage/manifolds/subset.py: Simplify code, make pyflakes happy
6bbd4aesrc/sage/manifolds/subset.py: Add coding header
2d7fda7src/sage/manifolds/subset.py: Simplify code more to make pyflakes happy
8d3ad85Merge #31764
60505b0Merge #31798

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 19, 2021

Changed commit from ac53984 to 60505b0

@mjungmath
Copy link

comment:35

One doctest has failed. Is that related to this ticket?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 30, 2021

comment:36

No, it's unrelated, see #31848

@egourgoulhon
Copy link
Member

comment:37

Looks nice!

There is a spurious line feed in the html documentation of ManifoldSubsetClosure, which can be fixed with

     - ``name`` -- (default: computed from the name of the subset)
-       string; name (symbol) given to the closure
+      string; name (symbol) given to the closure

Besides, there should be some EXAMPLES section in the main docstring of ManifoldSubsetClosure, and the latter should start with r""", instead of """, I think.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 2, 2021

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

b9909c0ManifoldSubsetClosure: Add examples, fix docstring markup

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 2, 2021

Changed commit from 60505b0 to b9909c0

@egourgoulhon
Copy link
Member

comment:39

I would not perform the import of ManifoldSubsetClosure in the EXAMPLES section, but rather construct the closure via the dedicated method closure(), since this is what the end user is supposed to do. In other words, I would rewrite the first part of the example as something like

sage: M = Manifold(2, 'R^2', structure='topological')
sage: c_cart.<x,y> = M.chart() # Cartesian coordinates on R^2
sage: D = M.open_subset('D', coord_def={c_cart: x^2+y^2<1}); D
Open subset D of the 2-dimensional topological manifold R^2
sage: cl_D = D.closure()
sage: cl_D
Topological closure cl_D of the Open subset D of the 2-dimensional
 topological manifold R^2
sage: latex(cl_D)
\mathop{\mathrm{cl}}(D)
sage: type(cl_D)
<class 'sage.manifolds.subsets.closure.ManifoldSubsetClosure_with_category'>
sage: cl_D.category()
Category of subobjects of sets

@egourgoulhon
Copy link
Member

comment:40

Also, in the docstring of ManifoldSubset.closure(), it would be nice to add an OUTPUT field as follows:

OUTPUT:

- an instance of :class:`~sage.manifolds.subsets.closure.ManifoldSubsetClosure`

This allows one to easily access to the documentation of class ManifoldSubsetClosure from that of closure(). For completeness, one may also add an INPUT field describing the arguments name and latex_name and their default values.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 3, 2021

Changed commit from b9909c0 to 16c6a72

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 3, 2021

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

16c6a72ManifoldSubsetClosure, ManifoldSubset.closure: Improve documentation

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 3, 2021

comment:42

Thanks for the suggestions, done.

@egourgoulhon
Copy link
Member

Reviewer: Eric Gourgoulhon

@egourgoulhon
Copy link
Member

comment:43

Thanks!

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 3, 2021

comment:44

Thank you!

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 19, 2021

Changed commit from 16c6a72 to 9abc617

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 19, 2021

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

359dde1Merge branch 't/31727/manifoldsubset__add_methods_subset_family__superset_family__equal_subset_family__deprecate_method_list_of_subsets' into t/31732/manifoldsubset__new_methods_declare_empty__declare_nonempty__is_empty__has_defined_points__open_cover_family
fd4506aMerge #31732
7fed9efMerge #31736
c4acd09Merge tag '9.4.beta2' into t/31763/manifoldsubset__new_methods_declare_subset__declare_superset
2ba18abMerge #31763
0e9e5cdMerge #31732
a96f736Merge #31736
582b58dMerge #31763
2113f78Merge #31764
9abc617Merge #31798

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 19, 2021

comment:46

Trivial merge of updated dependency #31763

@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