Opened 6 months ago

Closed 8 weeks ago

#31781 closed defect (fixed)

Improve handling of metrics on pseudo-Riemannian submanifolds

Reported by: egourgoulhon Owned by:
Priority: major Milestone: sage-9.5
Component: manifolds Keywords:
Cc: gh-mjungmath Merged in:
Authors: Eric Gourgoulhon Reviewers: Michael Jung
Report Upstream: N/A Work issues:
Branch: dc15aff (Commits, GitHub, GitLab) Commit: dc15aff72221572912f47c0ac12f6325ee4da3a7
Dependencies: Stopgaps:

Status badges

Description

In Sage 9.3.rc5, we have

sage: S2 = manifolds.Sphere(2)                                      
sage: g = S2.metric()
sage: g                                    
Riemannian metric g on the 2-sphere S^2 of radius 1 smoothly embedded 
 in the Euclidean space E^3

So far so good, but then

sage: g.display()
...
ValueError: no basis could be found for computing the components
 in the Coordinate frame (A, (d/dtheta,d/dphi))
sage: g.ricci_scalar()
...
ValueError: no basis could be found for computing the components
 in the Coordinate frame (A, (d/dtheta,d/dphi))

Change History (25)

comment:1 Changed 6 months ago by egourgoulhon

Actually, it appears that g is not initialized:

sage: S2 = manifolds.Sphere(2)                                             
sage: g = S2.metric()                                                        
sage: g._restrictions                                           
{}
Last edited 6 months ago by egourgoulhon (previous) (diff)

comment:2 Changed 6 months ago by gh-mjungmath

Since spheres are implemented as embedded manifolds, they are only endowed with the induced metric from the ambient space.

Thus I'd say this is no bug. In particular it could happen that the user wants to endow spheres with metrics different from the induced one.

I'd say we just mention this in the documentation.

comment:3 follow-up: Changed 6 months ago by gh-mjungmath

Wait. We implemented spheres as Riemannian submanifolds which means that S2.metric() should certainly give the induced metric.

But then this is more a bug of PseudoRiemannianSubmanifold where metric() is not delegating to induced_metric().

comment:4 follow-up: Changed 6 months ago by gh-mjungmath

As for my first comment, perhaps it is a good idea to add an optional argument such as induced_metric=True or riemannian=True for spheres to choose whether the submanifold should inherit the metric or not.

comment:5 in reply to: ↑ 3 Changed 6 months ago by egourgoulhon

Replying to gh-mjungmath:

Wait. We implemented spheres as Riemannian submanifolds which means that S2.metric() should certainly give the induced metric.

Yes!

But then this is more a bug of PseudoRiemannianSubmanifold where metric() is not delegating to induced_metric().

Indeed!

comment:6 in reply to: ↑ 4 ; follow-up: Changed 6 months ago by egourgoulhon

Replying to gh-mjungmath:

As for my first comment, perhaps it is a good idea to add an optional argument such as induced_metric=True or riemannian=True for spheres to choose whether the submanifold should inherit the metric or not.

Yes, I would vote for riemannian=True, such that if the user chooses riemannian=False, she/he ends up with a pure differentiable manifold, without any predefined metric. Similarly, we could have differentiable=True, so that choosing differentiable=False returns a mere topological manifold.

comment:7 in reply to: ↑ 6 Changed 6 months ago by gh-mjungmath

Replying to egourgoulhon:

Replying to gh-mjungmath:

As for my first comment, perhaps it is a good idea to add an optional argument such as induced_metric=True or riemannian=True for spheres to choose whether the submanifold should inherit the metric or not.

Yes, I would vote for riemannian=True, such that if the user chooses riemannian=False, she/he ends up with a pure differentiable manifold, without any predefined metric. Similarly, we could have differentiable=True, so that choosing differentiable=False returns a mere topological manifold.

On a second thought, #31241 might be a better solution for that.

comment:8 Changed 6 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Moving to 9.4, as 9.3 has been released.

