Opened 8 months ago

Closed 7 months 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: egourgoulhon, tscrim Merged in:
Authors: Florentin Jaffredo Reviewers: Eric Gourgoulhon, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: eb2b4c8 (Commits) Commit: eb2b4c84d579e8bc1c60bd899ea96d1ea9d44032
Dependencies: Stopgaps:

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 8 months ago by git

  • Commit changed from 176596b1386c5f1e6d69645b996d174bc27abeae to 75adec7167ca57e38d657dbc262a0e7a089b7d80

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

75adec7Fixed errors preventing documentation from building

comment:2 Changed 8 months ago by tscrim

  • Cc tscrim added

comment:3 Changed 8 months ago by git

  • Commit changed from 75adec7167ca57e38d657dbc262a0e7a089b7d80 to e32ba455933d559eb7abf8ddb8121332b5cdb2ee

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 8 months ago by git

  • Commit changed from e32ba455933d559eb7abf8ddb8121332b5cdb2ee to d88c0d2eb9808d635d70e21fb27458e20661341c

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

d88c0d2corrected bug when start_index!=0

comment:5 Changed 8 months ago by git

  • Commit changed from d88c0d2eb9808d635d70e21fb27458e20661341c to 697f904f395a26db04d6dd3449829a12845204a2

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

697f904Previous commit introduced a new bug, now fixed

comment:6 Changed 8 months ago by git

  • Commit changed from 697f904f395a26db04d6dd3449829a12845204a2 to 5627f2c585f6a04916e17b144e4410dca562b1fc

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

5627f2cfixed bug concerning restrictions wrongly propagated

comment:7 Changed 7 months ago by git

  • Commit changed from 5627f2c585f6a04916e17b144e4410dca562b1fc to 33d9dd68de9d0dce1fe7c24c8b27e48a583a12a2

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 7 months ago by gh-FlorentinJ

  • Milestone changed from sage-8.2 to sage-8.3
  • Status changed from new to needs_review

comment:9 Changed 7 months ago by git

  • Commit changed from 33d9dd68de9d0dce1fe7c24c8b27e48a583a12a2 to 99f584d40749acf692c2e4eae0a38a66636ab62f

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 7 months ago by egourgoulhon

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 7 months ago by egourgoulhon

  • Reviewers set to Eric Gourgoulhon

comment:12 Changed 7 months ago by egourgoulhon

  • Status changed from needs_review to needs_work

comment:13 Changed 7 months ago by tscrim

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 7 months ago by git

  • Commit changed from 99f584d40749acf692c2e4eae0a38a66636ab62f to e81ef396c124db47213ae30c0a531e45f5396fef

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 7 months ago by git

  • Commit changed from e81ef396c124db47213ae30c0a531e45f5396fef to 13bc650c5082bd6018970c8ca94c25e19bb23498

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

13bc650PEP8

comment:16 Changed 7 months ago by gh-FlorentinJ

  • Status changed from needs_work to needs_review

comment:17 Changed 7 months ago by egourgoulhon

  • Reviewers changed from Eric Gourgoulhon to Eric Gourgoulhon, Travis Scrimshaw
  • Status changed from needs_review to needs_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 7 months ago by git

  • Commit changed from 13bc650c5082bd6018970c8ca94c25e19bb23498 to bca9a1a07bac2d02fb78864071fa3f8d8d23d76c

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

bca9a1aNew accessors, improved documentation, typos

comment:19 Changed 7 months ago by gh-FlorentinJ

  • Status changed from needs_work to needs_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 follow-up: Changed 7 months ago by 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.

Last edited 7 months ago by tscrim (previous) (diff)

comment:21 in reply to: ↑ 20 Changed 7 months ago by egourgoulhon

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 7 months ago by git

  • Commit changed from bca9a1a07bac2d02fb78864071fa3f8d8d23d76c to eb2b4c84d579e8bc1c60bd899ea96d1ea9d44032

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 7 months ago by egourgoulhon

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 7 months ago by tscrim

  • Status changed from needs_review to positive_review

Nope.

comment:25 Changed 7 months ago by vbraun

  • Branch changed from public/manifolds/submanifolds to eb2b4c84d579e8bc1c60bd899ea96d1ea9d44032
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.