Opened 4 months ago

Closed 4 months ago

Last modified 3 months ago

#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) Commit:
Dependencies: Stopgaps:

Description (last modified by gh-mjungmath)

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 4 months ago by gh-mjungmath

  • Description modified (diff)

comment:2 Changed 4 months ago by gh-mjungmath

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 4 months ago by gh-mjungmath (previous) (diff)

comment:3 Changed 4 months ago by slelievre

  • Cc slelievre added
  • Summary changed from Adding Orientibility and Compactness to Manifolds: add orientability and compactness

comment:4 Changed 4 months ago by gh-mjungmath

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 4 months ago by gh-mjungmath (previous) (diff)

comment:5 Changed 4 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/orientability

comment:6 Changed 4 months ago by gh-mjungmath

  • Commit set to 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 4 months ago by gh-mjungmath

  • Cc gh-tobiasdiez added

comment:8 Changed 4 months ago by mkoeppe

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

comment:9 Changed 4 months ago by git

  • Commit changed from b8fcbe52c1193c6215b99e0c4b126844024f0d31 to 329453eef7cbd0c741c94e84fee5a9ed8ce8aaae

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 4 months ago by git

  • Commit changed from 329453eef7cbd0c741c94e84fee5a9ed8ce8aaae to 8ed6b68a606ecfe03a28c629a6b4715707a44b2c

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

8ed6b68Trac #30178: refinement of concepts

comment:11 Changed 4 months ago by git

  • Commit changed from 8ed6b68a606ecfe03a28c629a6b4715707a44b2c to 4725246514e77eeb5da36b58717ae96ba5bdb28e

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 4 months ago by gh-mjungmath

  • 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 4 months ago by gh-mjungmath

  • Description modified (diff)

comment:14 Changed 4 months ago by gh-mjungmath

  • Authors set to Michael Jung

I forgot the authorship...again.

comment:15 Changed 4 months ago by git

  • Commit changed from 4725246514e77eeb5da36b58717ae96ba5bdb28e to afd1577e54f27f0e5a12ba9a0b71f8812a5c837c

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

afd1577Trac #30178: doctests fixed

comment:16 Changed 4 months ago by git

  • Commit changed from afd1577e54f27f0e5a12ba9a0b71f8812a5c837c to a90489115b72283b63ebde813e5c60a0b4cd4d26

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

a904891Trac #30178: orientation check in metric fixed

comment:17 Changed 4 months ago by git

  • Commit changed from a90489115b72283b63ebde813e5c60a0b4cd4d26 to 76be28c90b797e06ed7247a1236871ae69e0036a

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 4 months ago by git

  • Commit changed from 76be28c90b797e06ed7247a1236871ae69e0036a to dbd4e5d4d75df3a750573c021db389eacaa0150e

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

dbd4e5dTrac #30178: typo in doc

comment:19 Changed 4 months ago by gh-mjungmath

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

comment:20 Changed 4 months ago by tscrim

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

comment:21 Changed 4 months ago by gh-mjungmath

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

comment:22 follow-up: Changed 4 months ago by 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.

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 4 months ago by gh-mjungmath

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.

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

Version 0, edited 4 months ago by gh-mjungmath (next)

comment:24 Changed 4 months ago by gh-mjungmath

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 4 months ago by gh-mjungmath (previous) (diff)

comment:25 follow-up: Changed 4 months ago by tscrim

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 4 months ago by gh-mjungmath

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 4 months ago by git

  • Commit changed from dbd4e5d4d75df3a750573c021db389eacaa0150e to f56f56e489a2d5969d04136c72912546b7547db4

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

f56f56eTrac #30178: improve readability

comment:28 Changed 4 months ago by git

  • Commit changed from f56f56e489a2d5969d04136c72912546b7547db4 to 54267bc9798965269ab1834e67b4cfdc0a60fd90

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

54267bcTrac #30178: indent

comment:29 Changed 4 months ago by gh-mjungmath

Like this?

comment:30 Changed 4 months ago by tscrim

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 4 months ago by git

  • Commit changed from 54267bc9798965269ab1834e67b4cfdc0a60fd90 to 3eaaaab6df13c43efcf05138a343952d3b97ea8b

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

3eaaaabTrac #30178: one line

comment:32 Changed 4 months ago by gh-mjungmath

Satisfied? :)

comment:33 Changed 4 months ago by gh-mjungmath

Patchbot is green.

comment:34 follow-up: Changed 4 months ago by 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.

         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: Changed 4 months ago by gh-mjungmath

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 4 months ago by tscrim

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 4 months ago by gh-mjungmath

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 4 months ago by gh-mjungmath (previous) (diff)

comment:38 Changed 4 months ago by git

  • Commit changed from 3eaaaab6df13c43efcf05138a343952d3b97ea8b to 071efcfeddedee76be284c2fd8298313ce832ed2

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 4 months ago by gh-mjungmath

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

comment:40 follow-up: Changed 4 months ago by 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.

comment:41 in reply to: ↑ 40 Changed 4 months ago by gh-mjungmath

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 4 months ago by git

  • Commit changed from 071efcfeddedee76be284c2fd8298313ce832ed2 to 9b5f74bdf08031d1ed03340a36a956b0eaefc148

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

9b5f74bTrac #30178: .. NOTE: block removed

comment:43 Changed 4 months ago by gh-mjungmath

Is this better? If not, could you please suggest an improvement, because then I don't know what you mean.

comment:44 Changed 4 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

That is perfect, thank you.

comment:45 Changed 4 months ago by gh-mjungmath

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

comment:46 Changed 4 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:47 Changed 4 months ago by git

  • Commit changed from 9b5f74bdf08031d1ed03340a36a956b0eaefc148 to a198d1f50890257f845e52e7ed7be0026d9ed54f

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

a198d1fMerge branch 'develop' into t/30178/orientability

comment:48 Changed 4 months ago by gh-mjungmath

  • Status changed from needs_work to needs_review

comment:49 Changed 4 months ago by tscrim

  • Status changed from needs_review to positive_review

comment:50 Changed 4 months ago by vbraun

  • Branch changed from u/gh-mjungmath/orientability to a198d1f50890257f845e52e7ed7be0026d9ed54f
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:51 Changed 3 months ago by egourgoulhon

  • Commit a198d1f50890257f845e52e7ed7be0026d9ed54f deleted

See #30519 for a follow up.

Note: See TracTickets for help on using tickets.