#30178 closed enhancement (fixed)
Manifolds: add orientability
Reported by: | gh-mjungmath | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.2 |
Component: | manifolds | Keywords: | |
Cc: | egourgoulhon, tscrim, mkoeppe, slelievre, gh-tobiasdiez | Merged in: | |
Authors: | Michael Jung | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | a198d1f (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description (last modified by )
It would be good if one can state whether a manifold is orientable as some statements only apply for these kind of manifolds. Of course, at the end it will be, except for very trivial cases, the user's responsibility that the conditions are actually satisfied.
Note: Compactness follows in a later ticket
Change History (51)
comment:1 Changed 8 months ago by
- Description modified (diff)
comment:2 Changed 8 months ago by
comment:3 Changed 8 months ago by
- Cc slelievre added
- Summary changed from Adding Orientibility and Compactness to Manifolds: add orientability and compactness
comment:4 Changed 8 months ago by
In the differentiable setting, things are easy and can be managed by using frames. This is quite convenient because there are manifolds which can be covered by one global frame, and therefore admit an orientation, but need at least two charts.
However, I know that there is a notion of orientability in the topological setting, where the use of charts is mandatory. I look for further literature.
I would prefer the chart version because it is applicable to both, topological and differentiable manifolds. Because the former case is easier and more practical, one could also implement orientations only for differentiable manifolds at first. Further opinions on that?
Addendum: If someone has an idea of how both cases, charts and frames, can be managed conveniently, I welcome that pretty much.
comment:5 Changed 8 months ago by
- Branch set to u/gh-mjungmath/orientability
comment:6 Changed 8 months ago by
- Commit set to b8fcbe52c1193c6215b99e0c4b126844024f0d31
I have added a first draft (without documentation). I wait for your opinions before I go on.
New commits:
b8fcbe5 | Trac #30178: first draft for orientations
|
comment:7 Changed 7 months ago by
- Cc gh-tobiasdiez added
comment:8 Changed 7 months ago by
Just a quick note that there is already an axiom Compact
provided by TopologicalSpaces
.
comment:9 Changed 7 months ago by
- Commit changed from b8fcbe52c1193c6215b99e0c4b126844024f0d31 to 329453eef7cbd0c741c94e84fee5a9ed8ce8aaae
Branch pushed to git repo; I updated commit sha1. New commits:
e1a34b3 | Trac #30178: orientibility for vector bundles
|
074b43c | Trac #30178: Merge branch 'orientation_alternative' into t/30178/orientability
|
39844ba | Trac #30178: unify notation
|
329453e | Trac #30178: orientabilty for vector bundles and diff manifolds finished
|
comment:10 Changed 7 months ago by
- Commit changed from 329453eef7cbd0c741c94e84fee5a9ed8ce8aaae to 8ed6b68a606ecfe03a28c629a6b4715707a44b2c
Branch pushed to git repo; I updated commit sha1. New commits:
8ed6b68 | Trac #30178: refinement of concepts
|
comment:11 Changed 7 months ago by
- Commit changed from 8ed6b68a606ecfe03a28c629a6b4715707a44b2c to 4725246514e77eeb5da36b58717ae96ba5bdb28e
comment:12 Changed 7 months ago by
- Description modified (diff)
- Status changed from new to needs_review
- Summary changed from Manifolds: add orientability and compactness to Manifolds: add orientability
comment:13 Changed 7 months ago by
- Description modified (diff)
comment:15 Changed 7 months ago by
- Commit changed from 4725246514e77eeb5da36b58717ae96ba5bdb28e to afd1577e54f27f0e5a12ba9a0b71f8812a5c837c
Branch pushed to git repo; I updated commit sha1. New commits:
afd1577 | Trac #30178: doctests fixed
|
comment:16 Changed 7 months ago by
- Commit changed from afd1577e54f27f0e5a12ba9a0b71f8812a5c837c to a90489115b72283b63ebde813e5c60a0b4cd4d26
Branch pushed to git repo; I updated commit sha1. New commits:
a904891 | Trac #30178: orientation check in metric fixed
|
comment:17 Changed 7 months ago by
- Commit changed from a90489115b72283b63ebde813e5c60a0b4cd4d26 to 76be28c90b797e06ed7247a1236871ae69e0036a
comment:18 Changed 7 months ago by
- Commit changed from 76be28c90b797e06ed7247a1236871ae69e0036a to dbd4e5d4d75df3a750573c021db389eacaa0150e
Branch pushed to git repo; I updated commit sha1. New commits:
dbd4e5d | Trac #30178: typo in doc
|
comment:19 Changed 7 months ago by
Patchbot won't go green, startup issues. Can one force another patchbot check?
comment:20 Changed 7 months ago by
There was a green patchbot run (also that plugin can be a bit more fickle).
comment:21 Changed 7 months ago by
Ah yes, on beta6. Anyway, this ticket is ready for review. :)
comment:22 follow-up: ↓ 23 Changed 7 months ago by
In response to your questions/comments from comment:2: I think it should just be the user's responsibility as it would be too costly and annoying to include code to check this. Trusting the usuer is done in a number of places in Sage too. As Matthias said, you can use Manifolds(R).Compact()
to indicate compactness.
A few stylistic things:
You are saving 1 character here
from sage.manifolds.differentiable.vector_bundle \ import TensorBundle
Just go over the 80 char/line limit; it makes it much more readable.
Similar for these changes:
sage: repr(TM) # indirect doctest - 'Tangent bundle TM over the 2-dimensional differentiable manifold M' + 'Tangent bundle TM over the 2-dimensional differentiable + manifold M' sage: TM._repr_() - 'Tangent bundle TM over the 2-dimensional differentiable manifold M' + 'Tangent bundle TM over the 2-dimensional differentiable + manifold M'
Further indent the last line on this change and all other similar changes:
{(Chart (M, (x, y)), - Chart (M, (u, v))): Change of coordinates from Chart (M, (x, y)) to Chart (M, (u, v))} + Chart (M, (u, v))): Change of coordinates from Chart (M, (x, y)) + to Chart (M, (u, v))}
to make it (more) clear that it is associated to that item.
comment:23 in reply to: ↑ 22 Changed 7 months ago by
Replying to tscrim:
In response to your questions/comments from comment:2: I think it should just be the user's responsibility as it would be too costly and annoying to include code to check this. Trusting the usuer is done in a number of places in Sage too. As Matthias said, you can use
Manifolds(R).Compact()
to indicate compactness.
Right. There are many tickets going on simultaneously. I have to catch them up first.
A few stylistic things:
You are saving 1 character here
from sage.manifolds.differentiable.vector_bundle \ import TensorBundleJust go over the 80 char/line limit; it makes it much more readable.
Similar for these changes:
sage: repr(TM) # indirect doctest - 'Tangent bundle TM over the 2-dimensional differentiable manifold M' + 'Tangent bundle TM over the 2-dimensional differentiable + manifold M' sage: TM._repr_() - 'Tangent bundle TM over the 2-dimensional differentiable manifold M' + 'Tangent bundle TM over the 2-dimensional differentiable + manifold M'Further indent the last line on this change and all other similar changes:
{(Chart (M, (x, y)), - Chart (M, (u, v))): Change of coordinates from Chart (M, (x, y)) to Chart (M, (u, v))} + Chart (M, (u, v))): Change of coordinates from Chart (M, (x, y)) + to Chart (M, (u, v))}to make it (more) clear that it is associated to that item.
Mh, okay. Well, I followed the PEP8 guidelines which indicates 79 characters per line: https://www.python.org/dev/peps/pep-0008/#maximum-line-length
comment:24 Changed 7 months ago by
Apparently, the guidelines have changed a couple of years ago. The PEP8 guidelines are linked in the Sage reference but their recommendations have not been updated.
comment:25 follow-up: ↓ 26 Changed 7 months ago by
PEP8 sets guidelines, but code readability is the most important thing. Each of those changes makes it harder to read the code.
comment:26 in reply to: ↑ 25 Changed 7 months ago by
Replying to tscrim:
PEP8 sets guidelines, but code readability is the most important thing. Each of those changes makes it harder to read the code.
Fair enough, I'll change it back. Thanks.
comment:27 Changed 7 months ago by
- Commit changed from dbd4e5d4d75df3a750573c021db389eacaa0150e to f56f56e489a2d5969d04136c72912546b7547db4
Branch pushed to git repo; I updated commit sha1. New commits:
f56f56e | Trac #30178: improve readability
|
comment:28 Changed 7 months ago by
- Commit changed from f56f56e489a2d5969d04136c72912546b7547db4 to 54267bc9798965269ab1834e67b4cfdc0a60fd90
Branch pushed to git repo; I updated commit sha1. New commits:
54267bc | Trac #30178: indent
|
comment:29 Changed 7 months ago by
Like this?
comment:30 Changed 7 months ago by
Yes, except I think you are better off leaving this alone:
from sage.manifolds.differentiable.vector_bundle import TensorBundle
(i.e., on one line).
comment:31 Changed 7 months ago by
- Commit changed from 54267bc9798965269ab1834e67b4cfdc0a60fd90 to 3eaaaab6df13c43efcf05138a343952d3b97ea8b
Branch pushed to git repo; I updated commit sha1. New commits:
3eaaaab | Trac #30178: one line
|
comment:32 Changed 7 months ago by
Satisfied? :)
comment:33 Changed 7 months ago by
Patchbot is green.
comment:34 follow-up: ↓ 35 Changed 7 months ago by
Yes, thank you. However, I do have a few other comments from another read-over. However, these should be the last of them.
The keys()
call here
for frame in base_space._get_min_covering(connection._coefficients.keys()):
seem like it is not needed.
INPUT: - - an orientation; either a chart / list of charts, or a vector frame / + - ``orientation`` -- either a chart / list of charts, or a vector frame / list of vector frames, covering ``self``
and similar.
This
.. NOTE:: This assumes that the manifold admits an orientation, see :meth:`~sage.manifolds.manifold.DifferentiableManifold.orientation` for details.
in volume_form()
(in differentiable/metric.py
) is wrong. An error is raised if the manifold does not admit an orientation. Subsequently in hodge_star
as well.
In set_orientation
is it the default orientation or simply the orientation? I don't think you allow multiple orientations on a manifold. Is that a planned future feature?
# set a priori no orientation
-> # default is no orientation
(or # set no orientation a priori
)
- if self.orientation(): - return True - return False + return bool(self.orientation())
comment:35 in reply to: ↑ 34 ; follow-up: ↓ 36 Changed 7 months ago by
Replying to tscrim:
Yes, thank you. However, I do have a few other comments from another read-over. However, these should be the last of them.
The
keys()
call herefor frame in base_space._get_min_covering(connection._coefficients.keys()):seem like it is not needed.
Ah yes. Thank you.
INPUT: - - an orientation; either a chart / list of charts, or a vector frame / + - ``orientation`` -- either a chart / list of charts, or a vector frame / list of vector frames, covering ``self``and similar.
Good. Thanks.
This
.. NOTE:: This assumes that the manifold admits an orientation, see :meth:`~sage.manifolds.manifold.DifferentiableManifold.orientation` for details.in
volume_form()
(indifferentiable/metric.py
) is wrong. An error is raised if the manifold does not admit an orientation. Subsequently inhodge_star
as well.
You mean wrong in the sense that we should note that having no orientation means this feature is not available?
In
set_orientation
is it the default orientation or simply the orientation? I don't think you allow multiple orientations on a manifold. Is that a planned future feature?
I cannot think of a case where a manifold has the orientation. Allowing multiple orientations means also allowing multiple volume forms. But which one then to choose for hodge_star
? I think it is reasonable to fix just one orientation.
# set a priori no orientation
-># default is no orientation
(or# set no orientation a priori
)- if self.orientation(): - return True - return False + return bool(self.orientation())
Good. Already done.
comment:36 in reply to: ↑ 35 Changed 7 months ago by
Replying to gh-mjungmath:
Replying to tscrim:
This
.. NOTE:: This assumes that the manifold admits an orientation, see :meth:`~sage.manifolds.manifold.DifferentiableManifold.orientation` for details.in
volume_form()
(indifferentiable/metric.py
) is wrong. An error is raised if the manifold does not admit an orientation. Subsequently inhodge_star
as well.You mean wrong in the sense that we should note that having no orientation means this feature is not available?
No, as in there is not an assumption being made. If it has not been defined, an error is raised rather than assuming that the manifold admits an orientation (and using that implicit orientation).
In
set_orientation
is it the default orientation or simply the orientation? I don't think you allow multiple orientations on a manifold. Is that a planned future feature?I cannot think of a case where a manifold has the orientation. Allowing multiple orientations means also allowing multiple volume forms. But which one then to choose for
hodge_star
? I think it is reasonable to fix just one orientation.
I know there are multiple orientations possible for a manifold, but there is a set orientation for the manifold. There is not a default one where a user can arbitrarily choose another as an input. Contrast this with charts. I would think of this like a manifold with a distinguished orientation (which can be changed later on, but the manifold has still indicated the one orientation to rule them all).
comment:37 Changed 7 months ago by
Okay, I will call it "preferred" orientation instead. It would be good if, in the trivial case, the default chart/frame is used. While applying this, I discovered a bug (see #30320).
comment:38 Changed 7 months ago by
- Commit changed from 3eaaaab6df13c43efcf05138a343952d3b97ea8b to 071efcfeddedee76be284c2fd8298313ce832ed2
comment:39 Changed 7 months ago by
See my new push. This approach is independent of #30320.
comment:40 follow-up: ↓ 41 Changed 7 months ago by
Just to be clear, as long as you raise an error when there is no orientation, you need to remove that .. NOTE::
because it is wrong. No assumption is made as an explicit check is performed.
comment:41 in reply to: ↑ 40 Changed 7 months ago by
Replying to tscrim:
Just to be clear, as long as you raise an error when there is no orientation, you need to remove that
.. NOTE::
because it is wrong. No assumption is made as an explicit check is performed.
Ah, it's just about the note block. I'll remove it.
comment:42 Changed 7 months ago by
- Commit changed from 071efcfeddedee76be284c2fd8298313ce832ed2 to 9b5f74bdf08031d1ed03340a36a956b0eaefc148
Branch pushed to git repo; I updated commit sha1. New commits:
9b5f74b | Trac #30178: .. NOTE: block removed
|
comment:43 Changed 7 months ago by
Is this better? If not, could you please suggest an improvement, because then I don't know what you mean.
comment:44 Changed 7 months ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
That is perfect, thank you.
comment:45 Changed 7 months ago by
Okay good. Thank you for the review and your patience. :)
comment:47 Changed 7 months ago by
- Commit changed from 9b5f74bdf08031d1ed03340a36a956b0eaefc148 to a198d1f50890257f845e52e7ed7be0026d9ed54f
Branch pushed to git repo; I updated commit sha1. New commits:
a198d1f | Merge branch 'develop' into t/30178/orientability
|
comment:48 Changed 7 months ago by
- Status changed from needs_work to needs_review
comment:49 Changed 7 months ago by
- Status changed from needs_review to positive_review
comment:50 Changed 7 months ago by
- Branch changed from u/gh-mjungmath/orientability to a198d1f50890257f845e52e7ed7be0026d9ed54f
- Resolution set to fixed
- Status changed from positive_review to closed
comment:51 Changed 6 months ago by
- Commit a198d1f50890257f845e52e7ed7be0026d9ed54f deleted
See #30519 for a follow up.
Regarding orientability:
One could add a method
set_orientation
which takes a list of charts or frames which cover the manifold yielding an orientation. Then it is the user's responsibility to check that this is indeed an orientation. Using this, one can construct things like volume forms etc.In very easy cases, like if the manifold is covered by only one chart, one can give the orientation right away.
Remark: Notice that the volume form returned by sage.manifolds.differentiable.metric.PseudoRiemannianMetric.volume_form does not necessarily yield a volume form at all. One indeed needs an oriented atlas. Otherwise, the gluing process could be messed up. This can all be prevented and covered by my proposal above.
Regarding Compactness:
I think, the best way would be to state compactness right from the beginning. Not stating compactness should, in my opinion, not exclude compactness. Maybe a new manifold class does a suitable job?