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: sage-9.4
Component: manifolds Keywords:
Cc: egourgoulhon, tscrim, yzh, gh-mjungmath 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:

Status badges

Description (last modified by mkoeppe)

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 gh-mjungmath

That sounds extremely interesting!

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

comment:2 Changed 6 months ago by 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.

comment:3 Changed 6 months ago by mkoeppe

  • Branch set to u/mkoeppe/topological_closure_of_embedded_submanifolds

comment:4 follow-up: Changed 6 months ago by mkoeppe

  • Authors set to Matthias Koeppe, ...
  • Commit set to 331aa59f61c065ac9b6237e5d37ae59ba652ece6

New commits:

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

comment:5 Changed 6 months ago by 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"?

comment:6 in reply to: ↑ 4 ; follow-up: Changed 6 months ago by gh-mjungmath

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.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 6 months ago by egourgoulhon

Replying to gh-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.

comment:8 follow-up: Changed 6 months ago by gh-mjungmath

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 egourgoulhon

Replying to gh-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/.

comment:10 Changed 6 months ago by mkoeppe

  • 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 mkoeppe

Replying to egourgoulhon:

Replying to gh-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

comment:12 Changed 6 months ago by git

  • Commit changed from 331aa59f61c065ac9b6237e5d37ae59ba652ece6 to 0579188638bb4f858976e9e8682f5c854e71eb66

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

0579188Fixup

comment:13 Changed 6 months ago by mkoeppe

  • Work issues set to redo on top of #31653

comment:14 Changed 6 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

comment:15 Changed 6 months ago by git

  • Commit changed from 0579188638bb4f858976e9e8682f5c854e71eb66 to 0666fae89c698e792b534c81395bb42deb2b3151

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

comment:16 Changed 6 months ago by git

  • Commit changed from 0666fae89c698e792b534c81395bb42deb2b3151 to 72294f04f8a99651a74f8629cdf3ba1278ffc0f4

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

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

comment:17 Changed 6 months ago by mkoeppe

  • Authors changed from Matthias Koeppe, ... to Matthias Koeppe
  • 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 mkoeppe

  • Dependencies changed from #31653 to #31653, #31763

comment:19 Changed 5 months ago by mkoeppe

  • 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 mkoeppe

  • Description modified (diff)

comment:21 Changed 5 months ago by mkoeppe

  • Dependencies changed from #31653, #31763 to #31653, #31763, #31798

comment:22 Changed 5 months ago by git

  • Commit changed from 72294f04f8a99651a74f8629cdf3ba1278ffc0f4 to 0faec94bad521406c4f4f2da04ef851051504ec9

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

6772570ManifoldSubset.union: Handle arbitrary unions
edfea33ManifoldSubset.union: Add example and plots
fdd0abaManifoldSubset.declare_union: Handle arbitrary unions
c2f484csrc/sage/manifolds/subset.py: import itertools
a083569ManifoldSubset._intersection_subset, _union_subset: Use declare_subset, declare_superset; fix doctest
da91653ManifoldSubset._union_subset: Fixup
3e6a9cfMerge #31764
3786ceeManifoldSubset.difference, complement: New
decd831Merge #31798
0faec94ManifoldSubset.is_closed, declare_closed: New

comment:23 Changed 5 months ago by mkoeppe

  • Status changed from new to needs_review

comment:24 Changed 5 months ago by mkoeppe

  • 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 mkoeppe

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 egourgoulhon

  • 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 gh-mjungmath

Looks like the failures are originated in #31798.

comment:28 Changed 5 months ago by mkoeppe

  • Dependencies changed from #31653, #31763, #31798 to #31763, #31798

comment:29 Changed 5 months ago by git

  • Commit changed from 0faec94bad521406c4f4f2da04ef851051504ec9 to 38c9e96e08560fbf7fa28397bba2e46b16e87e0e

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

94edd68ManifoldSubset.declare_superset: Fix documentation
41826b4ManifoldSubset.declare_{sub,super}set: Expand docstring
1c14655Merge #31763
6370e88ManifoldSubset.difference, complement: New
73eceacMerge #31798
2028258sage.manifolds.subsets.closure, TopologicalSubmanifold.closure: New
38c9e96ManifoldSubset.is_closed, declare_closed: New

comment:30 Changed 5 months ago by mkoeppe

Redid the branch on top of only #31763, #31798

comment:31 Changed 5 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:32 Changed 5 months ago by git

  • Commit changed from 38c9e96e08560fbf7fa28397bba2e46b16e87e0e to 9259a2c109ce9ac069e80f2c5aa090fb533e1690

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

3ef5141Merge #31763
79a625bManifoldSubset._reduce_intersection_members: Add examples, raise TypeError if input is empty family
8e70023ManifoldSubset._reduce_union_members: Add examples
ee4efc9ManifoldSubset._{union,intersection}_subset: Do cache the result here; add examples
e7f7a7dMerge #31764
9259a2cMerge #31798

comment:33 Changed 5 months ago by git

  • Commit changed from 9259a2c109ce9ac069e80f2c5aa090fb533e1690 to ac5398499c314b8a59b5994c2cfa8b5945f3b311

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

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

comment:34 Changed 5 months ago by git

  • Commit changed from ac5398499c314b8a59b5994c2cfa8b5945f3b311 to 60505b09c0a05e08282020ef1284db5ecff8ce6f

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

comment:35 Changed 5 months ago by gh-mjungmath

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

comment:36 Changed 5 months ago by mkoeppe

No, it's unrelated, see #31848

comment:37 Changed 5 months ago by egourgoulhon

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 git

  • Commit changed from 60505b09c0a05e08282020ef1284db5ecff8ce6f to b9909c061046a204354341833e3b3fcb4c6d7341

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

b9909c0ManifoldSubsetClosure: Add examples, fix docstring markup

comment:39 Changed 5 months ago by egourgoulhon

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

comment:40 Changed 5 months ago by egourgoulhon

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 git

  • Commit changed from b9909c061046a204354341833e3b3fcb4c6d7341 to 16c6a7219848d2045fe92b1ca50eb0195a05d91f

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

16c6a72ManifoldSubsetClosure, ManifoldSubset.closure: Improve documentation

comment:42 Changed 5 months ago by mkoeppe

Thanks for the suggestions, done.

comment:43 Changed 5 months ago by egourgoulhon

  • Reviewers set to Eric Gourgoulhon
  • Status changed from needs_review to positive_review

Thanks!

comment:44 Changed 5 months ago by mkoeppe

Thank you!

comment:45 Changed 4 months ago by git

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

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

comment:46 Changed 4 months ago by mkoeppe

  • Status changed from needs_review to positive_review

Trivial merge of updated dependency #31763

comment:47 Changed 4 months ago by vbraun

  • Branch changed from u/mkoeppe/topological_closure_of_embedded_submanifolds to 9abc6174394c85ca400650c814a3d27e7921729d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.