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:  sage9.4 
Component:  manifolds  Keywords:  
Cc:  ghmjungmath, 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: 
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 6 months ago by
comment:2 Changed 6 months ago by
It would lead to at least 3 more copies of the same code
comment:3 Changed 6 months ago by
 Branch set to u/mkoeppe/make_open_subsets_of_immersed_embedded_submanifolds_immersed_embedded_submanifolds
comment:4 Changed 6 months ago by
 Commit set to 1f6f2958ef44a3d9b00644d8b820851c33484aa0
comment:5 Changed 6 months ago by
 Commit changed from 1f6f2958ef44a3d9b00644d8b820851c33484aa0 to 58110f2ffda1a1736003fe3548586e372e673ba5
Branch pushed to git repo; I updated commit sha1. New commits:
58110f2  TopologicalSubmanifold.open_subset: Fix docstring

comment:6 Changed 6 months ago by
comment:7 Changed 6 months ago by
 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
 Commit changed from d815b63da8314abbb03cfc2afc83c40f61f21c78 to 004c23a2a059909105224fec4b8a9b046dc4885a
comment:9 in reply to: ↑ description ; followups: ↓ 10 ↓ 27 Changed 6 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 in reply to: ↑ 9 ; followup: ↓ 11 Changed 6 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 in reply to: ↑ 10 Changed 6 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 6 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 6 months ago by
 Commit changed from 004c23a2a059909105224fec4b8a9b046dc4885a to 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:14 Changed 6 months ago by
Here's the proposed refactoring.
comment:15 Changed 6 months ago by
 Status changed from new to needs_review
comment:16 Changed 6 months ago by
 Reviewers set to Eric Gourgoulhon
LGTM.
Could you please fix the coverage issues? (no docstest in the two methods _init_open_subset
)
comment:17 Changed 6 months ago by
 Commit changed from ef1614aafe5cabdce11443809ca932125105253b to 265b0e26d3db8fea35eec87b5ef9497b8f0ddfab
Branch pushed to git repo; I updated commit sha1. New commits:
265b0e2  TopologicalManifold._init_open_subset: Add docstring

comment:18 Changed 6 months ago by
First of several docstrings, more to come
comment:19 followup: ↓ 20 Changed 6 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 in reply to: ↑ 19 Changed 6 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:21 followup: ↓ 22 Changed 6 months ago by
OK, then I will rewrite it like this.
comment:22 in reply to: ↑ 21 ; followup: ↓ 26 Changed 6 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 6 months ago by
Just will add one more keyword arg to __init__
, but I think that should be fine.
comment:24 followup: ↓ 25 Changed 6 months ago by
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
comment:26 in reply to: ↑ 22 ; followup: ↓ 30 Changed 6 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 in reply to: ↑ 9 Changed 6 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 6 months ago by
 Commit changed from 265b0e26d3db8fea35eec87b5ef9497b8f0ddfab to 58b43f7902296384dade8a617441e205956d9824
comment:29 Changed 6 months ago by
Ready for review
comment:30 in reply to: ↑ 26 Changed 6 months ago by
 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 followup is #31677.
OK, very good. Thanks for your work!
comment:31 Changed 6 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 6 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:33 Changed 6 months ago by
Thanks for the review!
comment:34 in reply to: ↑ 32 Changed 6 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 6 months ago by
 Description modified (diff)
comment:36 Changed 4 months ago by
 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
Why not simply overwriting
open_subset
inTopologicalSubmanifold
etc.?