Opened 5 years ago

Closed 5 years ago

#25164 closed enhancement (fixed)

Embedded submanifolds

Reported by: gh-FlorentinJ Owned by:
Priority: major Milestone: sage-8.3
Component: geometry Keywords: submanifold, embedding, immersion, manifold
Cc: Eric Gourgoulhon, Travis Scrimshaw Merged in:
Authors: Florentin Jaffredo Reviewers: Eric Gourgoulhon, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: eb2b4c8 (Commits, GitHub, GitLab) Commit: eb2b4c84d579e8bc1c60bd899ea96d1ea9d44032
Dependencies: Stopgaps:

Status badges

Description

This ticket implements submanifolds immersed or embedded in topological or differentiable manifolds.

It doesn't yet implement calculations on these submanifolds, but can find coordinates adapted to a foliation, which is the first step in 3+1 formalism in general relativity.

This is part of the ​SageManifolds project; see the metaticket #18528 for an overview.

Change History (25)

comment:1 Changed 5 years ago by git

Commit: 176596b1386c5f1e6d69645b996d174bc27abeae75adec7167ca57e38d657dbc262a0e7a089b7d80

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

75adec7Fixed errors preventing documentation from building

comment:2 Changed 5 years ago by Travis Scrimshaw

Cc: Travis Scrimshaw added

comment:3 Changed 5 years ago by git

Commit: 75adec7167ca57e38d657dbc262a0e7a089b7d80e32ba455933d559eb7abf8ddb8121332b5cdb2ee

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

862bcafCreated a clean new branch
c5864bdChanged 'ambient' to 'base_manifold'
176596bAdded documentation
75adec7Fixed errors preventing documentation from building
d7c59e5Merge branch 'develop' of https://github.com/sagemath/sage into embedding
e32ba45Changed modules name, fixed errors in documentation

comment:4 Changed 5 years ago by git

Commit: e32ba455933d559eb7abf8ddb8121332b5cdb2eed88c0d2eb9808d635d70e21fb27458e20661341c

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

d88c0d2corrected bug when start_index!=0

comment:5 Changed 5 years ago by git

Commit: d88c0d2eb9808d635d70e21fb27458e20661341c697f904f395a26db04d6dd3449829a12845204a2

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

697f904Previous commit introduced a new bug, now fixed

comment:6 Changed 5 years ago by git

Commit: 697f904f395a26db04d6dd3449829a12845204a25627f2c585f6a04916e17b144e4410dca562b1fc

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

5627f2cfixed bug concerning restrictions wrongly propagated

comment:7 Changed 5 years ago by git

Commit: 5627f2c585f6a04916e17b144e4410dca562b1fc33d9dd68de9d0dce1fe7c24c8b27e48a583a12a2

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

33d9dd6Latex names are now supported in adapted charts, substitutions are stored.

comment:8 Changed 5 years ago by gh-FlorentinJ

Milestone: sage-8.2sage-8.3
Status: newneeds_review

comment:9 Changed 5 years ago by git

Commit: 33d9dd68de9d0dce1fe7c24c8b27e48a583a12a299f584d40749acf692c2e4eae0a38a66636ab62f

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

a05e9ecMerge branch 'develop' into embedding
d03474cTypo
99f584dIncompatibility with 8.2 Euclidean spaces solved

comment:10 Changed 5 years ago by Eric Gourgoulhon

Thanks for this piece of work!

Here are some (rather minor) comments:

  • About the directory structure: instead of having everything in the subdirectory src/sage/manifolds/submanifold, one could (i) put differentiable_submanifold.py and pseudo_riemannian_submanifold.py into a subdirectory src/sage/manifolds/differentiable/submanifold and (ii) put topological_submanifold.py in src/sage/manifolds (no need for a subdirectory here since it is only one file). In this way, everything related with differentiable manifolds is kept under src/sage/manifolds/differentiable
  • One doctest fails in src/sage/manifold/differentiable/real_line.py due to the change of the keyword parameter ambient to base_manifold
  • For the class OpenInterval defined in src/sage/manifold/differentiable/real_line.py, the new keyword argument base_manifold of __init__ sounds a little akward in this context; maybe it could be replaced by base_interval or ambient_interval; also ambient in the docstring of OpenInterval must be updated.
  • Maybe you could introduce a method TopologicalSubmanifold.set_embedding that would perform both set_immersion and declare_embedding.
  • In the docstring of set_immersion, the description of phi_inverse should be extended, by precising that this is the inverse of phi onto its image; besides, phi_inverse is a not a good keyword, inverse or inverse_from_image is probably better
  • Some PEP8 recommendations: don't use spaces around the = sign when used to indicate a keyword argument and do not use \ to break a line inside parentheses; for instance, rewrite lines 360-361 of topological_submanifold.py as
                sage: N.set_immersion(phi, phi_inverse=phi_inv, var=t,
                ....:                 t_inverse={t:phi_inv_t})
    
  • Put back quotes around LaTeX expressions in docstrings, so that Sphinx can render them properly: for instance, in the docstrings of topological_submanifold.py, replace all \phi by `\phi`.
  • In the documentation of TopologicalSubmanifold.plot: `ParametricSurface` should be replaced by ``ParametricSurface`` and :func: `ParametricSurface` by :class:`~sage.plot.plot3d.parametric_surface.ParametricSurface`

