Opened 20 months ago
Closed 17 months ago
#31644 closed enhancement (fixed)
Topological closure of manifold subsets, methods ManifoldSubset.closure, is_closed, declare_closed
Reported by:  Matthias Köppe  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  manifolds  Keywords:  
Cc:  Eric Gourgoulhon, Travis Scrimshaw, Yuan Zhou, Michael Jung  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 20 months ago by
comment:2 Changed 20 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 20 months ago by
Branch:  → u/mkoeppe/topological_closure_of_embedded_submanifolds 

comment:4 followup: 6 Changed 20 months ago by
Authors:  → Matthias Koeppe, ... 

Commit:  → 331aa59f61c065ac9b6237e5d37ae59ba652ece6 
New commits:
331aa59  sage.manifolds.closure_topological_submanifold, TopologicalSubmanifold.closure: New

comment:5 Changed 20 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 followup: 7 Changed 20 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 followup: 11 Changed 20 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 20 months ago by
Speaking of boundary; I think we should slowly but surely consider to introduce manifolds with boundary...
comment:9 Changed 20 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 20 months ago by
Dependencies:  → #31653 

Summary:  Topological closure of embedded submanifolds → Topological closure of manifold subsets, embedded submanifolds 
comment:11 Changed 20 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 20 months ago by
Commit:  331aa59f61c065ac9b6237e5d37ae59ba652ece6 → 0579188638bb4f858976e9e8682f5c854e71eb66 

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

comment:13 Changed 20 months ago by
Work issues:  → redo on top of #31653 

comment:14 Changed 20 months ago by
Milestone:  sage9.3 → sage9.4 

comment:15 Changed 19 months ago by
Commit:  0579188638bb4f858976e9e8682f5c854e71eb66 → 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 19 months ago by
Commit:  0666fae89c698e792b534c81395bb42deb2b3151 → 72294f04f8a99651a74f8629cdf3ba1278ffc0f4 

comment:17 Changed 19 months ago by
Authors:  Matthias Koeppe, ... → Matthias Koeppe 

Description:  modified (diff) 
Summary:  Topological closure of manifold subsets, embedded submanifolds → Topological closure of manifold subsets 
Work issues:  redo on top of #31653 
comment:18 Changed 19 months ago by
Dependencies:  #31653 → #31653, #31763 

comment:19 Changed 19 months ago by
Summary:  Topological closure of manifold subsets → Topological closure of manifold subsets, methods ManifoldSubset.closure, is_closed 

comment:20 Changed 19 months ago by
Description:  modified (diff) 

comment:21 Changed 19 months ago by
Dependencies:  #31653, #31763 → #31653, #31763, #31798 

comment:22 Changed 19 months ago by
Commit:  72294f04f8a99651a74f8629cdf3ba1278ffc0f4 → 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 19 months ago by
Status:  new → needs_review 

comment:24 Changed 19 months ago by
Description:  modified (diff) 

Summary:  Topological closure of manifold subsets, methods ManifoldSubset.closure, is_closed → Topological closure of manifold subsets, methods ManifoldSubset.closure, is_closed, declare_closed 
comment:25 Changed 19 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 19 months ago by
Status:  needs_review → needs_work 

Looks nice, but there are several issues reported by the patchbot, including doctest failures.
comment:28 Changed 19 months ago by
Dependencies:  #31653, #31763, #31798 → #31763, #31798 

comment:29 Changed 19 months ago by
Commit:  0faec94bad521406c4f4f2da04ef851051504ec9 → 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:31 Changed 19 months ago by
Status:  needs_work → needs_review 

comment:32 Changed 19 months ago by
Commit:  38c9e96e08560fbf7fa28397bba2e46b16e87e0e → 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 19 months ago by
Commit:  9259a2c109ce9ac069e80f2c5aa090fb533e1690 → ac5398499c314b8a59b5994c2cfa8b5945f3b311 

Branch pushed to git repo; I updated commit sha1. New commits:
ac53984  src/sage/manifolds/subsets/__init__.py: New

comment:34 Changed 19 months ago by
Commit:  ac5398499c314b8a59b5994c2cfa8b5945f3b311 → 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:37 Changed 18 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 18 months ago by
Commit:  60505b09c0a05e08282020ef1284db5ecff8ce6f → b9909c061046a204354341833e3b3fcb4c6d7341 

Branch pushed to git repo; I updated commit sha1. New commits:
b9909c0  ManifoldSubsetClosure: Add examples, fix docstring markup

comment:39 Changed 18 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 18 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 18 months ago by
Commit:  b9909c061046a204354341833e3b3fcb4c6d7341 → 16c6a7219848d2045fe92b1ca50eb0195a05d91f 

Branch pushed to git repo; I updated commit sha1. New commits:
16c6a72  ManifoldSubsetClosure, ManifoldSubset.closure: Improve documentation

comment:43 Changed 18 months ago by
Reviewers:  → Eric Gourgoulhon 

Status:  needs_review → positive_review 
Thanks!
comment:45 Changed 18 months ago by
Commit:  16c6a7219848d2045fe92b1ca50eb0195a05d91f → 9abc6174394c85ca400650c814a3d27e7921729d 

Status:  positive_review → 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 18 months ago by
Status:  needs_review → positive_review 

Trivial merge of updated dependency #31763
comment:47 Changed 17 months ago by
Branch:  u/mkoeppe/topological_closure_of_embedded_submanifolds → 9abc6174394c85ca400650c814a3d27e7921729d 

Resolution:  → fixed 
Status:  positive_review → closed 
That sounds extremely interesting!
Btw: Do you really mean cell complexes or rather simplicial complexes?