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

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

[gh-mjungmath]

Why not simply overwriting open_subset in TopologicalSubmanifold etc.?

[mkoeppe]

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

[mkoeppe]

Here's the first copy

---

New commits:

​1f6f295

TopologicalSubmanifold.open_subset: New

[git]

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

​58110f2

TopologicalSubmanifold.open_subset: Fix docstring

[git]

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

​d815b63

{DifferentiableSubmanifold,PseudoRiemannianSubmanifold}.open_subset: New

[git]

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

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

[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
​004c23a	Add/update examples

[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?

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

[mkoeppe]

I'll leave this for another ticket then.

I will do the refactoring using a new method _init_open_subset

[git]

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

​ef1614a

Refactor Manifold.open_subset methods through new method _init_open_subset

[mkoeppe]

Here's the proposed refactoring.

[egourgoulhon]

LGTM.

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

[git]

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

​265b0e2

TopologicalManifold._init_open_subset: Add docstring

[mkoeppe]

First of several docstrings, more to come

[mkoeppe]

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

[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?

[mkoeppe]

OK, then I will rewrite it like this.

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

[mkoeppe]

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

[mkoeppe]

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

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

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

[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

[git]

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

​a93f7ed	TopologicalSubmanifold._init_open_subset: Add docstring
​58b43f7	DifferentiableManifold._init_open_subset: Add docstring

[mkoeppe]

Ready for review

[egourgoulhon]

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!

[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?

[mkoeppe]

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

[mkoeppe]

Thanks for the review!

[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! :)