Opened 3 months ago

Last modified 6 weeks ago

#32228 needs_work enhancement

Move examples from sage.geometry.riemannian_manifolds to sage.manifolds

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.5
Component: manifolds Keywords:
Cc: egourgoulhon, gh-mjungmath, 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:

Status badges

Description

... with deprecation

Change History (13)

comment:1 Changed 3 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Dependencies set to #32116

comment:2 Changed 3 months ago by mkoeppe

  • Dependencies changed from #32116 to #32116, #32102

comment:3 Changed 3 months ago by mkoeppe

  • Branch set to u/mkoeppe/move_examples_from_sage_geometry_riemannian_manifolds_to_sage_manifolds

comment:4 Changed 3 months ago by git

  • Commit set to fcf62a69569fa2664d99d87088256da2f154b795

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

fcf62a6ParametrizedSurface3D: Initialize immersion, replace attribute 'equation' by lazy attribute

comment:5 Changed 3 months ago by mkoeppe

The next step would be to deprecate most of the methods

comment:6 Changed 3 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:7 Changed 6 weeks ago by git

  • Commit changed from fcf62a69569fa2664d99d87088256da2f154b795 to 1b4fe44c1eaa8eccca72fc29a6fd143953236a92

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

1b4fe44Merge tag '9.5.beta1' into t/32228/move_examples_from_sage_geometry_riemannian_manifolds_to_sage_manifolds

comment:8 Changed 6 weeks ago by mkoeppe

  • Status changed from new to needs_review

comment:9 Changed 6 weeks ago by gh-mjungmath

Do we want to fix the syntax-block errors and unused import in this ticket?

Coverage is likely more for another ticket.

comment:10 Changed 6 weeks ago by egourgoulhon

  • 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 manifold-type 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 follow-up: Changed 6 weeks ago by vdelecroix

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 equivalent PseudoRiemannianSubmanifold way of doing
  • for each relevant doctest in ParametrizedSurface3D, the same computation should be reproduced with a PseudoRiemannianSubmanifold 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 6 weeks ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:13 in reply to: ↑ 11 Changed 6 weeks ago by egourgoulhon

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.

Note: See TracTickets for help on using tickets.