Opened 6 months ago
Closed 8 weeks ago
#31781 closed defect (fixed)
Improve handling of metrics on pseudoRiemannian submanifolds
Reported by:  egourgoulhon  Owned by:  

Priority:  major  Milestone:  sage9.5 
Component:  manifolds  Keywords:  
Cc:  ghmjungmath  Merged in:  
Authors:  Eric Gourgoulhon  Reviewers:  Michael Jung 
Report Upstream:  N/A  Work issues:  
Branch:  dc15aff (Commits, GitHub, GitLab)  Commit:  dc15aff72221572912f47c0ac12f6325ee4da3a7 
Dependencies:  Stopgaps: 
Description
In Sage 9.3.rc5, we have
sage: S2 = manifolds.Sphere(2) sage: g = S2.metric() sage: g Riemannian metric g on the 2sphere 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
comment:2 Changed 6 months ago by
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 followup: ↓ 5 Changed 6 months ago by
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 followup: ↓ 6 Changed 6 months ago by
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
Replying to ghmjungmath:
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
wheremetric()
is not delegating toinduced_metric()
.
Indeed!
comment:6 in reply to: ↑ 4 ; followup: ↓ 7 Changed 6 months ago by
Replying to ghmjungmath:
As for my first comment, perhaps it is a good idea to add an optional argument such as
induced_metric=True
orriemannian=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
Replying to egourgoulhon:
Replying to ghmjungmath:
As for my first comment, perhaps it is a good idea to add an optional argument such as
induced_metric=True
orriemannian=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 choosesriemannian=False
, she/he ends up with a pure differentiable manifold, without any predefined metric. Similarly, we could havedifferentiable=True
, so that choosingdifferentiable=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
 Milestone changed from sage9.3 to sage9.4
Moving to 9.4, as 9.3 has been released.
comment:9 followup: ↓ 11 Changed 2 months ago by
(see comment:11 below)
comment:10 followup: ↓ 12 Changed 2 months ago by
Should we instead attack the issue with PseudoRiemannianSubmanifold
and it's metric()
call?
comment:11 in reply to: ↑ 9 ; followup: ↓ 13 Changed 2 months ago by
Replying to ghmjungmath:
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 enduser.
This issue is by the way not restricted to spheres. Also PseudoRiemannian 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 PseudoRiemannian manifolds.
At this point it might also worth to mention that differentiable manifolds with imposed metrics are not seen as PseudoRiemannian manifolds. We don't even have a category for PseudoRiemannian manifolds.
comment:12 in reply to: ↑ 10 Changed 2 months ago by
Replying to ghmjungmath:
Should we instead attack the issue with
PseudoRiemannianSubmanifold
and it'smetric()
call?
Yes I think so (see comment:13 below).
comment:13 in reply to: ↑ 11 ; followup: ↓ 14 Changed 2 months ago by
Replying to ghmjungmath:
This issue is by the way not restricted to spheres. Also PseudoRiemannian 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 PseudoRiemannian manifolds.
In the current implementation, on any PseudoRiemannianManifold
,
metric()
returns the default metricmetric('h')
initializes a new metric with nameh
To be consistent, on a PseudoRiemannianSubmanifold
, we should have the following behavior:
metric()
redirects toinduced_metric()
, since this is clearly the default metric on a pseudoRiemannian submanifoldmetric('h')
initializes a new metric with nameh
To acheive this, we should redefine metric()
in the subclass PseudoRiemannianSubmanifold
.
comment:14 in reply to: ↑ 13 ; followup: ↓ 15 Changed 2 months ago by
Replying to egourgoulhon:
Replying to ghmjungmath:
This issue is by the way not restricted to spheres. Also PseudoRiemannian 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 PseudoRiemannian manifolds.
In the current implementation, on any
PseudoRiemannianManifold
,
metric()
returns the default metricmetric('h')
initializes a new metric with nameh
To be consistent, on a
PseudoRiemannianSubmanifold
, we should have the following behavior:
metric()
redirects toinduced_metric()
, since this is clearly the default metric on a pseudoRiemannian submanifoldmetric('h')
initializes a new metric with nameh
To acheive this, we should redefine
metric()
in the subclassPseudoRiemannianSubmanifold
.
My thought exactly!
comment:15 in reply to: ↑ 14 ; followup: ↓ 16 Changed 2 months ago by
comment:16 in reply to: ↑ 15 ; followup: ↓ 17 Changed 2 months ago by
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
Replying to ghmjungmath:
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
 Branch set to public/manifolds/submanifold_metric31781
 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 pseudoRiemannian 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:
dc15aff  Implement PseudoRiemannianSubmanifold.metric() and various related improvements (Trac #31781)

comment:19 Changed 2 months ago by
 Milestone changed from sage9.4 to sage9.5
comment:20 followup: ↓ 21 Changed 2 months ago by
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 pseudoRiemannian 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
Replying to ghmjungmath:
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 pseudoRiemannian 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:23 Changed 2 months ago by
Thank you for the explanation.
comment:25 Changed 8 weeks ago by
 Branch changed from public/manifolds/submanifold_metric31781 to dc15aff72221572912f47c0ac12f6325ee4da3a7
 Resolution set to fixed
 Status changed from positive_review to closed
Actually, it appears that
g
is not initialized: