Opened 3 years ago
Closed 3 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: | egourgoulhon, tscrim | 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 3 years ago by
- Commit changed from 176596b1386c5f1e6d69645b996d174bc27abeae to 75adec7167ca57e38d657dbc262a0e7a089b7d80
comment:2 Changed 3 years ago by
- Cc tscrim added
comment:3 Changed 3 years ago by
- Commit changed from 75adec7167ca57e38d657dbc262a0e7a089b7d80 to 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 3 years ago by
- Commit changed from e32ba455933d559eb7abf8ddb8121332b5cdb2ee to d88c0d2eb9808d635d70e21fb27458e20661341c
Branch pushed to git repo; I updated commit sha1. New commits:
d88c0d2 | corrected bug when start_index!=0
|
comment:5 Changed 3 years ago by
- Commit changed from d88c0d2eb9808d635d70e21fb27458e20661341c to 697f904f395a26db04d6dd3449829a12845204a2
Branch pushed to git repo; I updated commit sha1. New commits:
697f904 | Previous commit introduced a new bug, now fixed
|
comment:6 Changed 3 years ago by
- Commit changed from 697f904f395a26db04d6dd3449829a12845204a2 to 5627f2c585f6a04916e17b144e4410dca562b1fc
Branch pushed to git repo; I updated commit sha1. New commits:
5627f2c | fixed bug concerning restrictions wrongly propagated
|
comment:7 Changed 3 years ago by
- Commit changed from 5627f2c585f6a04916e17b144e4410dca562b1fc to 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 3 years ago by
- Milestone changed from sage-8.2 to sage-8.3
- Status changed from new to needs_review
comment:9 Changed 3 years ago by
- Commit changed from 33d9dd68de9d0dce1fe7c24c8b27e48a583a12a2 to 99f584d40749acf692c2e4eae0a38a66636ab62f
comment:10 Changed 3 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 360-361 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 3 years ago by
- Reviewers set to Eric Gourgoulhon
comment:12 Changed 3 years ago by
- Status changed from needs_review to needs_work
comment:13 Changed 3 years ago by
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 3 years ago by
- Commit changed from 99f584d40749acf692c2e4eae0a38a66636ab62f to e81ef396c124db47213ae30c0a531e45f5396fef
comment:15 Changed 3 years ago by
- Commit changed from e81ef396c124db47213ae30c0a531e45f5396fef to 13bc650c5082bd6018970c8ca94c25e19bb23498
Branch pushed to git repo; I updated commit sha1. New commits:
13bc650 | PEP8
|
comment:16 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:17 Changed 3 years ago by
- 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 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 3 years ago by
- Commit changed from 13bc650c5082bd6018970c8ca94c25e19bb23498 to bca9a1a07bac2d02fb78864071fa3f8d8d23d76c
Branch pushed to git repo; I updated commit sha1. New commits:
bca9a1a | New accessors, improved documentation, typos
|
comment:19 Changed 3 years ago by
- 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: ↓ 21 Changed 3 years ago by
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.
comment:21 in reply to: ↑ 20 Changed 3 years ago by
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 3 years ago by
- Commit changed from bca9a1a07bac2d02fb78864071fa3f8d8d23d76c to eb2b4c84d579e8bc1c60bd899ea96d1ea9d44032
comment:23 Changed 3 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 3 years ago by
- Branch changed from public/manifolds/submanifolds to eb2b4c84d579e8bc1c60bd899ea96d1ea9d44032
- Resolution set to fixed
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Fixed errors preventing documentation from building