Opened 20 months ago
Closed 18 months ago
#31674 closed enhancement (fixed)
Make open subsets of immersed/embedded submanifolds immersed/embedded submanifolds
Reported by:  Matthias Köppe  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  manifolds  Keywords:  
Cc:  Michael Jung, Eric Gourgoulhon, Travis Scrimshaw  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Eric Gourgoulhon 
Report Upstream:  N/A  Work issues:  
Branch:  58b43f7 (Commits, GitHub, GitLab)  Commit:  58b43f7902296384dade8a617441e205956d9824 
Dependencies:  Stopgaps: 
Description (last modified by )
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 20 months ago by
comment:3 Changed 20 months ago by
Branch:  → u/mkoeppe/make_open_subsets_of_immersed_embedded_submanifolds_immersed_embedded_submanifolds 

comment:4 Changed 20 months ago by
Commit:  → 1f6f2958ef44a3d9b00644d8b820851c33484aa0 

comment:5 Changed 20 months ago by
Commit:  1f6f2958ef44a3d9b00644d8b820851c33484aa0 → 58110f2ffda1a1736003fe3548586e372e673ba5 

Branch pushed to git repo; I updated commit sha1. New commits:
58110f2  TopologicalSubmanifold.open_subset: Fix docstring

comment:6 Changed 20 months ago by
Authors:  → Matthias Koeppe 

comment:7 Changed 20 months ago by
Commit:  58110f2ffda1a1736003fe3548586e372e673ba5 → d815b63da8314abbb03cfc2afc83c40f61f21c78 

Branch pushed to git repo; I updated commit sha1. New commits:
d815b63  {DifferentiableSubmanifold,PseudoRiemannianSubmanifold}.open_subset: New

comment:8 Changed 20 months ago by
Commit:  d815b63da8314abbb03cfc2afc83c40f61f21c78 → 004c23a2a059909105224fec4b8a9b046dc4885a 

comment:9 followups: 10 27 Changed 20 months ago by
Replying to mkoeppe:
To implement this, perhaps the method
open_subset
, already now duplicated betweenTopologicalManifold
andDifferentiableManifold
could be refactored through a new methodopen_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

comment:10 followup: 11 Changed 20 months ago by
Replying to egourgoulhon:
Replying to mkoeppe: Note that the comment
#!# update vector frames and change of framesin
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 Changed 20 months ago by
Replying to mkoeppe:
Replying to egourgoulhon:
Replying to mkoeppe: Note that the comment
#!# update vector frames and change of framesin
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 20 months ago by
I'll leave this for another ticket then.
I will do the refactoring using a new method _init_open_subset
comment:13 Changed 20 months ago by
Commit:  004c23a2a059909105224fec4b8a9b046dc4885a → ef1614aafe5cabdce11443809ca932125105253b 

Branch pushed to git repo; I updated commit sha1. New commits:
ef1614a  Refactor Manifold.open_subset methods through new method _init_open_subset

comment:15 Changed 20 months ago by
Status:  new → needs_review 

comment:16 Changed 20 months ago by
Reviewers:  → Eric Gourgoulhon 

LGTM.
Could you please fix the coverage issues? (no docstest in the two methods _init_open_subset
)
comment:17 Changed 20 months ago by
Commit:  ef1614aafe5cabdce11443809ca932125105253b → 265b0e26d3db8fea35eec87b5ef9497b8f0ddfab 

Branch pushed to git repo; I updated commit sha1. New commits:
265b0e2  TopologicalManifold._init_open_subset: Add docstring

comment:19 followup: 20 Changed 20 months ago by
A question: Should this method rather be invoked by the __init__
method of the subset whenever base_manifold
is passed?
comment:20 Changed 20 months ago by
Replying to mkoeppe:
A question: Should this method rather be invoked by the
__init__
method of the subset wheneverbase_manifold
is passed?
Yes, why not?
comment:22 followup: 26 Changed 20 months ago by
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 20 months ago by
Just will add one more keyword arg to __init__
, but I think that should be fine.
comment:24 followup: 25 Changed 20 months ago by
I think I would also like to move TopologicalManifold.open_subset
to its superclass ManifoldSubset
.
comment:25 Changed 20 months ago by
comment:26 followup: 30 Changed 20 months ago by
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 followup is #31677.
comment:27 Changed 20 months ago by
Replying to egourgoulhon:
Note that the comment
#!# update vector frames and change of framesin
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 20 months ago by
Commit:  265b0e26d3db8fea35eec87b5ef9497b8f0ddfab → 58b43f7902296384dade8a617441e205956d9824 

comment:30 Changed 20 months ago by
Status:  needs_review → 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 followup is #31677.
OK, very good. Thanks for your work!
comment:31 Changed 20 months ago by
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 followup: 34 Changed 20 months ago by
Well, the _repr_
methods in their entirety are there in multiple copies. This could be simplified, but not on this ticket.
comment:34 Changed 20 months ago by
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 20 months ago by
Description:  modified (diff) 

comment:36 Changed 18 months ago by
Branch:  u/mkoeppe/make_open_subsets_of_immersed_embedded_submanifolds_immersed_embedded_submanifolds → 58b43f7902296384dade8a617441e205956d9824 

Resolution:  → fixed 
Status:  positive_review → closed 
Why not simply overwriting
open_subset
inTopologicalSubmanifold
etc.?