Opened 5 years ago
Closed 5 years ago
#25164 closed enhancement (fixed)
Embedded submanifolds
Reported by:  ghFlorentinJ  Owned by:  

Priority:  major  Milestone:  sage8.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: 
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
Commit:  176596b1386c5f1e6d69645b996d174bc27abeae → 75adec7167ca57e38d657dbc262a0e7a089b7d80 

comment:2 Changed 5 years ago by
Cc:  Travis Scrimshaw added 

comment:3 Changed 5 years ago by
Commit:  75adec7167ca57e38d657dbc262a0e7a089b7d80 → e32ba455933d559eb7abf8ddb8121332b5cdb2ee 

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

comment:4 Changed 5 years ago by
Commit:  e32ba455933d559eb7abf8ddb8121332b5cdb2ee → d88c0d2eb9808d635d70e21fb27458e20661341c 

Branch pushed to git repo; I updated commit sha1. New commits:
d88c0d2  corrected bug when start_index!=0

comment:5 Changed 5 years ago by
Commit:  d88c0d2eb9808d635d70e21fb27458e20661341c → 697f904f395a26db04d6dd3449829a12845204a2 

Branch pushed to git repo; I updated commit sha1. New commits:
697f904  Previous commit introduced a new bug, now fixed

comment:6 Changed 5 years ago by
Commit:  697f904f395a26db04d6dd3449829a12845204a2 → 5627f2c585f6a04916e17b144e4410dca562b1fc 

Branch pushed to git repo; I updated commit sha1. New commits:
5627f2c  fixed bug concerning restrictions wrongly propagated

comment:7 Changed 5 years ago by
Commit:  5627f2c585f6a04916e17b144e4410dca562b1fc → 33d9dd68de9d0dce1fe7c24c8b27e48a583a12a2 

Branch pushed to git repo; I updated commit sha1. New commits:
33d9dd6  Latex names are now supported in adapted charts, substitutions are stored.

comment:8 Changed 5 years ago by
Milestone:  sage8.2 → sage8.3 

Status:  new → needs_review 
comment:9 Changed 5 years ago by
Commit:  33d9dd68de9d0dce1fe7c24c8b27e48a583a12a2 → 99f584d40749acf692c2e4eae0a38a66636ab62f 

comment:10 Changed 5 years ago by
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) putdifferentiable_submanifold.py
andpseudo_riemannian_submanifold.py
into a subdirectorysrc/sage/manifolds/differentiable/submanifold
and (ii) puttopological_submanifold.py
insrc/sage/manifolds
(no need for a subdirectory here since it is only one file). In this way, everything related with differentiable manifolds is kept undersrc/sage/manifolds/differentiable
 One doctest fails in
src/sage/manifold/differentiable/real_line.py
due to the change of the keyword parameterambient
tobase_manifold
 For the class
OpenInterval
defined insrc/sage/manifold/differentiable/real_line.py
, the new keyword argumentbase_manifold
of__init__
sounds a little akward in this context; maybe it could be replaced bybase_interval
orambient_interval
; alsoambient
in the docstring ofOpenInterval
must be updated.  Maybe you could introduce a method
TopologicalSubmanifold.set_embedding
that would perform bothset_immersion
anddeclare_embedding
.  In the docstring of
set_immersion
, the description ofphi_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
orinverse_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 360361 oftopological_submanifold.py
assage: 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
Reviewers:  → Eric Gourgoulhon 

comment:12 Changed 5 years ago by
Status:  needs_review → needs_work 

comment:13 Changed 5 years ago by
I don't think it is worthwhile to have a directory with 12 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
Commit:  99f584d40749acf692c2e4eae0a38a66636ab62f → e81ef396c124db47213ae30c0a531e45f5396fef 

comment:15 Changed 5 years ago by
Commit:  e81ef396c124db47213ae30c0a531e45f5396fef → 13bc650c5082bd6018970c8ca94c25e19bb23498 

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

comment:16 Changed 5 years ago by
Status:  needs_work → needs_review 

comment:17 Changed 5 years ago by
Reviewers:  Eric Gourgoulhon → Eric Gourgoulhon, Travis Scrimshaw 

Status:  needs_review → 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 methodambient()
was introduced as an alias ofmanifold()
. For submanifolds, which inherit fromManifoldSubset
, 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 aliasambient = manifold
insrc/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 argumentambient
asambient=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 filesubmanifold.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
andset_immersion
, it is said that the default value of the parametert_inverse
is{}
, while it is actuallyNone
 A typo in line 128 of
topological_submanifold.py
: "or computed" > "are computed" (I guess)
comment:18 Changed 5 years ago by
Commit:  13bc650c5082bd6018970c8ca94c25e19bb23498 → bca9a1a07bac2d02fb78864071fa3f8d8d23d76c 

Branch pushed to git repo; I updated commit sha1. New commits:
bca9a1a  New accessors, improved documentation, typos

comment:19 Changed 5 years ago by
Status:  needs_work → 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 followup: 21 Changed 5 years ago by
I don't see the harm in having such an alias. It is good for consistency/ducktyping and a similar pattern has been used for free modules. However, the nomenclature clash might require renaming one of the methods.
comment:21 Changed 5 years ago by
Replying to tscrim:
I don't see the harm in having such an alias. It is good for consistency/ducktyping 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
Commit:  bca9a1a07bac2d02fb78864071fa3f8d8d23d76c → eb2b4c84d579e8bc1c60bd899ea96d1ea9d44032 

comment:23 Changed 5 years ago by
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:25 Changed 5 years ago by
Branch:  public/manifolds/submanifolds → eb2b4c84d579e8bc1c60bd899ea96d1ea9d44032 

Resolution:  → fixed 
Status:  positive_review → closed 
Branch pushed to git repo; I updated commit sha1. New commits:
Fixed errors preventing documentation from building