#30320 closed defect (fixed)
set_default_frame() too restrictive on parallelizable manifolds
Reported by: | gh-mjungmath | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.2 |
Component: | manifolds | Keywords: | |
Cc: | egourgoulhon, tscrim | Merged in: | |
Authors: | Michael Jung | Reviewers: | Eric Gourgoulhon |
Report Upstream: | N/A | Work issues: | |
Branch: | 7f50b3d (Commits, GitHub, GitLab) | 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 3-dimensional 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 9 months ago by
comment:2 Changed 9 months ago by
I think, both options are reasonable. The first is, of course, more comprehensible for to user.
I don't see a third choice. If you have any ideas, I'd welcome them.
comment:3 follow-up: ↓ 4 Changed 9 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 S3. 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 9 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 S3. 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 9 months ago by
- Branch set to u/gh-mjungmath/parallelizable_manifold_not_covered_by_one_chart_and_default_frame
comment:6 Changed 9 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 9 months ago by
Similarly for charts, btw, though more unlikely. I've removed that error message, too. Agreed?
comment:8 Changed 8 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:9 follow-up: ↓ 10 Changed 7 months ago by
Is that good or some more work needed here?
comment:10 in reply to: ↑ 9 ; follow-up: ↓ 11 Changed 7 months 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 gh-mjungmath:
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 ; follow-up: ↓ 12 Changed 7 months 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 ; follow-up: ↓ 13 Changed 7 months 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 months ago by
comment:14 Changed 7 months ago by
- Branch changed from u/gh-mjungmath/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 7 months ago by
- Milestone changed from sage-9.3 to sage-9.2
I see two options here:
set_default_frame
, even in the parallelizable case.M
in case it haven't coveredM
before.