Opened 3 months ago

Closed 6 weeks ago

Last modified 6 weeks ago

#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) 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 3 months ago by gh-mjungmath

I see two options here:

  1. One allows also frames defined on subsets for set_default_frame, even in the parallelizable case.
  2. One overwrites the default frame for the first frame which covers M in case it haven't covered M before.

comment:2 Changed 3 months ago by gh-mjungmath

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.

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

comment:3 follow-up: Changed 3 months ago by 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 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 gh-mjungmath

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 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...

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 gh-mjungmath

  • Branch set to u/gh-mjungmath/parallelizable_manifold_not_covered_by_one_chart_and_default_frame

comment:6 Changed 3 months ago by gh-mjungmath

  • Authors set to Michael Jung
  • Commit set to 7f50b3d5ff7786fc6a7874a8b28314a1fdfeca02
  • Status changed from new to needs_review

New commits:

7f50b3dTrac #30320: error messages removed

comment:7 Changed 3 months ago by gh-mjungmath

Similarly for charts, btw, though more unlikely. I've removed that error message, too. Agreed?

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

comment:8 Changed 7 weeks ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:9 follow-up: Changed 7 weeks ago by gh-mjungmath

Is that good or some more work needed here?

comment:10 in reply to: ↑ 9 ; follow-up: Changed 7 weeks ago by egourgoulhon

  • 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: Changed 7 weeks ago by 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.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 7 weeks ago by gh-mjungmath

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 egourgoulhon

Replying to gh-mjungmath:

Besides, you meant the metaticket #30525, right?

Yes.

comment:14 Changed 6 weeks ago by vbraun

  • 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 6 weeks ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.2
Note: See TracTickets for help on using tickets.