Opened 15 months ago
Closed 13 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: |
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 15 months ago by
comment:2 Changed 15 months ago by
It would lead to at least 3 more copies of the same code
comment:3 Changed 15 months ago by
- Branch set to u/mkoeppe/make_open_subsets_of_immersed_embedded_submanifolds_immersed_embedded_submanifolds
comment:4 Changed 15 months ago by
- Commit set to 1f6f2958ef44a3d9b00644d8b820851c33484aa0
comment:5 Changed 15 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 15 months ago by
comment:7 Changed 15 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 15 months ago by
- Commit changed from d815b63da8314abbb03cfc2afc83c40f61f21c78 to 004c23a2a059909105224fec4b8a9b046dc4885a
comment:9 in reply to: ↑ description ; follow-ups: ↓ 10 ↓ 27 Changed 15 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 ; follow-up: ↓ 11 Changed 15 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 15 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 15 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 15 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 15 months ago by
Here's the proposed refactoring.
comment:15 Changed 15 months ago by
- Status changed from new to needs_review
comment:16 Changed 15 months ago by
- Reviewers set to Eric Gourgoulhon
LGTM.
Could you please fix the coverage issues? (no docstring in the two methods _init_open_subset
)
comment:17 Changed 15 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 15 months ago by
First of several docstrings, more to come
comment:19 follow-up: ↓ 20 Changed 15 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 15 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 follow-up: ↓ 22 Changed 15 months ago by
OK, then I will rewrite it like this.
comment:22 in reply to: ↑ 21 ; follow-up: ↓ 26 Changed 15 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 15 months ago by
Just will add one more keyword arg to __init__
, but I think that should be fine.
comment:24 follow-up: ↓ 25 Changed 15 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 15 months ago by
comment:26 in reply to: ↑ 22 ; follow-up: ↓ 30 Changed 15 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 follow-up is #31677.
comment:27 in reply to: ↑ 9 Changed 15 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 15 months ago by
- Commit changed from 265b0e26d3db8fea35eec87b5ef9497b8f0ddfab to 58b43f7902296384dade8a617441e205956d9824
comment:29 Changed 15 months ago by
Ready for review
comment:30 in reply to: ↑ 26 Changed 15 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 follow-up is #31677.
OK, very good. Thanks for your work!
comment:31 Changed 15 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 follow-up: ↓ 34 Changed 15 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 15 months ago by
Thanks for the review!
comment:34 in reply to: ↑ 32 Changed 15 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 15 months ago by
- Description modified (diff)
comment:36 Changed 13 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.?