Opened 6 months ago

Closed 4 months ago

#31674 closed enhancement (fixed)

Make open subsets of immersed/embedded submanifolds immersed/embedded submanifolds

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.4
Component: manifolds Keywords:
Cc: gh-mjungmath, egourgoulhon, tscrim Merged in:
Authors: Matthias Koeppe Reviewers: Eric Gourgoulhon
Report Upstream: N/A Work issues:
Branch: 58b43f7 (Commits, GitHub, GitLab) Commit: 58b43f7902296384dade8a617441e205956d9824
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

This would be convenient for example for #31660.

To implement this, we refactor the method open_subset, already now duplicated between TopologicalManifold and DifferentiableManifold through a new method _init_open_subset.

Change History (36)

comment:1 Changed 6 months ago by gh-mjungmath

Why not simply overwriting open_subset in TopologicalSubmanifold etc.?

comment:2 Changed 6 months ago by mkoeppe

It would lead to at least 3 more copies of the same code

comment:3 Changed 6 months ago by mkoeppe

  • Branch set to u/mkoeppe/make_open_subsets_of_immersed_embedded_submanifolds_immersed_embedded_submanifolds

comment:4 Changed 6 months ago by mkoeppe

  • Commit set to 1f6f2958ef44a3d9b00644d8b820851c33484aa0

Here's the first copy


New commits:

1f6f295TopologicalSubmanifold.open_subset: New

comment:5 Changed 6 months ago by git

  • Commit changed from 1f6f2958ef44a3d9b00644d8b820851c33484aa0 to 58110f2ffda1a1736003fe3548586e372e673ba5

Branch pushed to git repo; I updated commit sha1. New commits:

58110f2TopologicalSubmanifold.open_subset: Fix docstring

comment:6 Changed 6 months ago by mkoeppe

  • Authors set to Matthias Koeppe

comment:7 Changed 6 months ago by git

  • Commit changed from 58110f2ffda1a1736003fe3548586e372e673ba5 to d815b63da8314abbb03cfc2afc83c40f61f21c78

Branch pushed to git repo; I updated commit sha1. New commits:

d815b63{DifferentiableSubmanifold,PseudoRiemannianSubmanifold}.open_subset: New

comment:8 Changed 6 months ago by git

  • Commit changed from d815b63da8314abbb03cfc2afc83c40f61f21c78 to 004c23a2a059909105224fec4b8a9b046dc4885a

Branch pushed to git repo; I updated commit sha1. New commits:

aac8835{Topological,Differentiable,PseudoRiemannianSubmanifold}._repr_: Print subsets as subsets
004c23aAdd/update examples

comment:9 in reply to: ↑ description ; follow-ups: Changed 6 months ago by egourgoulhon

Replying to mkoeppe:

To implement this, perhaps the method open_subset, already now duplicated between TopologicalManifold and DifferentiableManifold could be refactored through a new method open_subset_class.

It sounds indeed a good idea to refactor the common code in the two (now three) open_subset methods!

Note that the comment

#!# update vector frames and change of frames

in DifferentiableManifold.open_subset means that vector frames that are not coordinate frames could be inherited from those on the ambient manifold. I've put the marker #!# to indicate that this is not implemented yet. For example:

sage: M = Manifold(2, 'M')                                                                                              
sage: X.<x,y> = M.chart()                                                                                               
sage: e0 = M.vector_field(1, 1)                                                                                          
sage: e1 = M.vector_field(1, -2)                                                                                         
sage: e = M.vector_frame('e', (e0, e1))                                                                                   
sage: M.frames()                                                                                                        
[Coordinate frame (M, (d/dx,d/dy)), Vector frame (M, (e_0,e_1))]
sage: U = M.open_subset('U', coord_def={X: x>0})                                                                        
sage: U.frames()                                                                                                        
[Coordinate frame (U, (d/dx,d/dy))]

The vector frame e is thus not transmitted to U. Of course, this can be achieved at any moment by the method restrict:

sage: e.restrict(U)                                                                                                     
Vector frame (U, (e_0,e_1))
sage: U.frames()                                                                                                        
[Coordinate frame (U, (d/dx,d/dy)), Vector frame (U, (e_0,e_1))]

New commits:

aac8835{Topological,Differentiable,PseudoRiemannianSubmanifold}._repr_: Print subsets as subsets
004c23aAdd/update examples

comment:10 in reply to: ↑ 9 ; follow-up: Changed 6 months ago by mkoeppe

Replying to egourgoulhon:

Replying to mkoeppe: Note that the comment

#!# update vector frames and change of frames

in DifferentiableManifold.open_subset means that vector frames that are not coordinate frames could be inherited from those on the ambient manifold. I've put the marker #!# to indicate that this is not implemented yet.

Yes, I have faithfully copied these TODO markers. Should this be implemented on this ticket?

comment:11 in reply to: ↑ 10 Changed 6 months ago by egourgoulhon

Replying to mkoeppe:

Replying to egourgoulhon:

Replying to mkoeppe: Note that the comment

#!# update vector frames and change of frames

in DifferentiableManifold.open_subset means that vector frames that are not coordinate frames could be inherited from those on the ambient manifold. I've put the marker #!# to indicate that this is not implemented yet.

Yes, I have faithfully copied these TODO markers. Should this be implemented on this ticket?

Maybe. I leave it to you. Actually, I don't remember why this was not implemented in the first version of the open_subset method.

comment:12 Changed 6 months ago by mkoeppe

