Embedded submanifolds

[gh-FlorentinJ]

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.

[git]

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

​75adec7

Fixed errors preventing documentation from building

[git]

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

​862bcaf	Created a clean new branch
​c5864bd	Changed 'ambient' to 'base_manifold'
​176596b	Added documentation
​75adec7	Fixed errors preventing documentation from building
​d7c59e5	Merge branch 'develop' of https://github.com/sagemath/sage into embedding
​e32ba45	Changed modules name, fixed errors in documentation

[git]

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

​d88c0d2

corrected bug when start_index!=0

[git]

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

​697f904

Previous commit introduced a new bug, now fixed

[git]

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

​5627f2c

fixed bug concerning restrictions wrongly propagated

[git]

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

​33d9dd6

Latex names are now supported in adapted charts, substitutions are stored.

[git]

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

​a05e9ec	Merge branch 'develop' into embedding
​d03474c	Typo
​99f584d	Incompatibility with 8.2 Euclidean spaces solved

[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`

[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.

[git]

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

​e77ebe0	Solved failed doctest due to old keyword 'ambient'
​3747790	Some documentation improvements
​e81ef39	Moved files as suggested

[git]

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

​13bc650

PEP8

[Eric Gourgoulhon]

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)

[git]

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

​bca9a1a

New accessors, improved documentation, typos

[gh-FlorentinJ]

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.

[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.

[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.

[git]

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

​c9a7082	Merge branch 'public/manifolds/submanifolds' of git://trac.sagemath.org/sage into Sage 8.3.beta2
​eb2b4c8	Reviewer tweaks for submanifolds

[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?

[Travis Scrimshaw]

Nope.