comment:11 Changed 5 years ago by Eric Gourgoulhon

Reviewers: Eric Gourgoulhon

comment:12 Changed 5 years ago by Eric Gourgoulhon

Status: needs_reviewneeds_work

comment:13 Changed 5 years ago by Travis Scrimshaw

I don't think it is worthwhile to have a directory with 1-2 files unless you plan for it to grow. At least to me, I don't see it growing much. So for (i), I would just put them both in manifolds/differentiable.

comment:14 Changed 5 years ago by git

Commit: 99f584d40749acf692c2e4eae0a38a66636ab62fe81ef396c124db47213ae30c0a531e45f5396fef

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

e77ebe0Solved failed doctest due to old keyword 'ambient'
3747790Some documentation improvements
e81ef39Moved files as suggested

comment:15 Changed 5 years ago by git

Commit: e81ef396c124db47213ae30c0a531e45f5396fef13bc650c5082bd6018970c8ca94c25e19bb23498

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

13bc650PEP8

comment:16 Changed 5 years ago by gh-FlorentinJ

Status: needs_workneeds_review

comment:17 Changed 5 years ago by Eric Gourgoulhon

Reviewers: Eric GourgoulhonEric Gourgoulhon, Travis Scrimshaw
Status: needs_reviewneeds_work

Thanks for these changes. In particular, I agree with Travis (comment:13) that there is no need for a specific subdirectory.

A few more comments, first regarding the code:

  • In class sage.manifolds.subset.ManifoldSubset a method ambient() was introduced as an alias of manifold(). For submanifolds, which inherit from ManifoldSubset, this clashes with the meaning given to ambient. To avoid any confusion and since it does not appear to be massively used, I would remove the alias ambient = manifold in src/sage/manifolds/subset.py.
  • One should probably introduce accessor methods in class TopologicalSubmanifold:
    • ambient()
    • immersion()
    • embedding()
  • In lines 2464 and 2490 of src/sage/manifolds/manifold.py, one should pass the argument ambient as ambient=ambient (it is a good policy to always specify the keyword in passing keyword arguments, in case the order of arguments is changed)

Regarding the documentation:

  • In src/doc/en/reference/manifolds/index.rst, I don't think it is worth to have a whole section devoted to submanifolds in the main menu. It seems more relevant to add the various submanifolds as subsections of the sections they belong to: topological submanifolds in topological manifolds, etc. In doing so, there is no longer any need of the file submanifold.rst
  • In src/sage/manifolds/differentiable/differentiable_submanifold.py, a better title would be Submanifolds of differentiable manifolds. Also here you should specify what is meant by submanifold: in the current phrasing, it defaults to embedded submanifold (I am OK with this), but you should allow for immersed submanifolds
  • In the docstring of the methods set_embedding and set_immersion, it is said that the default value of the parameter t_inverse is {}, while it is actually None
  • A typo in line 128 of topological_submanifold.py: "or computed" --> "are computed" (I guess)

comment:18 Changed 5 years ago by git

Commit: 13bc650c5082bd6018970c8ca94c25e19bb23498bca9a1a07bac2d02fb78864071fa3f8d8d23d76c

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

bca9a1aNew accessors, improved documentation, typos

comment:19 Changed 5 years ago by gh-FlorentinJ

Status: needs_workneeds_review

I'm not sure removing the method ambient is a good idea given that it is an abstract method of a parent class of ManifoldSubset, and so should be implemented in every child class.

Not doing so raises errors during calls of TestSuite.

comment:20 Changed 5 years ago by Travis Scrimshaw

I don't see the harm in having such an alias. It is good for consistency/duck-typing and a similar pattern has been used for free modules. However, the nomenclature clash might require renaming one of the methods.

Last edited 5 years ago by Travis Scrimshaw (previous) (diff)

comment:21 in reply to:  20 Changed 5 years ago by Eric Gourgoulhon

Replying to tscrim:

I don't see the harm in having such an alias. It is good for consistency/duck-typing and a similar pattern has been used for free modules. However, the nomenclature clash might require renaming one of the methods.

OK let us keep the alias ambient in ManifoldSubset; in some sense, it is coherent to redefine it for submanifolds (as it is now), so there is no need for renaming.

comment:22 Changed 5 years ago by git

Commit: bca9a1a07bac2d02fb78864071fa3f8d8d23d76ceb2b4c84d579e8bc1c60bd899ea96d1ea9d44032

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

c9a7082Merge branch 'public/manifolds/submanifolds' of git://trac.sagemath.org/sage into Sage 8.3.beta2
eb2b4c8Reviewer tweaks for submanifolds

comment:23 Changed 5 years ago by Eric Gourgoulhon

I've just pushed some reviewer tweaks, which regard mostly the documentation. On my side, it's OK for a positive review. Travis, do you have any further comment?

comment:24 Changed 5 years ago by Travis Scrimshaw

Status: needs_reviewpositive_review

Nope.

comment:25 Changed 5 years ago by Volker Braun

Branch: public/manifolds/submanifoldseb2b4c84d579e8bc1c60bd899ea96d1ea9d44032
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.