I'll leave this for another ticket then.

I will do the refactoring using a new method _init_open_subset

comment:13 Changed 6 months ago by git

  • Commit changed from 004c23a2a059909105224fec4b8a9b046dc4885a to ef1614aafe5cabdce11443809ca932125105253b

Branch pushed to git repo; I updated commit sha1. New commits:

ef1614aRefactor Manifold.open_subset methods through new method _init_open_subset

comment:14 Changed 6 months ago by mkoeppe

Here's the proposed refactoring.

comment:15 Changed 6 months ago by mkoeppe

  • Status changed from new to needs_review

comment:16 Changed 6 months ago by egourgoulhon

  • Reviewers set to Eric Gourgoulhon

LGTM.

Could you please fix the coverage issues? (no docstest in the two methods _init_open_subset)

Last edited 6 months ago by egourgoulhon (previous) (diff)

comment:17 Changed 6 months ago by git

  • Commit changed from ef1614aafe5cabdce11443809ca932125105253b to 265b0e26d3db8fea35eec87b5ef9497b8f0ddfab

Branch pushed to git repo; I updated commit sha1. New commits:

265b0e2TopologicalManifold._init_open_subset: Add docstring

comment:18 Changed 6 months ago by mkoeppe

First of several docstrings, more to come

comment:19 follow-up: Changed 6 months ago by mkoeppe

A question: Should this method rather be invoked by the __init__ method of the subset whenever base_manifold is passed?

comment:20 in reply to: ↑ 19 Changed 6 months ago by egourgoulhon

Replying to mkoeppe:

A question: Should this method rather be invoked by the __init__ method of the subset whenever base_manifold is passed?

Yes, why not?

comment:21 follow-up: Changed 6 months ago by mkoeppe

OK, then I will rewrite it like this.

comment:22 in reply to: ↑ 21 ; follow-up: Changed 6 months ago by egourgoulhon

Replying to mkoeppe:

OK, then I will rewrite it like this.

Giving a second thought, this seems indeed a good idea! It makes the object returned by __init__ as "finished" as possible.

comment:23 Changed 6 months ago by mkoeppe

Just will add one more keyword arg to __init__, but I think that should be fine.

comment:24 follow-up: Changed 6 months ago by mkoeppe

I think I would also like to move TopologicalManifold.open_subset to its superclass ManifoldSubset.

comment:25 in reply to: ↑ 24 Changed 6 months ago by mkoeppe

Replying to mkoeppe:

I think I would also like to move TopologicalManifold.open_subset to its superclass ManifoldSubset.

I'll do this in #31677.

comment:26 in reply to: ↑ 22 ; follow-up: Changed 6 months ago by mkoeppe

Replying to egourgoulhon:

Replying to mkoeppe:

OK, then I will rewrite it like this.

Giving a second thought, this seems indeed a good idea! It makes the object returned by __init__ as "finished" as possible.

I tried this approach but it led to complicated changes. Too much for one ticket. So I'll revert to the original plan, completing this ticket by adding docstrings for _init_open_subset. An immediate follow-up is #31677.

comment:27 in reply to: ↑ 9 Changed 6 months ago by mkoeppe

Replying to egourgoulhon:

Note that the comment

#!# update vector frames and change of frames

in DifferentiableManifold.open_subset means that vector frames that are not coordinate frames could be inherited from those on the ambient manifold. I've put the marker #!# to indicate that this is not implemented yet.

I've opened #31678 for this

comment:28 Changed 6 months ago by git

  • Commit changed from 265b0e26d3db8fea35eec87b5ef9497b8f0ddfab to 58b43f7902296384dade8a617441e205956d9824

Branch pushed to git repo; I updated commit sha1. New commits:

a93f7edTopologicalSubmanifold._init_open_subset: Add docstring
58b43f7DifferentiableManifold._init_open_subset: Add docstring

comment:29 Changed 6 months ago by mkoeppe

Ready for review

comment:30 in reply to: ↑ 26 Changed 6 months ago by egourgoulhon

  • Status changed from needs_review to positive_review

Replying to mkoeppe:

Replying to egourgoulhon:

Giving a second thought, this seems indeed a good idea! It makes the object returned by __init__ as "finished" as possible.

I tried this approach but it led to complicated changes. Too much for one ticket. So I'll revert to the original plan, completing this ticket by adding docstrings for _init_open_subset. An immediate follow-up is #31677.

OK, very good. Thanks for your work!

comment:31 Changed 6 months ago by gh-mjungmath

Is there a way to get rid of the various copies of

+        if self is not self._manifold:
+            return "Open subset {} of the {}".format(self._name, self._manifold)

and delegate it?

comment:32 follow-up: Changed 6 months ago by mkoeppe

Well, the _repr_ methods in their entirety are there in multiple copies. This could be simplified, but not on this ticket.

comment:33 Changed 6 months ago by mkoeppe

Thanks for the review!

comment:34 in reply to: ↑ 32 Changed 6 months ago by gh-mjungmath

Replying to mkoeppe:

Well, the _repr_ methods in their entirety are there in multiple copies. This could be simplified, but not on this ticket.

Right, makes sense.

Nice work though! :)

comment:35 Changed 6 months ago by mkoeppe

  • Description modified (diff)

comment:36 Changed 4 months ago by vbraun

  • Branch changed from u/mkoeppe/make_open_subsets_of_immersed_embedded_submanifolds_immersed_embedded_submanifolds to 58b43f7902296384dade8a617441e205956d9824
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.