Opened 6 months ago
Closed 4 months ago
#31644 closed enhancement (fixed)
Topological closure of manifold subsets, methods ManifoldSubset.closure, is_closed, declare_closed
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  manifolds  Keywords:  
Cc:  egourgoulhon, tscrim, yzh, ghmjungmath  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Eric Gourgoulhon 
Report Upstream:  N/A  Work issues:  
Branch:  9abc617 (Commits, GitHub, GitLab)  Commit:  9abc6174394c85ca400650c814a3d27e7921729d 
Dependencies:  #31763, #31798  Stopgaps: 
Description (last modified by )
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.
Change History (47)
comment:1 Changed 6 months ago by
comment:2 Changed 6 months ago by
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.
comment:3 Changed 6 months ago by
 Branch set to u/mkoeppe/topological_closure_of_embedded_submanifolds
comment:4 followup: ↓ 6 Changed 6 months ago by
 Commit set to 331aa59f61c065ac9b6237e5d37ae59ba652ece6
New commits:
331aa59  sage.manifolds.closure_topological_submanifold, TopologicalSubmanifold.closure: New

comment:5 Changed 6 months ago by
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"?
comment:6 in reply to: ↑ 4 ; followup: ↓ 7 Changed 6 months ago by
Replying to mkoeppe:
New commits:
331aa59 sage.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 counterexample.
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.
comment:7 in reply to: ↑ 6 ; followup: ↓ 11 Changed 6 months ago by
Replying to ghmjungmath:
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.
comment:8 followup: ↓ 9 Changed 6 months ago by
Speaking of boundary; I think we should slowly but surely consider to introduce manifolds with boundary...
comment:9 in reply to: ↑ 8 Changed 6 months ago by
Replying to ghmjungmath:
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/.
comment:10 Changed 6 months ago by
 Dependencies set to #31653
 Summary changed from Topological closure of embedded submanifolds to Topological closure of manifold subsets, embedded submanifolds
comment:11 in reply to: ↑ 7 Changed 6 months ago by
Replying to egourgoulhon:
Replying to ghmjungmath:
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
comment:12 Changed 6 months ago by
 Commit changed from 331aa59f61c065ac9b6237e5d37ae59ba652ece6 to 0579188638bb4f858976e9e8682f5c854e71eb66
Branch pushed to git repo; I updated commit sha1. New commits:
0579188  Fixup

comment:13 Changed 6 months ago by
 Work issues set to redo on top of #31653
comment:14 Changed 6 months ago by
 Milestone changed from sage9.3 to sage9.4
comment:15 Changed 6 months ago by
 Commit changed from 0579188638bb4f858976e9e8682f5c854e71eb66 to 0666fae89c698e792b534c81395bb42deb2b3151
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
26c7e56  src/sage/manifolds/continuous_map_image.py: Update doctests

3e273bb  TopologicalSubmanifold.as_subset: New

9726d36  Docstring work

19762ae  ImageManifoldSubset: New parameter domain_subset, use it in ContinuousMap.image

964f9f7  src/sage/manifolds/continuous_map.py: Update copyright

e711215  src/sage/manifolds/continuous_map_image.py: Add tests

0f3e36d  Link in documentation of sage.manifolds.continuous_map_image

218117b  sage.manifolds.closure_topological_submanifold, TopologicalSubmanifold.closure: New

3b3662e  Fixup

0666fae  Change to ClosureOfManifoldSubset

comment:16 Changed 6 months ago by
 Commit changed from 0666fae89c698e792b534c81395bb42deb2b3151 to 72294f04f8a99651a74f8629cdf3ba1278ffc0f4
comment:17 Changed 6 months ago by
 Description modified (diff)
 Summary changed from Topological closure of manifold subsets, embedded submanifolds to Topological closure of manifold subsets
 Work issues redo on top of #31653 deleted
comment:18 Changed 5 months ago by
 Dependencies changed from #31653 to #31653, #31763
comment:19 Changed 5 months ago by
 Summary changed from Topological closure of manifold subsets to Topological closure of manifold subsets, methods ManifoldSubset.closure, is_closed
comment:20 Changed 5 months ago by
 Description modified (diff)
comment:21 Changed 5 months ago by
 Dependencies changed from #31653, #31763 to #31653, #31763, #31798
comment:22 Changed 5 months ago by
 Commit changed from 72294f04f8a99651a74f8629cdf3ba1278ffc0f4 to 0faec94bad521406c4f4f2da04ef851051504ec9
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
6772570  ManifoldSubset.union: Handle arbitrary unions

edfea33  ManifoldSubset.union: Add example and plots

fdd0aba  ManifoldSubset.declare_union: Handle arbitrary unions

c2f484c  src/sage/manifolds/subset.py: import itertools

a083569  ManifoldSubset._intersection_subset, _union_subset: Use declare_subset, declare_superset; fix doctest

da91653  ManifoldSubset._union_subset: Fixup

3e6a9cf  Merge #31764

3786cee  ManifoldSubset.difference, complement: New

decd831  Merge #31798

0faec94  ManifoldSubset.is_closed, declare_closed: New

comment:23 Changed 5 months ago by
 Status changed from new to needs_review
comment:24 Changed 5 months ago by
 Description modified (diff)
 Summary changed from Topological closure of manifold subsets, methods ManifoldSubset.closure, is_closed to Topological closure of manifold subsets, methods ManifoldSubset.closure, is_closed, declare_closed
