Opened 13 months ago
Last modified 4 months ago
#32228 needs_work enhancement
Move examples from sage.geometry.riemannian_manifolds to sage.manifolds
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage9.7 
Component:  manifolds  Keywords:  
Cc:  egourgoulhon, ghmjungmath, vdelecroix  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  u/mkoeppe/move_examples_from_sage_geometry_riemannian_manifolds_to_sage_manifolds (Commits, GitHub, GitLab)  Commit:  1b4fe44c1eaa8eccca72fc29a6fd143953236a92 
Dependencies:  #32116, #32102  Stopgaps: 
Description
... with deprecation
Change History (15)
comment:1 Changed 13 months ago by
 Dependencies set to #32116
comment:2 Changed 13 months ago by
 Dependencies changed from #32116 to #32116, #32102
comment:3 Changed 13 months ago by
 Branch set to u/mkoeppe/move_examples_from_sage_geometry_riemannian_manifolds_to_sage_manifolds
comment:4 Changed 13 months ago by
 Commit set to fcf62a69569fa2664d99d87088256da2f154b795
comment:5 Changed 13 months ago by
The next step would be to deprecate most of the methods
comment:6 Changed 13 months ago by
 Milestone changed from sage9.4 to sage9.5
comment:7 Changed 11 months ago by
 Commit changed from fcf62a69569fa2664d99d87088256da2f154b795 to 1b4fe44c1eaa8eccca72fc29a6fd143953236a92
Branch pushed to git repo; I updated commit sha1. New commits:
1b4fe44  Merge tag '9.5.beta1' into t/32228/move_examples_from_sage_geometry_riemannian_manifolds_to_sage_manifolds

comment:8 Changed 11 months ago by
 Status changed from new to needs_review
comment:9 Changed 11 months ago by
Do we want to fix the syntaxblock errors and unused import in this ticket?
Coverage is likely more for another ticket.
comment:10 Changed 11 months ago by
 Cc vdelecroix added
Thanks for this first move.
However it is quite incomplete at this stage. Indeed, it makes ParametrizedSurface3D
a derived class of PseudoRiemannianSubmanifold
, which is very natural. But many of the methods, like first_fundamental_form
, second_fundamental_form
, point
, plot
, tangent_vector
, etc. have their code left in place, so that they appear redefined with respect to the corresponding PseudoRiemannianSubmanifold
methods with very different
inputs and outputs. For instance, ParametrizedSurface3D.tangent_vector
takes a tuple of coordinates and returns a tuple of vector components, while PseudoRiemannianSubmanifold.tangent_vector
takes a manifold point and returns an element of the tangent space at that point. It would be quite disturbing for the end user that manifolds.Ellipsoid()
behaves so differently from manifolds.Sphere(2)
. At first glance, all the methods implemented in ParametrizedSurface3D
have their equivalent in PseudoRiemannianSubmanifold
, but it requires some work to implement things properly. I guess the question is then: shall this be done in this ticket?
Also what about the deprecation? If we change the behavior of most methods to make them coincide with the manifold framework, we should leave the old code in src/sage/geometry/riemannian_manifolds
(with a deprecation warning), shouldn't we? In doing so, during the deprecation period, we would have:
sage: C1 = manifolds.Catenoid() # returns a manifoldtype object sage: C2 = surfaces.Catenoid() # returns a (deprecated) old type of object
I am adding Vincent to Cc, since he took part in the initial implementation of ParametrizedSurface3D
in #10132.
comment:11 followup: ↓ 13 Changed 11 months ago by
If all methods from ParametrizedSurface3D
have equivalent functionalities in PseudoRiemannianSubmanifold
I would say that deprecation of the class makes sense. However, this could potentially breaks a lot of code in the future. For that reason, I would suggest to carefully document the "new way of doing". That is to say
 in each method of
ParametrizedSurface3D
there should be an explanation of the equivalentPseudoRiemannianSubmanifold
way of doing  for each relevant doctest in
ParametrizedSurface3D
, the same computation should be reproduced with aPseudoRiemannianSubmanifold
object.
This is just my way of thinking. I will not work on the ticket and you are definitely free to take any decision you would find more appropriate.
comment:12 Changed 11 months ago by
 Status changed from needs_review to needs_work
comment:13 in reply to: ↑ 11 Changed 11 months ago by
Replying to vdelecroix:
This is just my way of thinking. I will not work on the ticket and you are definitely free to take any decision you would find more appropriate.
Thanks for your feedback! On my side, this project is on my todo list, but with a zillion of other things. I hope to do it relatively soon, unless someone do it before.
comment:14 Changed 8 months ago by
 Milestone changed from sage9.5 to sage9.6
comment:15 Changed 4 months ago by
 Milestone changed from sage9.6 to sage9.7
Branch pushed to git repo; I updated commit sha1. New commits:
ParametrizedSurface3D: Initialize immersion, replace attribute 'equation' by lazy attribute