Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#30178 closed enhancement (fixed)

Manifolds: add orientability

Reported by: Michael Jung Owned by:
Priority: major Milestone: sage-9.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:

Status badges

Description (last modified by Michael Jung)

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 Michael Jung

Description: modified (diff)

comment:2 Changed 2 years ago by Michael Jung

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?

Last edited 2 years ago by Michael Jung (previous) (diff)

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

Cc: Samuel Lelièvre added
Summary: Adding Orientibility and CompactnessManifolds: add orientability and compactness

comment:4 Changed 2 years ago by Michael Jung

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.

Last edited 2 years ago by Michael Jung (previous) (diff)

comment:5 Changed 2 years ago by Michael Jung

Branch: u/gh-mjungmath/orientability

comment:6 Changed 2 years ago by Michael Jung

Commit: b8fcbe52c1193c6215b99e0c4b126844024f0d31

I have added a first draft (without documentation). I wait for your opinions before I go on.


New commits:

b8fcbe5Trac #30178: first draft for orientations

comment:7 Changed 2 years ago by Michael Jung

Cc: Tobias Diez added

comment:8 Changed 2 years ago by Matthias Köppe

Just a quick note that there is already an axiom Compact provided by TopologicalSpaces.

comment:9 Changed 2 years ago by git

Commit: b8fcbe52c1193c6215b99e0c4b126844024f0d31329453eef7cbd0c741c94e84fee5a9ed8ce8aaae

Branch pushed to git repo; I updated commit sha1. New commits:

e1a34b3Trac #30178: orientibility for vector bundles
074b43cTrac #30178: Merge branch 'orientation_alternative' into t/30178/orientability
39844baTrac #30178: unify notation
329453eTrac #30178: orientabilty for vector bundles and diff manifolds finished

comment:10 Changed 2 years ago by git

Commit: 329453eef7cbd0c741c94e84fee5a9ed8ce8aaae8ed6b68a606ecfe03a28c629a6b4715707a44b2c

Branch pushed to git repo; I updated commit sha1. New commits:

8ed6b68Trac #30178: refinement of concepts

comment:11 Changed 2 years ago by git

Commit: 8ed6b68a606ecfe03a28c629a6b4715707a44b2c4725246514e77eeb5da36b58717ae96ba5bdb28e

Branch pushed to git repo; I updated commit sha1. New commits:

034cdf7Trac #30178: volume form adapted to new code
dec40cdTrac #30178: documentation improved
4725246Trac #30178: orientation finished

comment:12 Changed 2 years ago by Michael Jung

Description: modified (diff)
Status: newneeds_review
Summary: Manifolds: add orientability and compactnessManifolds: add orientability

comment:13 Changed 2 years ago by Michael Jung

Description: modified (diff)

comment:14 Changed 2 years ago by Michael Jung

Authors: Michael Jung

I forgot the authorship...again.

comment:15 Changed 2 years ago by git

Commit: 4725246514e77eeb5da36b58717ae96ba5bdb28eafd1577e54f27f0e5a12ba9a0b71f8812a5c837c

Branch pushed to git repo; I updated commit sha1. New commits:

afd1577Trac #30178: doctests fixed

comment:16 Changed 2 years ago by git

Commit: afd1577e54f27f0e5a12ba9a0b71f8812a5c837ca90489115b72283b63ebde813e5c60a0b4cd4d26

Branch pushed to git repo; I updated commit sha1. New commits:

a904891Trac #30178: orientation check in metric fixed

comment:17 Changed 2 years ago by git

Commit: a90489115b72283b63ebde813e5c60a0b4cd4d2676be28c90b797e06ed7247a1236871ae69e0036a

Branch pushed to git repo; I updated commit sha1. New commits:

f2c3a3cTrac #30178: better error message
76be28cTrac #30178: documentation improved + error message reverted

comment:18 Changed 2 years ago by git

Commit: 76be28c90b797e06ed7247a1236871ae69e0036adbd4e5d4d75df3a750573c021db389eacaa0150e

Branch pushed to git repo; I updated commit sha1. New commits:

dbd4e5dTrac #30178: typo in doc

comment:19 Changed 2 years ago by Michael Jung

Patchbot won't go green, startup issues. Can one force another patchbot check?

comment:20 Changed 2 years ago by Travis Scrimshaw

