#30320 closed defect (fixed)
set_default_frame() too restrictive on parallelizable manifolds
Reported by:  ghmjungmath  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  manifolds  Keywords:  
Cc:  egourgoulhon, tscrim  Merged in:  
Authors:  Michael Jung  Reviewers:  Eric Gourgoulhon 
Report Upstream:  N/A  Work issues:  
Branch:  7f50b3d (Commits)  Commit:  7f50b3d5ff7786fc6a7874a8b28314a1fdfeca02 
Dependencies:  Stopgaps: 
Description
sage: M = Manifold(3, 'M') sage: U = M.open_subset('U'); V = M.open_subset('V') sage: c_xyz.<x,y,z> = U.chart(); c_uvt.<u,v,t> = V.chart() sage: M.default_frame() Coordinate frame (U, (d/dx,d/dy,d/dz)) sage: e = M.vector_frame('e'); e Vector frame (M, (e_0,e_1,e_2)) sage: M.default_frame() Coordinate frame (U, (d/dx,d/dy,d/dz)) sage: M.set_default_frame(c_xyz.frame()) Traceback (most recent call last): ... ValueError: the frame domain must coincide with the 3dimensional differentiable manifold M
In case of M=SU(2)=S^3
this can happen: it cannot be covered by one chart but is parallelizable.
However, due to the error message, the method default_frame
is obviously intended to return the default frame which covers M
if possible. This is not given in the above example.
Change History (15)
comment:1 Changed 3 months ago by
comment:2 Changed 3 months ago by
I think, both options are reasonable. The first is, of course, more comprehensible to user.
I don't see a third choice. If you have any ideas, I'd welcome them.
comment:3 followup: ↓ 4 Changed 3 months ago by
Thanks for handling this. In all the sage/manifolds settings, the "default frame" is initially set as the first frame defined on the manifold, usually (but not necessarily) the coordinate frame corresponding to the first chart defined on the manifold. For consistency among all kinds of manifolds, I would be inclined to keep this behaviour even for parallelizable manifolds not covered by a a single chart, such as S^{3}. So I would vote for option 1 of comment:1. Moreover, although it is also quite natural, option 2 would perform a silent change of the default frame, which may surprise the end user.
In DifferentiableManifold.set_default_frame()
, there is
if frame._domain is not self: if self.is_manifestly_parallelizable(): raise ValueError("the frame domain must coincide with " + "the {}".format(self))
which triggers the error reported in the ticket description. Maybe we can simply remove this...
comment:4 in reply to: ↑ 3 Changed 3 months ago by
Replying to egourgoulhon:
Thanks for handling this. In all the sage/manifolds settings, the "default frame" is initially set as the first frame defined on the manifold, usually (but not necessarily) the coordinate frame corresponding to the first chart defined on the manifold. For consistency among all kinds of manifolds, I would be inclined to keep this behaviour even for parallelizable manifolds not covered by a a single chart, such as S^{3}. So I would vote for option 1 of comment:1. Moreover, although it is also quite natural, option 2 would perform a silent change of the default frame, which may surprise the end user.
In
DifferentiableManifold.set_default_frame()
, there isif frame._domain is not self: if self.is_manifestly_parallelizable(): raise ValueError("the frame domain must coincide with " + "the {}".format(self))which triggers the error reported in the ticket description. Maybe we can simply remove this...
This is exactly what I meant by "The first [option] is [...] more comprehensible to user.". Thanks for pointing this out in such detail.
Vector bundles currently use option 1 as well, btw.
Then, I will take care of removing the corresponding error messages and adapt the documentation if necessary. This is good, because #30178 can then be kept.
comment:5 Changed 3 months ago by
 Branch set to u/ghmjungmath/parallelizable_manifold_not_covered_by_one_chart_and_default_frame
comment:6 Changed 3 months ago by
 Commit set to 7f50b3d5ff7786fc6a7874a8b28314a1fdfeca02
 Status changed from new to needs_review
New commits:
7f50b3d  Trac #30320: error messages removed

comment:7 Changed 3 months ago by
Similarly for charts, btw, though more unlikely. I've removed that error message, too. Agreed?
comment:8 Changed 7 weeks ago by
 Milestone changed from sage9.2 to sage9.3
comment:9 followup: ↓ 10 Changed 7 weeks ago by
Is that good or some more work needed here?
comment:10 in reply to: ↑ 9 ; followup: ↓ 11 Changed 7 weeks ago by
 Reviewers set to Eric Gourgoulhon
 Status changed from needs_review to positive_review
 Summary changed from Parallelizable Manifold not covered by one Chart and default_frame to set_default_frame() too restrictive on parallelizable manifolds
Replying to ghmjungmath:
Is that good or some more work needed here?
Yes it's good. Sorry for the delay.
PS: I've added the ticket to #30235, where it was missing.
comment:11 in reply to: ↑ 10 ; followup: ↓ 12 Changed 7 weeks ago by
Replying to egourgoulhon:
PS: I've added the ticket to #30235, where it was missing.
Btw, don't hesitate to add relevant tickets there.
comment:12 in reply to: ↑ 11 ; followup: ↓ 13 Changed 7 weeks ago by
Replying to egourgoulhon:
Replying to egourgoulhon:
PS: I've added the ticket to #30235, where it was missing.
Btw, don't hesitate to add relevant tickets there.
Okay, sure. :)
Besides, you meant the metaticket #30525, right?
comment:13 in reply to: ↑ 12 Changed 7 weeks ago by
comment:14 Changed 6 weeks ago by
 Branch changed from u/ghmjungmath/parallelizable_manifold_not_covered_by_one_chart_and_default_frame to 7f50b3d5ff7786fc6a7874a8b28314a1fdfeca02
 Resolution set to fixed
 Status changed from positive_review to closed
comment:15 Changed 6 weeks ago by
 Milestone changed from sage9.3 to sage9.2
I see two options here:
set_default_frame
, even in the parallelizable case.M
in case it haven't coveredM
before.