comment:9 follow-up: Changed 2 months ago by gh-mjungmath

(see comment:11 below)

Last edited 2 months ago by gh-mjungmath (previous) (diff)

comment:10 follow-up: Changed 2 months ago by gh-mjungmath

Should we instead attack the issue with PseudoRiemannianSubmanifold and it's metric() call?

Last edited 2 months ago by gh-mjungmath (previous) (diff)

comment:11 in reply to: ↑ 9 ; follow-up: Changed 2 months ago by gh-mjungmath

Replying to gh-mjungmath:

I think we should implement this flag. Ticket #31241 just gives a more natural way to implement this. But I am afraid this ticket is not nearly ready. So I would be fine having a tentative implementation to make the above syntax more convenient for the end-user.

This issue is by the way not restricted to spheres. Also Pseudo-Riemannian manifolds can have different metrics. The current implementation even accommodates this. From that perspective, despite what I said earlier, I actually don't see the need for such a flag because this is in full line with the current way of using Pseudo-Riemannian manifolds.

At this point it might also worth to mention that differentiable manifolds with imposed metrics are not seen as Pseudo-Riemannian manifolds. We don't even have a category for Pseudo-Riemannian manifolds.

Last edited 2 months ago by gh-mjungmath (previous) (diff)

comment:12 in reply to: ↑ 10 Changed 2 months ago by egourgoulhon

Replying to gh-mjungmath:

Should we instead attack the issue with PseudoRiemannianSubmanifold and it's metric() call?

Yes I think so (see comment:13 below).

comment:13 in reply to: ↑ 11 ; follow-up: Changed 2 months ago by egourgoulhon

Replying to gh-mjungmath:

This issue is by the way not restricted to spheres. Also Pseudo-Riemannian manifolds can have different metrics. The current implementation even accommodates this. From that perspective, despite what I said earlier, I actually don't see the need for such a flag because this is in full line with the current way of using Pseudo-Riemannian manifolds.

In the current implementation, on any PseudoRiemannianManifold,

  • metric() returns the default metric
  • metric('h') initializes a new metric with name h

To be consistent, on a PseudoRiemannianSubmanifold, we should have the following behavior:

  • metric() redirects to induced_metric(), since this is clearly the default metric on a pseudo-Riemannian submanifold
  • metric('h') initializes a new metric with name h

To acheive this, we should redefine metric() in the subclass PseudoRiemannianSubmanifold.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 2 months ago by gh-mjungmath

Replying to egourgoulhon:

Replying to gh-mjungmath:

This issue is by the way not restricted to spheres. Also Pseudo-Riemannian manifolds can have different metrics. The current implementation even accommodates this. From that perspective, despite what I said earlier, I actually don't see the need for such a flag because this is in full line with the current way of using Pseudo-Riemannian manifolds.

In the current implementation, on any PseudoRiemannianManifold,

  • metric() returns the default metric
  • metric('h') initializes a new metric with name h

To be consistent, on a PseudoRiemannianSubmanifold, we should have the following behavior:

  • metric() redirects to induced_metric(), since this is clearly the default metric on a pseudo-Riemannian submanifold
  • metric('h') initializes a new metric with name h

To acheive this, we should redefine metric() in the subclass PseudoRiemannianSubmanifold.

My thought exactly!

comment:15 in reply to: ↑ 14 ; follow-up: Changed 2 months ago by egourgoulhon

Replying to gh-mjungmath:

My thought exactly!

Very good. Do you plan to implement this?

comment:16 in reply to: ↑ 15 ; follow-up: Changed 2 months ago by gh-mjungmath

Replying to egourgoulhon:

My thought exactly!

Very good. Do you plan to implement this?

I'd prefer to first work on tickets that are needed for my current project. Time is slipping through. As soon as those are done, I could come back to this one.

comment:17 in reply to: ↑ 16 Changed 2 months ago by egourgoulhon

Replying to gh-mjungmath:

I'd prefer to first work on tickets that are needed for my current project. Time is slipping through. As soon as those are done, I could come back to this one.