comment:25 Changed 5 months ago by
I have put the new class in a new subpackage sage.manifolds.subsets
(see https://trac.sagemath.org/ticket/30139#comment:33)
comment:26 Changed 5 months ago by
 Status changed from needs_review to needs_work
Looks nice, but there are several issues reported by the patchbot, including doctest failures.
comment:27 Changed 5 months ago by
Looks like the failures are originated in #31798.
comment:28 Changed 5 months ago by
 Dependencies changed from #31653, #31763, #31798 to #31763, #31798
comment:29 Changed 5 months ago by
 Commit changed from 0faec94bad521406c4f4f2da04ef851051504ec9 to 38c9e96e08560fbf7fa28397bba2e46b16e87e0e
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
94edd68  ManifoldSubset.declare_superset: Fix documentation

41826b4  ManifoldSubset.declare_{sub,super}set: Expand docstring

1c14655  Merge #31763

6370e88  ManifoldSubset.difference, complement: New

73eceac  Merge #31798

2028258  sage.manifolds.subsets.closure, TopologicalSubmanifold.closure: New

38c9e96  ManifoldSubset.is_closed, declare_closed: New

comment:30 Changed 5 months ago by
comment:31 Changed 5 months ago by
 Status changed from needs_work to needs_review
comment:32 Changed 5 months ago by
 Commit changed from 38c9e96e08560fbf7fa28397bba2e46b16e87e0e to 9259a2c109ce9ac069e80f2c5aa090fb533e1690
Branch pushed to git repo; I updated commit sha1. New commits:
3ef5141  Merge #31763

79a625b  ManifoldSubset._reduce_intersection_members: Add examples, raise TypeError if input is empty family

8e70023  ManifoldSubset._reduce_union_members: Add examples

ee4efc9  ManifoldSubset._{union,intersection}_subset: Do cache the result here; add examples

e7f7a7d  Merge #31764

9259a2c  Merge #31798

comment:33 Changed 5 months ago by
 Commit changed from 9259a2c109ce9ac069e80f2c5aa090fb533e1690 to ac5398499c314b8a59b5994c2cfa8b5945f3b311
Branch pushed to git repo; I updated commit sha1. New commits:
ac53984  src/sage/manifolds/subsets/__init__.py: New

comment:34 Changed 5 months ago by
 Commit changed from ac5398499c314b8a59b5994c2cfa8b5945f3b311 to 60505b09c0a05e08282020ef1284db5ecff8ce6f
Branch pushed to git repo; I updated commit sha1. New commits:
21739de  src/sage/manifolds/subset.py: Simplify code, make pyflakes happy

6bbd4ae  src/sage/manifolds/subset.py: Add coding header

2d7fda7  src/sage/manifolds/subset.py: Simplify code more to make pyflakes happy

8d3ad85  Merge #31764

60505b0  Merge #31798

comment:35 Changed 5 months ago by
One doctest has failed. Is that related to this ticket?
comment:36 Changed 5 months ago by
No, it's unrelated, see #31848
comment:37 Changed 5 months ago by
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.
comment:38 Changed 5 months ago by
 Commit changed from 60505b09c0a05e08282020ef1284db5ecff8ce6f to b9909c061046a204354341833e3b3fcb4c6d7341
Branch pushed to git repo; I updated commit sha1. New commits:
b9909c0  ManifoldSubsetClosure: Add examples, fix docstring markup

comment:39 Changed 5 months ago by
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 2dimensional topological manifold R^2 sage: cl_D = D.closure() sage: cl_D Topological closure cl_D of the Open subset D of the 2dimensional 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
comment:40 Changed 5 months ago by
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.
comment:41 Changed 5 months ago by
 Commit changed from b9909c061046a204354341833e3b3fcb4c6d7341 to 16c6a7219848d2045fe92b1ca50eb0195a05d91f
Branch pushed to git repo; I updated commit sha1. New commits:
16c6a72  ManifoldSubsetClosure, ManifoldSubset.closure: Improve documentation

comment:42 Changed 5 months ago by
Thanks for the suggestions, done.
comment:43 Changed 5 months ago by
 Reviewers set to Eric Gourgoulhon
 Status changed from needs_review to positive_review
Thanks!
comment:44 Changed 5 months ago by
Thank you!
comment:45 Changed 4 months ago by
 Commit changed from 16c6a7219848d2045fe92b1ca50eb0195a05d91f to 9abc6174394c85ca400650c814a3d27e7921729d
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
359dde1  Merge 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

fd4506a  Merge #31732

7fed9ef  Merge #31736

c4acd09  Merge tag '9.4.beta2' into t/31763/manifoldsubset__new_methods_declare_subset__declare_superset

2ba18ab  Merge #31763

0e9e5cd  Merge #31732

a96f736  Merge #31736

582b58d  Merge #31763

2113f78  Merge #31764

9abc617  Merge #31798

comment:46 Changed 4 months ago by
 Status changed from needs_review to positive_review
Trivial merge of updated dependency #31763
comment:47 Changed 4 months ago by
 Branch changed from u/mkoeppe/topological_closure_of_embedded_submanifolds to 9abc6174394c85ca400650c814a3d27e7921729d
 Resolution set to fixed
 Status changed from positive_review to closed
That sounds extremely interesting!
Btw: Do you really mean cell complexes or rather simplicial complexes?