There was a green patchbot run (also that plugin can be a bit more fickle).

comment:21 Changed 2 years ago by Michael Jung

Ah yes, on beta6. Anyway, this ticket is ready for review. :)

comment:22 Changed 2 years ago by Travis Scrimshaw

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 2 years ago by Michael Jung

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 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.

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

Last edited 2 years ago by Michael Jung (previous) (diff)

comment:24 Changed 2 years ago by Michael Jung

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.

Last edited 2 years ago by Michael Jung (previous) (diff)

comment:25 Changed 2 years ago by Travis Scrimshaw

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 2 years ago by Michael Jung

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 git

Commit: dbd4e5d4d75df3a750573c021db389eacaa0150ef56f56e489a2d5969d04136c72912546b7547db4

Branch pushed to git repo; I updated commit sha1. New commits:

f56f56eTrac #30178: improve readability

comment:28 Changed 2 years ago by git

Commit: f56f56e489a2d5969d04136c72912546b7547db454267bc9798965269ab1834e67b4cfdc0a60fd90

Branch pushed to git repo; I updated commit sha1. New commits:

54267bcTrac #30178: indent

comment:29 Changed 2 years ago by Michael Jung

Like this?

comment:30 Changed 2 years ago by Travis Scrimshaw

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 git

Commit: 54267bc9798965269ab1834e67b4cfdc0a60fd903eaaaab6df13c43efcf05138a343952d3b97ea8b

Branch pushed to git repo; I updated commit sha1. New commits:

3eaaaabTrac #30178: one line

comment:32 Changed 2 years ago by Michael Jung

Satisfied? :)

comment:33 Changed 2 years ago by Michael Jung

Patchbot is green.

comment:34 Changed 2 years ago by Travis Scrimshaw

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 ; Changed 2 years ago by Michael Jung

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 here

for 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() (in differentiable/metric.py) is wrong. An error is raised if the manifold does not admit an orientation. Subsequently in hodge_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 2 years ago by Travis Scrimshaw

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() (in differentiable/metric.py) is wrong. An error is raised if the manifold does not admit an orientation. Subsequently in hodge_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 Michael Jung

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).

Last edited 2 years ago by Michael Jung (previous) (diff)

comment:38 Changed 2 years ago by git

Commit: 3eaaaab6df13c43efcf05138a343952d3b97ea8b071efcfeddedee76be284c2fd8298313ce832ed2

Branch pushed to git repo; I updated commit sha1. New commits:

70fae37Trac #30178: cosmetic improvements
071efcfTrac #30178: prefer default chart/frame being orientation

comment:39 Changed 2 years ago by Michael Jung

See my new push. This approach is independent of #30320.

comment:40 Changed 2 years ago by Travis Scrimshaw

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 2 years ago by Michael Jung

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 git

Commit: 071efcfeddedee76be284c2fd8298313ce832ed29b5f74bdf08031d1ed03340a36a956b0eaefc148

Branch pushed to git repo; I updated commit sha1. New commits:

9b5f74bTrac #30178: .. NOTE: block removed

comment:43 Changed 2 years ago by Michael Jung

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 Travis Scrimshaw

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

That is perfect, thank you.

comment:45 Changed 2 years ago by Michael Jung

Okay good. Thank you for the review and your patience. :)

comment:46 Changed 2 years ago by Volker Braun

Status: positive_reviewneeds_work

Merge conflict

comment:47 Changed 2 years ago by git

Commit: 9b5f74bdf08031d1ed03340a36a956b0eaefc148a198d1f50890257f845e52e7ed7be0026d9ed54f

Branch pushed to git repo; I updated commit sha1. New commits:

a198d1fMerge branch 'develop' into t/30178/orientability

comment:48 Changed 2 years ago by Michael Jung

Status: needs_workneeds_review

comment:49 Changed 2 years ago by Travis Scrimshaw

Status: needs_reviewpositive_review

comment:50 Changed 2 years ago by Volker Braun

Branch: u/gh-mjungmath/orientabilitya198d1f50890257f845e52e7ed7be0026d9ed54f
Resolution: fixed
Status: positive_reviewclosed

comment:51 Changed 2 years ago by Eric Gourgoulhon

Commit: a198d1f50890257f845e52e7ed7be0026d9ed54f

See #30519 for a follow up.

Note: See TracTickets for help on using tickets.