Opened 6 months ago
Closed 6 months ago
#30519 closed defect (fixed)
Fix check of orientation in volume form
Reported by:  egourgoulhon  Owned by:  

Priority:  critical  Milestone:  sage9.2 
Component:  manifolds  Keywords:  orientation, volume_form 
Cc:  ghmjungmath, tscrim  Merged in:  
Authors:  Eric Gourgoulhon  Reviewers:  Michael Jung 
Report Upstream:  N/A  Work issues:  
Branch:  e7c9d04 (Commits, GitHub, GitLab)  Commit:  e7c9d04759a03c6088fab9e43343f6349d521182 
Dependencies:  Stopgaps: 
Description
After the introduction of manifold orientation in #30178, attempting to get the volume form of a metric on a manifold without any predefined orientation yields a weird error message:
sage: M = Manifold(2, 'M', structure='Riemannian') # the 2sphere sage: U = M.open_subset('U'); V = M.open_subset('V') sage: M.declare_union(U, V) sage: c_xy.<x,y> = U.chart() # stereographic coord. from the North pole sage: c_uv.<u,v> = V.chart() # stereographic coord. from the South pole sage: xy_to_uv = c_xy.transition_map(c_uv, (x/(x^2+y^2), y/(x^2+y^2)), ....: intersection_name='W', restrictions1= x^2+y^2!=0, ....: restrictions2= u^2+v^2!=0) sage: uv_to_xy = xy_to_uv.inverse() sage: eU = c_xy.frame(); eV = c_uv.frame() sage: g = M.metric() sage: g[eU,0,0], g[eU,1,1] = 4/(1+x^2+y^2)^2, 4/(1+x^2+y^2)^2 sage: g.add_comp_by_continuation(eV, U.intersection(V), chart=c_uv) sage: g.volume_form() Traceback (most recent call last): ... IndexError: list index out of range
This surprising error message (which list index?) is due to a bad check for an orientation in PseudoRiemannianMetric.volume_form()
; the fix is
 if orient is None: + if not orient:
Indeed, the attribute _orientation
of manifolds is initialized as []
, not as None
. With the fix, the error message is more expressive:
ValueError: 2dimensional Riemannian manifold M must admit an orientation
In addition to the above fix, with a new doctest checking it, this ticket corrects some mistake in the documentation illustrating the choice of an orientation on the 2sphere in the docstring of class PseudoRiemannianMetric
:
sage: M.set_orientation([c_xy, c_uv])
does not define a valid orientation because the stereographic frames from the North and South poles have opposite orientations, the Jacobian of the transition map c_xy
> c_uv
being negative. It is replaced here by
sage: f = U.vector_frame('f', (eU[2], eU[1])) sage: M.set_orientation([eV, f])
Change History (13)
comment:1 Changed 6 months ago by
 Branch set to public/manifolds/fix_check_orientation30519
 Commit set to 6229d65246a7d058f85caed87d7912f536930ae5
comment:2 Changed 6 months ago by
 Cc ghmjungmath tscrim added
 Status changed from new to needs_review
comment:3 followups: ↓ 6 ↓ 7 Changed 6 months ago by
Thank you so much for catching this bug!
Of course, the orientation changes when using stereographic projections! I just haven't thought about it in the first place, thank you for catching this, too.
This is indeed a nice improvement of the documentation as well.

Just one minor thing. I made the same mistake in diff_form.py
:

src/sage/manifolds/differentiable/diff_form.py
diff git a/src/sage/manifolds/differentiable/diff_form.py b/src/sage/manifolds/differentiable/diff_form.py index 417183a..f0855ad 100644
a b class DiffForm(TensorField): 646 646 sage: uv_to_xy = xy_to_uv.inverse() 647 647 sage: W = U.intersection(V) # The complement of the two poles 648 648 sage: eU = c_xy.frame() ; eV = c_uv.frame() 649 sage: M.set_orientation([eU, eV]) # make M orientable 649 650 sage: g = M.metric('g') 650 651 sage: g[eU,1,1], g[eU,2,2] = 4/(1+x^2+y^2)^2, 4/(1+x^2+y^2)^2 651 652 sage: g[eV,1,1], g[eV,2,2] = 4/(1+u^2+v^2)^2, 4/(1+u^2+v^2)^2
comment:4 in reply to: ↑ description Changed 6 months ago by
Replying to egourgoulhon:
In addition to the above fix, with a new doctest checking it, this ticket corrects some mistake in the documentation illustrating the choice of an orientation on the 2sphere in the docstring of class
PseudoRiemannianMetric
:sage: M.set_orientation([c_xy, c_uv])does not define a valid orientation because the stereographic frames from the North and South poles have opposite orientations, the Jacobian of the transition map
c_xy
>c_uv
being negative. It is replaced here bysage: f = U.vector_frame('f', (eU[2], eU[1])) sage: M.set_orientation([eV, f])
I recently noticed this while teaching my class as well and was going to tell you. Thank you for catching this and fixing it.
comment:5 Changed 6 months ago by
 Commit changed from 6229d65246a7d058f85caed87d7912f536930ae5 to e7c9d04759a03c6088fab9e43343f6349d521182
Branch pushed to git repo; I updated commit sha1. New commits:
e7c9d04  #30519: Correct orientation in a diff form example

comment:6 in reply to: ↑ 3 Changed 6 months ago by
Replying to ghmjungmath:
Just one minor thing. I made the same mistake in
diff_form.py
:
Thanks for pointing this out; it's corrected in the latest commit.
comment:7 in reply to: ↑ 3 ; followup: ↓ 12 Changed 6 months ago by
Replying to ghmjungmath:
Thank you so much for catching this bug!
You're welcome. Thanks to you for having introduced orientations on manifolds. They were really missing!
Of course, the orientation changes when using stereographic projections! I just haven't thought about it in the first place,
No problem. The error was already there, when the volume form took the same shape, with the same sign, in the two stereographic frames. Same thing in the S^2 example notebook, which I am going to correct tomorrow.
comment:8 Changed 6 months ago by
 Status changed from needs_review to positive_review
Perfect. Thank you.
LGTM.
comment:9 Changed 6 months ago by
 Reviewers set to Michael Jung
comment:10 Changed 6 months ago by
 Priority changed from major to critical
comment:11 Changed 6 months ago by
Thank you for the review!
comment:12 in reply to: ↑ 7 Changed 6 months ago by
Replying to egourgoulhon:
Same thing in the S^2 example notebook, which I am going to correct tomorrow.
Done; see cells [221] and below.
comment:13 Changed 6 months ago by
 Branch changed from public/manifolds/fix_check_orientation30519 to e7c9d04759a03c6088fab9e43343f6349d521182
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Fix check of orientation in volume form (Trac #30519)