No problem, I'll implement it soon.

comment:18 Changed 2 months ago by egourgoulhon

  • Authors set to Eric Gourgoulhon
  • Branch set to public/manifolds/submanifold_metric-31781
  • Commit set to dc15aff72221572912f47c0ac12f6325ee4da3a7
  • Status changed from new to needs_review
  • Summary changed from Unusable metric on spheres from the manifold catalog to Improve handling of metrics on pseudo-Riemannian submanifolds

According to the discussion in comment:13 and comment:14, the above branch reimplements the method metric in the subclass PseudoRiemannianSubmanifold of PseudoRiemannianManifold. In preparing it, I've realized that the handling of the metric names was somewhat messy and I've reorganized it. I've also made the default name of the metric of spheres in the manifold catalog to be g instead of gamma, which is more natural, I think.


New commits:

dc15affImplement PseudoRiemannianSubmanifold.metric() and various related improvements (Trac #31781)

comment:19 Changed 2 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:20 follow-up: Changed 2 months ago by gh-mjungmath

Is there a specific reason for this change?

@@ -98,12 +98,12 @@ class DegenerateManifold(DifferentiableManifold):
     - [DB1996]_
     - [DS2010]_
     """
-    def __init__(self, n, name, metric_name='g', signature=None, base_manifold=None,
-                 diff_degree=infinity, latex_name=None,
+    def __init__(self, n, name, metric_name=None, signature=None,
+                 base_manifold=None, diff_degree=infinity, latex_name=None,
                  metric_latex_name=None, start_index=0, category=None,
                  unique_tag=None):
         r"""
-        Construct a pseudo-Riemannian manifold.
+        Construct a degenerate manifold.
 
         TESTS::
 
@@ -130,7 +130,9 @@ class DegenerateManifold(DifferentiableManifold):
                                         category=category)
         self._metric = None # to be initialized by metric()
         self._metric_signature = signature
-        if not isinstance(metric_name, str):
+        if metric_name is None:
+            metric_name = 'g'
+        elif not isinstance(metric_name, str):
             raise TypeError("{} is not a string".format(metric_name))
         self._metric_name = metric_name
         if metric_latex_name is None:

Other than that, LGTM. What is wrong with the patchbot?

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

Replying to gh-mjungmath:

Is there a specific reason for this change?

-    def __init__(self, n, name, metric_name='g', signature=None, base_manifold=None,
-                 diff_degree=infinity, latex_name=None,
+    def __init__(self, n, name, metric_name=None, signature=None,
+                 base_manifold=None, diff_degree=infinity, latex_name=None,

This is to normalize all the manifold __init__'s; this is mandory for constructing degenerate submanifolds, which inherit both from DegenerateManifold and DifferentiableSubmanifold.

         r"""
-        Construct a pseudo-Riemannian manifold.
+        Construct a degenerate manifold.

This corrects a (copy/paste) error in the docstring.

-        if not isinstance(metric_name, str):
+        if metric_name is None:
+            metric_name = 'g'
+        elif not isinstance(metric_name, str):
             raise TypeError("{} is not a string".format(metric_name))
         self._metric_name = metric_name
         if metric_latex_name is None:

This restores g as the default metric name, after the __init__ entry has been normalized with metric_name=None.

Other than that, LGTM. What is wrong with the patchbot?

I don't know. At least one of them gave a green light (the only plugin failure is non_ascii, but that plugin is no longer relevant and should be removed IMHO).

comment:22 Changed 2 months ago by gh-mjungmath

  • Status changed from needs_review to positive_review

Alright.

comment:23 Changed 2 months ago by gh-mjungmath

Thank you for the explanation.

comment:24 Changed 2 months ago by egourgoulhon

  • Reviewers set to Michael Jung

Thank you for the review!

comment:25 Changed 8 weeks ago by vbraun

  • Branch changed from public/manifolds/submanifold_metric-31781 to dc15aff72221572912f47c0ac12f6325ee4da3a7
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.