#30178 closed enhancement (fixed)
Manifolds: add orientability
Reported by:  Michael Jung  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  manifolds  Keywords:  
Cc:  Eric Gourgoulhon, Travis Scrimshaw, Matthias Köppe, Samuel Lelièvre, Tobias Diez  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 2 years ago by
Description:  modified (diff) 

comment:3 Changed 2 years ago by
Cc:  Samuel Lelièvre added 

Summary:  Adding Orientibility and Compactness → Manifolds: add orientability and compactness 
comment:4 Changed 2 years 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 2 years ago by
Branch:  → u/ghmjungmath/orientability 

comment:6 Changed 2 years ago by
Commit:  → 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 2 years ago by
Cc:  Tobias Diez added 

comment:8 Changed 2 years ago by
Just a quick note that there is already an axiom Compact
provided by TopologicalSpaces
.
comment:9 Changed 2 years ago by
Commit:  b8fcbe52c1193c6215b99e0c4b126844024f0d31 → 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 2 years ago by
Commit:  329453eef7cbd0c741c94e84fee5a9ed8ce8aaae → 8ed6b68a606ecfe03a28c629a6b4715707a44b2c 

Branch pushed to git repo; I updated commit sha1. New commits:
8ed6b68  Trac #30178: refinement of concepts

comment:11 Changed 2 years ago by
Commit:  8ed6b68a606ecfe03a28c629a6b4715707a44b2c → 4725246514e77eeb5da36b58717ae96ba5bdb28e 

comment:12 Changed 2 years ago by
Description:  modified (diff) 

Status:  new → needs_review 
Summary:  Manifolds: add orientability and compactness → Manifolds: add orientability 
comment:13 Changed 2 years ago by
Description:  modified (diff) 

comment:15 Changed 2 years ago by
Commit:  4725246514e77eeb5da36b58717ae96ba5bdb28e → afd1577e54f27f0e5a12ba9a0b71f8812a5c837c 

Branch pushed to git repo; I updated commit sha1. New commits:
afd1577  Trac #30178: doctests fixed

comment:16 Changed 2 years ago by
Commit:  afd1577e54f27f0e5a12ba9a0b71f8812a5c837c → a90489115b72283b63ebde813e5c60a0b4cd4d26 

Branch pushed to git repo; I updated commit sha1. New commits:
a904891  Trac #30178: orientation check in metric fixed

comment:17 Changed 2 years ago by
Commit:  a90489115b72283b63ebde813e5c60a0b4cd4d26 → 76be28c90b797e06ed7247a1236871ae69e0036a 

comment:18 Changed 2 years ago by
Commit:  76be28c90b797e06ed7247a1236871ae69e0036a → dbd4e5d4d75df3a750573c021db389eacaa0150e 

Branch pushed to git repo; I updated commit sha1. New commits:
dbd4e5d  Trac #30178: typo in doc

comment:19 Changed 2 years ago by
Patchbot won't go green, startup issues. Can one force another patchbot check?
comment:20 Changed 2 years ago by
There was a green patchbot run (also that plugin can be a bit more fickle).
comment:22 followup: 23 Changed 2 years 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 2dimensional differentiable manifold M' + 'Tangent bundle TM over the 2dimensional differentiable + manifold M' sage: TM._repr_()  'Tangent bundle TM over the 2dimensional differentiable manifold M' + 'Tangent bundle TM over the 2dimensional 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 Changed 2 years 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 2dimensional differentiable manifold M' + 'Tangent bundle TM over the 2dimensional differentiable + manifold M' sage: TM._repr_()  'Tangent bundle TM over the 2dimensional differentiable manifold M' + 'Tangent bundle TM over the 2dimensional 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/pep0008/#maximumlinelength
comment:24 Changed 2 years 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 followup: 26 Changed 2 years 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 Changed 2 years 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 2 years ago by
Commit:  dbd4e5d4d75df3a750573c021db389eacaa0150e → f56f56e489a2d5969d04136c72912546b7547db4 

Branch pushed to git repo; I updated commit sha1. New commits:
f56f56e  Trac #30178: improve readability

comment:28 Changed 2 years ago by
Commit:  f56f56e489a2d5969d04136c72912546b7547db4 → 54267bc9798965269ab1834e67b4cfdc0a60fd90 

Branch pushed to git repo; I updated commit sha1. New commits:
54267bc  Trac #30178: indent

comment:30 Changed 2 years 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 2 years ago by
Commit:  54267bc9798965269ab1834e67b4cfdc0a60fd90 → 3eaaaab6df13c43efcf05138a343952d3b97ea8b 

Branch pushed to git repo; I updated commit sha1. New commits:
3eaaaab  Trac #30178: one line

comment:34 followup: 35 Changed 2 years ago by
Yes, thank you. However, I do have a few other comments from another readover. 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 followup: 36 Changed 2 years ago by
Replying to tscrim:
Yes, thank you. However, I do have a few other comments from another readover. 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 Changed 2 years ago by
Replying to ghmjungmath:
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 2 years 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 2 years ago by
Commit:  3eaaaab6df13c43efcf05138a343952d3b97ea8b → 071efcfeddedee76be284c2fd8298313ce832ed2 

comment:40 followup: 41 Changed 2 years 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 Changed 2 years 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 2 years ago by
Commit:  071efcfeddedee76be284c2fd8298313ce832ed2 → 9b5f74bdf08031d1ed03340a36a956b0eaefc148 

Branch pushed to git repo; I updated commit sha1. New commits:
9b5f74b  Trac #30178: .. NOTE: block removed

comment:43 Changed 2 years 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 2 years ago by
Reviewers:  → Travis Scrimshaw 

Status:  needs_review → positive_review 
That is perfect, thank you.
comment:47 Changed 2 years ago by
Commit:  9b5f74bdf08031d1ed03340a36a956b0eaefc148 → a198d1f50890257f845e52e7ed7be0026d9ed54f 

Branch pushed to git repo; I updated commit sha1. New commits:
a198d1f  Merge branch 'develop' into t/30178/orientability

comment:48 Changed 2 years ago by
Status:  needs_work → needs_review 

comment:49 Changed 2 years ago by
Status:  needs_review → positive_review 

comment:50 Changed 2 years ago by
Branch:  u/ghmjungmath/orientability → a198d1f50890257f845e52e7ed7be0026d9ed54f 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:51 Changed 2 years ago by
Commit:  a198d1f50890257f845e52e7ed7be0026d9ed54f 

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?