Opened 23 months ago

Closed 20 months ago

Last modified 19 months ago

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

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 in manifolds/all.py by

import .catalog as manifolds

Within the file, you should move the import

from sage.manifolds.manifold import Manifold

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.

comment:2 Changed 23 months ago by git

  • Commit changed from a6f428660e8a9a5ded6888bb80b4e488dc3c36d6 to 911bde9c48467f37d145095dbd462ed47bb309c9

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

911bde9Refactored code, better signature handling

comment:3 Changed 23 months ago by gh-FlorentinJ

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

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

  • Commit changed from 911bde9c48467f37d145095dbd462ed47bb309c9 to 7a5bde2cd4ee60f91625545535c8ac21a0962cd7

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

7a5bde2Now uses EuclideanSpace, added doc entry

comment:6 Changed 23 months ago by tscrim

  • Cc tscrim added

comment:7 Changed 23 months ago by git

  • Commit changed from 7a5bde2cd4ee60f91625545535c8ac21a0962cd7 to ecca83272e56a4a2091c02769321f713d9a7c0f6

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

ecca832minor fix

comment:8 Changed 23 months ago by git

  • Commit changed from ecca83272e56a4a2091c02769321f713d9a7c0f6 to 4ff840bfd6f8cb8a9380456a6acbc11ecab172f1

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

028012danother small fix
4ff840bFix wrong Kerr metric

comment:9 Changed 22 months ago by git

  • Commit changed from 4ff840bfd6f8cb8a9380456a6acbc11ecab172f1 to a3c6996765b274f6353b81adb34a6cbbd5f2cc7c

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

de1d433Added stereographic chart for 2d spheres
5594f9dChanged the immersion of the sphere
622fff3Previous commit was missing some expressions
a3c6996Added a small check

comment:10 Changed 22 months ago by gh-FlorentinJ

  • Status changed from new to needs_review

comment:11 Changed 21 months ago by egourgoulhon

A few comments:

  • In src/sage/manifolds/catalog.py, there is a reference to the class ManifoldGenerator, 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 well
    sage: 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 to
    sage: 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 be g 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, replace
    xnames = tuple(["x_{}".format(i) for i in range(3)])
    E = Euclidean(names=xnames)
    
    by
    E = EuclideanSpace(3, symbols='X Y Z')
    
  • Similarly, in the code for manifolds.Sphere, replace
    xnames = ("X","Y","Z")
    E = Euclidean(names=xnames)
    
    by
    E = EuclideanSpace(3, symbols='X Y Z')
    
  • In the code of manifolds.Minkowski, the structure should be Lorentzian instead of pseudo-Riemannian:
    M = Manifold(4, 'M', structure='Lorentzian')
    

comment:12 Changed 21 months ago by egourgoulhon

  • Status changed from needs_review to needs_work

comment:13 Changed 20 months ago by git

  • Commit changed from a3c6996765b274f6353b81adb34a6cbbd5f2cc7c to a8aa3ac0a3ed4da3ee58b2fa9bfd69cbd6f2d0d3

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

64fbf09Merge branch 'public/manifolds/generator' of git://trac.sagemath.org/sage into public/manifolds/generator
a8aa3acReviewer changes and some bug fixes to manifolds catalog.

comment:14 follow-up: Changed 20 months ago by tscrim

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

  • Commit changed from a8aa3ac0a3ed4da3ee58b2fa9bfd69cbd6f2d0d3 to 18479687f5fa6be0c9699b386c3c5cc0f0c4da3b

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

1847968Small tweaks in manifolds catalog

comment:16 Changed 20 months ago by egourgoulhon

  • Summary changed from Simple manifold generator to Add manifold catalog

comment:17 in reply to: ↑ 14 Changed 20 months ago by egourgoulhon

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 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 :/).

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

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

  • Commit changed from 18479687f5fa6be0c9699b386c3c5cc0f0c4da3b to f0acd1f3765c78486aba1817416cfd76c70ab341

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

f0acd1fCorrect import in manifolds catalog

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

Replying to egourgoulhon:

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

This is corrected in the last commit. So at this stage, I would say that everything is fine for me.

comment:21 Changed 20 months ago by git

  • Commit changed from f0acd1f3765c78486aba1817416cfd76c70ab341 to ba166f87999a9afb44025713782affaa2d18f8c6

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

ba166f8Correct stereographic coord. changes in the Sphere() entry of manifold catalog

comment:22 Changed 20 months ago by egourgoulhon

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

  • Status changed from needs_review to positive_review

LGTM. Thank you.

comment:24 Changed 20 months ago by vbraun

  • Branch changed from public/manifolds/generator to ba166f87999a9afb44025713782affaa2d18f8c6
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:25 Changed 19 months ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

This should be re-targeted for 8.5.

Note: See TracTickets for help on using tickets.