#25869 closed enhancement (fixed)
Add manifold catalog
Reported by: | gh-FlorentinJ | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.5 |
Component: | geometry | Keywords: | generator, shortcut, manifold |
Cc: | egourgoulhon, tscrim | Merged in: | |
Authors: | Florentin Jaffredo | Reviewers: | Eric Gourgoulhon, Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | ba166f8 (Commits, GitHub, GitLab) | Commit: | ba166f87999a9afb44025713782affaa2d18f8c6 |
Dependencies: | Stopgaps: |
Description
This ticket implements a few shortcuts to define some manifolds.
With this ticket, n-sphere, torus, Minkowski and Kerr spacetime can be declared in just one line, which is convenient for casual users and future doctests or examples.
Change History (25)
comment:1 Changed 3 years ago by
comment:2 Changed 3 years ago by
- Commit changed from a6f428660e8a9a5ded6888bb80b4e488dc3c36d6 to 911bde9c48467f37d145095dbd462ed47bb309c9
Branch pushed to git repo; I updated commit sha1. New commits:
911bde9 | Refactored code, better signature handling
|
comment:3 Changed 3 years ago by
I changed it according to your remarks. I originally used the file surface3d_generators.py
as an example, but i didn't realize it was an exception.
I'm currently experiencing difficulties building the documentation (for no reason at all, it worked well last week), so i don't know if it build properly.
comment:4 Changed 3 years ago by
Many thanks for introducing a manifold catalog, at last!
A short comment: to avoid any confusion and code duplication, I would remove Euclidean
from the catalog, since Euclidean spaces can now be easily constructed with EuclideanSpace
, as implemented in #24623 (merged in Sage 8.3.beta0). Or even better, manifolds.Euclidean()
could return an object of class EuclideanSpace
(possibly of the subclasses EuclideanPlane
or Euclidean3dimSpace
).
comment:5 Changed 3 years ago by
- Commit changed from 911bde9c48467f37d145095dbd462ed47bb309c9 to 7a5bde2cd4ee60f91625545535c8ac21a0962cd7
Branch pushed to git repo; I updated commit sha1. New commits:
7a5bde2 | Now uses EuclideanSpace, added doc entry
|
comment:6 Changed 3 years ago by
- Cc tscrim added
comment:7 Changed 3 years ago by
- Commit changed from 7a5bde2cd4ee60f91625545535c8ac21a0962cd7 to ecca83272e56a4a2091c02769321f713d9a7c0f6
Branch pushed to git repo; I updated commit sha1. New commits:
ecca832 | minor fix
|
comment:8 Changed 3 years ago by
- Commit changed from ecca83272e56a4a2091c02769321f713d9a7c0f6 to 4ff840bfd6f8cb8a9380456a6acbc11ecab172f1
comment:9 Changed 3 years ago by
- Commit changed from 4ff840bfd6f8cb8a9380456a6acbc11ecab172f1 to a3c6996765b274f6353b81adb34a6cbbd5f2cc7c
comment:10 Changed 3 years ago by
- Status changed from new to needs_review
comment:11 Changed 2 years ago by
A few comments:
- In
src/sage/manifolds/catalog.py
, there is a reference to the classManifoldGenerator
, while no such class does exist. - The example provided for
manifolds.Euclidean
is too short (a single line!). A slightly more illustrative example (3 lines) would be:sage: E.<x, y, z> = manifolds.Euclidean() sage: E Euclidean space E^3
You could add as wellsage: E.atlas() [Chart (E^3, (x, y, z))] sage: E.metric().display() g = dx*dx + dy*dy + dz*dz
- Similarly the example of
manifolds.Sphere
could be extended tosage: S.<th, ph> = manifolds.Sphere() sage: S 2-dimensional pseudo-Riemannian submanifold S embedded in 3-dimensional differentiable manifold E^3 sage: S.atlas() [Chart (S, (th, ph))] sage: S.metric().display() gamma = dth*dth + sin(th)^2 dph*dph
By the way, shouldn't the (default) metric name should beg
here (as in the other manifolds of the catalog)? - The example for
manifolds.Kerr
should be extended as well, to something like:sage: m, a = var('m, a') sage: K = manifolds.Kerr(m, a) sage: K 4-dimensional Lorentzian manifold M sage: K.atlas() [Chart (M, (t, r, th, ph))] sage: K.metric().display() g = (2*m*r/(a^2*cos(th)^2 + r^2) - 1) dt*dt + 2*a*m*r*sin(th)^2/(a^2*cos(th)^2 + r^2) dt*dph + (a^2*cos(th)^2 + r^2)/(a^2 - 2*m*r + r^2) dr*dr + (a^2*cos(th)^2 + r^2) dth*dth + 2*a*m*r*sin(th)^2/(a^2*cos(th)^2 + r^2) dph*dt + (2*a^2*m*r*sin(th)^2/(a^2*cos(th)^2 + r^2) + a^2 + r^2)*sin(th)^2 dph*dph
- Using
sage: m, a = var('m, a') sage: K.<t, r, th, ph> = manifolds.Kerr(m, a)
leads to wrong coordinate ranges:sage: K.default_chart().coord_range() t: (-oo, +oo); r: (-oo, +oo); th: (-oo, +oo); ph: (-oo, +oo)
- The example for
manifolds.Torus
should be extended to something like:sage: T.<theta, phi> = manifolds.Torus(3,1) sage: T 2-dimensional pseudo-Riemannian submanifold M embedded in 3-dimensional differentiable manifold E^3 sage: T.atlas() [Chart (M, (theta, phi))] sage: T.embedding().display() M --> E^3 (theta, phi) |--> (x_0, x_1, x_2) = ((cos(theta) + 3)*cos(phi), (cos(theta) + 3)*sin(phi), sin(theta)) sage: T.metric().display() gamma = dtheta*dtheta + (cos(theta)^2 + 6*cos(theta) + 9) dphi*dphi
- In the code for
manifolds.Torus
, replacexnames = tuple(["x_{}".format(i) for i in range(3)]) E = Euclidean(names=xnames)
byE = EuclideanSpace(3, symbols='X Y Z')
- Similarly, in the code for
manifolds.Sphere
, replacexnames = ("X","Y","Z") E = Euclidean(names=xnames)
byE = EuclideanSpace(3, symbols='X Y Z')
- In the code of
manifolds.Minkowski
, the structure should beLorentzian
instead ofpseudo-Riemannian
:M = Manifold(4, 'M', structure='Lorentzian')
comment:12 Changed 2 years ago by
- Status changed from needs_review to needs_work
comment:13 Changed 2 years ago by
- Commit changed from a3c6996765b274f6353b81adb34a6cbbd5f2cc7c to a8aa3ac0a3ed4da3ee58b2fa9bfd69cbd6f2d0d3
comment:14 follow-up: ↓ 17 Changed 2 years ago by
- Reviewers set to Eric Gourgoulhon, Travis Scrimshaw
- Status changed from needs_work to needs_review
I have fixed all of Eric's comments in comment:11, as well as fixed a bug in that Sphere
was only implemented with the assumption that it was 2 dimensional (this now raises a NotImplementedError
instead of a more cryptic error). Eric, if my changes are good with you, then we can set this to a positive review (unfortunately, it seems we just missed the 8.4 window :/).
comment:15 Changed 2 years ago by
- Commit changed from a8aa3ac0a3ed4da3ee58b2fa9bfd69cbd6f2d0d3 to 18479687f5fa6be0c9699b386c3c5cc0f0c4da3b
Branch pushed to git repo; I updated commit sha1. New commits:
1847968 | Small tweaks in manifolds catalog
|
comment:16 Changed 2 years ago by
- Summary changed from Simple manifold generator to Add manifold catalog
comment:17 in reply to: ↑ 14 Changed 2 years ago by
Replying to tscrim:
I have fixed all of Eric's comments in comment:11, as well as fixed a bug in that
Sphere
was only implemented with the assumption that it was 2 dimensional (this now raises aNotImplementedError
instead of a more cryptic error). Eric, if my changes are good with you, then we can set this to a positive review (unfortunately, it seems we just missed the 8.4 window :/).
Thanks Travis for the changes! I'm 100% fine with them. In the commit of comment:15, I've made a few very small tweaks + the change of "ADM" to "Kerr" for the second coordinate system of Kerr spacetime, since "Kerr" is the common name in the literature for such coordinates. From my point of view, the ticket is ready to go.
comment:18 follow-up: ↓ 20 Changed 2 years ago by
Reading your comment:1, I am realizing that I've probably made a mistake by removing
from sage.manifolds.manifold import Manifold
from each function to put it at the top of the file...
Similar thing for EuclideanSpace
...
(well in the current case, both Manifold
and EuclideanSpace
should already be imported in the global namespace, but for clarity, we should make all the imports in catalog.py
be local to the functions, shouldn't we?)
comment:19 Changed 2 years ago by
- Commit changed from 18479687f5fa6be0c9699b386c3c5cc0f0c4da3b to f0acd1f3765c78486aba1817416cfd76c70ab341
Branch pushed to git repo; I updated commit sha1. New commits:
f0acd1f | Correct import in manifolds catalog
|
comment:20 in reply to: ↑ 18 Changed 2 years ago by
Replying to egourgoulhon:
Reading your comment:1, I am realizing that I've probably made a mistake by removing
from sage.manifolds.manifold import Manifoldfrom each function to put it at the top of the file... Similar thing for
EuclideanSpace
...
This is corrected in the last commit. So at this stage, I would say that everything is fine for me.
comment:21 Changed 2 years ago by
- Commit changed from f0acd1f3765c78486aba1817416cfd76c70ab341 to ba166f87999a9afb44025713782affaa2d18f8c6
Branch pushed to git repo; I updated commit sha1. New commits:
ba166f8 | Correct stereographic coord. changes in the Sphere() entry of manifold catalog
|
comment:22 Changed 2 years ago by
Giving a last look at the code, I've rewritten slightly the change of stereographic coordinates part in the sphere, thereby removing two pyflakes errors revealed by the patchbot. This time, this should finally be OK for me.
comment:23 Changed 2 years ago by
- Status changed from needs_review to positive_review
LGTM. Thank you.
comment:24 Changed 2 years ago by
- Branch changed from public/manifolds/generator to ba166f87999a9afb44025713782affaa2d18f8c6
- Resolution set to fixed
- Status changed from positive_review to closed
comment:25 Changed 2 years ago by
- Milestone changed from sage-8.4 to sage-8.5
This should be re-targeted for 8.5.
A more Pythonic way would be to have these be functions in a module and import the module (this is how most the other catalogs are done in Sage). So the new file you've created should be renames as something like
catalog.py
and imported inmanifolds/all.py
byWithin the file, you should move the import
within each function. I might also consider masquerading the functions as classes by making them start with uppercase letters or breaking with Python conventions by capitalizing the proper names, but Sage doesn't really have a true convention for these things.
Also, typo:
``sng``
, but I think the sign should also take an generic tuple of length 4.