Opened 12 months ago

Closed 9 months ago

#28159 closed enhancement (fixed)

Vector Bundles

Reported by: gh-DeRhamSource Owned by:
Priority: major Milestone: sage-9.0
Component: geometry Keywords: vector bundles, manifolds
Cc: tscrim, egourgoulhon Merged in:
Authors: Michael Jung Reviewers: Eric Gourgoulhon, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 4ae4ca3 (Commits) Commit: 4ae4ca37508374ef9074cdbf9c650bb0d6ae8211
Dependencies: #28189 Stopgaps:

Description (last modified by gh-DeRhamSource)

Implementation of topological and differentiable vector bundles over manifolds.

Features:

  • abstract vector bundles on topological manifolds
  • trivializations
  • local frames
  • simple sections
  • vector bundle fibers
  • differentiable vector bundles and tensor bundles

Improvements in the preexisting code

  • restriction of vectorframes depended on some strange order: fixed!
  • SR enforcement for automorphism field inversion removed (due to ticket #28189)
  • is_unit() method superficially implemented for scalar fields to ensure division free matrix inversion for changes of frames (may be removed when #28629 is solved)

Planned in the farther future

  • sections of general tensor products and automorphism fields on vector bundles
  • tensor products and Whitney sums of vector bundles
  • better zero treatment as in #28562
  • TensorField inherits from Section, VectorFrame inherits from LocalFrame
  • total space comes with induced charts

Change History (128)

comment:1 Changed 12 months ago by gh-DeRhamSource

  • Authors set to Michael Jung
  • Cc tscrim egourgoulhon added
  • Keywords vector bundles manifolds added
  • Summary changed from vector_bundles to Vector Bundles
  • Type changed from PLEASE CHANGE to enhancement

comment:2 Changed 12 months ago by gh-DeRhamSource

  • Component changed from PLEASE CHANGE to geometry

comment:3 Changed 12 months ago by gh-DeRhamSource

  • Branch set to u/gh-DeRhamSource/vector_bundles

comment:4 follow-up: Changed 12 months ago by gh-DeRhamSource

  • Commit set to 5d5520d19abace16d1eced2d67a28e2f1fbaa9fb

Please take a short look and give me a short feedback. I want to know whether I'm on the right way or not.

Furthermore, I appreciate tricks and advices. Thanks in advance! :)


New commits:

5d5520dBasic structure
Last edited 12 months ago by gh-DeRhamSource (previous) (diff)

comment:5 Changed 12 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:6 in reply to: ↑ 4 Changed 12 months ago by egourgoulhon

Replying to gh-DeRhamSource:

Please take a short look and give me a short feedback. I want to know whether I'm on the right way or not.

Furthermore, I appreciate tricks and advices. Thanks in advance! :)

Thanks for this endeavor. I gave a look: it looks very nice! Please go on.

A minor comment: for class names, full names are preferable, so TopolVectorBundle could be replaced by TopologicalVectorBundle. Also maybe, in the context of vector bundles, TransitionMap is more appropriate than CoordChange.

comment:7 Changed 12 months ago by gh-DeRhamSource

Thanks!

Last edited 12 months ago by gh-DeRhamSource (previous) (diff)

comment:8 Changed 12 months ago by git

  • Commit changed from 5d5520d19abace16d1eced2d67a28e2f1fbaa9fb to ed60a5d8c5c03acf2627265bae19c8139376b126

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

ed60a5dTrivializations almost finished

comment:9 Changed 12 months ago by git

  • Commit changed from ed60a5d8c5c03acf2627265bae19c8139376b126 to b23b6768f4ea50f7f9aedaa117ec13ca6a84d576

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

2a2bbb428189: matrices
0919d50Merge branch 't/28189/28189' into t/28159/vector_bundles
b23b676Matrix inversion adapted to ticket #28189

comment:10 Changed 12 months ago by gh-DeRhamSource

  • Dependencies set to #28189

comment:11 Changed 12 months ago by git

  • Commit changed from b23b6768f4ea50f7f9aedaa117ec13ca6a84d576 to bc1fac17d30a5f60f61e33a4e54aec2d216d64de

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

bc1fac1Fibers and vectors of fibers implemented

comment:12 follow-up: Changed 12 months ago by gh-DeRhamSource

What about synergy and compatibility with the old code? I mean, tangent bundles and vector fields are just special cases of vector bundles and sections. So, it'd be nice when we don't have two different implementations for the very same thing. How would you like to handle that? Any suggestions?

comment:13 Changed 12 months ago by gh-DeRhamSource

Also, it could be a good idea to generalize tensor fields and vector fields to tensor product sections and treat tensor fields as a special case then. But that's a huge step with pretty much effort (because I'm not that much into your complete manifold code and the synergies between your files), where I'd appreciate some help -- if you agree of course.

My first approach in mind is to implement just simple sections and program tensor products of bundles aswell.

Last edited 12 months ago by gh-DeRhamSource (previous) (diff)

comment:14 in reply to: ↑ 12 Changed 12 months ago by egourgoulhon

Replying to gh-DeRhamSource:

What about synergy and compatibility with the old code? I mean, tangent bundles and vector fields are just special cases of vector bundles and sections. So, it'd be nice when we don't have two different implementations for the very same thing. How would you like to handle that? Any suggestions?

Sorry for the delay in replying. IMHO, you should introduce a subclass of TopologicalVectorBundle for the tangent bundle (or more generally for tensor bundles) and simply have the method section of this subclass return a vector field on the manifold, as defined by the "old" code. Maybe it is sufficient to have a single subclass for all tensor bundles, including the tangent and cotangent bundles, i.e. something like

class TensorBundle(TopologicalVectorBundle):

    def __init__(self, manifold, k, l):
        rank = manifold.dim()**(k+l)
        if (k, l) == (1, 0):
            name = "T{}".format(manifold._name)
        elif (k, l) == (0, 1):
            name = "T*{}".format(manifold._name)
        else:
            name = "T^({}{}){}".format(k, l, manifold._name)
        # similar code for latex_name
        TopologicalVectorBundle.__init__(self, rank, name, manifold, 
                                         field=manifold.base_field(),
                                         latex_name=latex_name)
        self._tensor_type = (k, l)

    def section(self, *args, **kwargs):
        if self.tensor_type == (1, 0):
            return self._base_space.vector_field(*args, **kwargs)
        return self._base_space.tensor_field(*(self._tensor_type + args), **kwargs)

In the code for differentiable manifolds, you could then add

class DifferentiableManifold(TopologicalManifold):

    ...

    def tangent_bundle(self):
        return TensorBundle(self, 1, 0)

    def cotangent_bundle(self):
        return TensorBundle(self, 0, 1)

    def tensor_bundle(self, k, l)
        return TensorBundle(self, k, l)

Last edited 12 months ago by egourgoulhon (previous) (diff)

comment:15 Changed 12 months ago by egourgoulhon

Also, I don't know if this is useful at this stage, but you could introduce an intermediate class, DifferentiableVectorBundle say, and let TensorBundle inherits from it. Besides, since (the total spaces of) differentiable vector bundles are differentiable manifolds, this intermediate class could inherit from DifferentiableManifold as well:

class DifferentiableVectorBundle(TopologicalVectorBundle, DifferentiableManifold):

    def __init__(self, rank, name, base_space, diff_degree=infinity,
                 latex_name=None, category=None)
        field = base_space.base_field()
        TopologicalVectorBundle.__init__(self, rank, name, base_space, field=field,
                                         latex_name=latex_name, category=category)
        dim = base_space.dim() * rank
        if isinstance(field, RealField_class):
            structure = RealDifferentialStructure()
        else:
            structure = DifferentialStructure()
        DifferentiableManifold.__init__(self, dim, name, field, structure,
                                        diff_degree=diff_degree, 
                                        latex_name=latex_name, 
                                        start_index=base_space.start_index())

class TensorBundle(DifferentiableVectorBundle):
    ...

comment:16 Changed 12 months ago by egourgoulhon

In addition, to make the link with preexisting code, one could redefine the method fiber() of TensorBundle to return the tangent space at the given point if (k,l)==(1,0) or the corresponding tensor product for other values of (k,l):

class TensorBundle(DifferentiableVectorBundle):

    def __init__(self, manifold, k, l):
        rank = manifold.dim()**(k+l)
        if (k, l) == (1, 0):
            name = "T{}".format(manifold._name)
        elif (k, l) == (0, 1):
            name = "T*{}".format(manifold._name)
        else:
            name = "T^({}{}){}".format(k, l, manifold._name)
        # similar code for latex_name
        DifferentiableVectorBundle.__init__(self, rank, name, manifold, 
                                            diff_degree=manifold.diff_degree(),
                                            latex_name=latex_name)
        self._tensor_type = (k, l)

    def section(self, *args, **kwargs):
        if self.tensor_type == (1, 0):
            return self._base_space.vector_field(*args, **kwargs)
        return self._base_space.tensor_field(*(self._tensor_type + args), **kwargs)
  
    def fiber(self, point):
        return self._base_space.tangent_space(point).tensor_module(*self._tensor_type)

comment:17 follow-up: Changed 12 months ago by gh-DeRhamSource

Sounds like a very good compromise to me. I'll see what I can do. Thanks for your input!

But I feel not comfortable with inheriting from DifferentiableManifold. There are too many conflicts (frame(), atlas(), etc.). Maybe, having a total_space() method returning the corresponding manifold would be a better idea. What do you think?

comment:18 in reply to: ↑ 17 Changed 12 months ago by egourgoulhon

Replying to gh-DeRhamSource:

Sounds like a very good compromise to me. I'll see what I can do. Thanks for your input!

But I feel not comfortable with inheriting from DifferentiableManifold. There are too many conflicts (frame(), atlas(), etc.). Maybe, having a total_space() method returning the corresponding manifold would be a better idea. What do you think?

This seems indeed a good idea. Also sounds closer to the mathematical definition of a vector bundle as a tuple (E, M, pi), which distinguishes the bundle from its base space E.

comment:19 follow-up: Changed 12 months ago by gh-DeRhamSource

I need some help, again. I'm now trying to program local frames and sections and looked up how you managed it for vector fields. Maybe you can shortly explain how you did it? The code is pretty huge and spreaded. Especially, I'm interested in changes from one (coordinate) frame into another for different domains. Thanks! :)

Version 1, edited 12 months ago by gh-DeRhamSource (previous) (next) (diff)

comment:20 in reply to: ↑ 19 Changed 12 months ago by egourgoulhon

Replying to gh-DeRhamSource:

I need some help, again. I'm now trying to program local frames and sections and looked up how you managed it for vector fields. Maybe you can shortly explain how you did it? The code is pretty huge and spreaded. Especially, I'm interested in changes from one (coordinate) frame into another for different domains and the restriction process and display for tensor fields. Thanks! :)

You can find some details of the implementation in this article. Please tell me if this does not cover what you are looking for.

comment:21 follow-up: Changed 12 months ago by gh-DeRhamSource

Thanks, that was very helpful.

I think, one should do the following: Implement arbitrary tensor product sections on one particular vector bundle and apply and adapt the construction as it is done for tensor fields, and then derive tensor fields from this more general construction. Or even better: Implement tensor product sections of mixed vector bundles over the same base space (e.g. \Gamma(E \otimes E^* \otimes \Lambda^k F)).

Unfortunately, I'm running out of time. Moreover, for this endeavour, you need to understand the whole code around tensor fields quite perfectly. I would implement just simple sections for now and apply the compromise we agreed on above. Maybe I can try this generalization after my thesis or one of you is willing to take the effort.

However, simple sections seem enough for the beginning because it is possible to construct aforementioned sections by constructing the corresponding bundles and then defining simple sections on them. Still, imao, the way above is the most flexible and overall convenient.

What do you think?

Last edited 12 months ago by gh-DeRhamSource (previous) (diff)

comment:22 in reply to: ↑ 21 Changed 11 months ago by egourgoulhon

Replying to gh-DeRhamSource:

I would implement just simple sections for now and apply the compromise we agreed on above. Maybe I can try this generalization after my thesis or one of you is willing to take the effort.

However, simple sections seem enough for the beginning because it is possible to construct aforementioned sections by constructing the corresponding bundles and then defining simple sections on them. Still, imao, the way above is the most flexible and overall convenient.

What do you think?

Sorry for the delay in replying. Yes, implementing simple sections as a first stage seems the thing to do at the moment.

comment:23 Changed 11 months ago by git

  • Commit changed from bc1fac17d30a5f60f61e33a4e54aec2d216d64de to 94cdc74abfac7f4f3d03a43121eebbabe95cc8a9

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

c539562intermediate step
a2cd749Intermediate step 2
157c3a0Intermediate step 3
d7d4115Merge remote-tracking branch 'trac/develop' into t/28159/vector_bundles
94cdc74Introducing local frames

comment:24 Changed 11 months ago by git

  • Commit changed from 94cdc74abfac7f4f3d03a43121eebbabe95cc8a9 to c59f443dd7259aca2214639ab0e2f6e1dc4f4151

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

c59f443Minor issues

comment:25 Changed 11 months ago by git

  • Commit changed from c59f443dd7259aca2214639ab0e2f6e1dc4f4151 to 12bfb7937bc3e978d8bec410929633889b1e487a

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

46e081cMain parts finished
12bfb79Framework and doctests mostly finished. Please check!

comment:26 follow-up: Changed 11 months ago by gh-DeRhamSource

The doctests take pretty much effort (especially for the section file). Maybe, I will not finish them on time. But for now, the code is mostly finished. Some things are still to do, but sections seem to work. Please take a look and write me if something is missing or should be done better.

Best regards, Michael

Last edited 11 months ago by gh-DeRhamSource (previous) (diff)

comment:27 Changed 11 months ago by gh-DeRhamSource

  • Dependencies #28189 deleted

comment:28 in reply to: ↑ 26 ; follow-up: Changed 10 months ago by egourgoulhon

Replying to gh-DeRhamSource:

The doctests take pretty much effort (especially for the section file). Maybe, I will not finish them on time. But for now, the code is mostly finished. Some things are still to do, but sections seem to work. Please take a look and write me if something is missing or should be done better.

Sorry for the delay in replying. I gave a look and it looks very good! Please go on.

A few comments/questions:

  • you've deleted the dependency to #28189 while some source files modified in this ticket (those in src/sage/matrix and src/sage/sets) pertain to #28189
  • vector bundles have not been added to the reference manual yet; I guess you have to edit/add some files in src/doc/en/reference/manifolds and check that the documentation builds correcty with
    sage -docbuild reference/manifolds html
    
  • notice that the doctests that output a dictionary, like
    sage: E.changes_of_frame()
    
    in line 105 of trivialization.py, must be marked # random
  • there is a typo in line 38 of trivialization.py:
        `(p, v) \mapsto \pi^{-1}(p)` is a linear isomorphism.  
    
    must be replaced by something like
        `(p, v) \mapsto v` is a linear isomorphism from `\pi^{-1}(p)` to `K^n`.
    
  • another typo, in line 8 of vector_bundle.py:
    - the set `E_p=\pi^{-1}(x)` has the vector space structure of `K^n`,
    
    x has to be replaced by p.
  • the function TopologicalVectorBundle.atlas() returns a shallow copy of self._atlas; why does it not return directly self._atlas ?
  • shouldn't the method TopologicalVectorBundle.total_space() be cached? (possibly using the decorator @cached_method)

comment:29 Changed 10 months ago by gh-DeRhamSource

  • Dependencies set to #28189

comment:30 in reply to: ↑ 28 ; follow-up: Changed 10 months ago by gh-DeRhamSource

Sorry for the delay in replying. I gave a look and it looks very good! Please go on.


Don't worry. :)

A few comments/questions:

  • you've deleted the dependency to #28189 while some source files modified in this ticket (those in src/sage/matrix and src/sage/sets) pertain to #28189


Added again.

  • vector bundles have not been added to the reference manual yet; I guess you have to edit/add some files in src/doc/en/reference/manifolds and check that the documentation builds correcty with
    sage -docbuild reference/manifolds html
    


Comes later, when all the doc is finished. :)

  • notice that the doctests that output a dictionary, like
    sage: E.changes_of_frame()
    
    in line 105 of trivialization.py, must be marked # random


What do you mean?

  • there is a typo in line 38 of trivialization.py:
        `(p, v) \mapsto \pi^{-1}(p)` is a linear isomorphism.  
    
    must be replaced by something like
        `(p, v) \mapsto v` is a linear isomorphism from `\pi^{-1}(p)` to `K^n`.
    
  • another typo, in line 8 of vector_bundle.py:
    - the set `E_p=\pi^{-1}(x)` has the vector space structure of `K^n`,
    
    x has to be replaced by p.


Corrected. The projection compatibility was missing, too.

  • the function TopologicalVectorBundle.atlas() returns a shallow copy of self._atlas; why does it not return directly self._atlas ?


I just borrowed the same code lines from manifold.py. I guess, this is for convenience: One should not allow to alter the list when getting the atlas through this method.

  • shouldn't the method TopologicalVectorBundle.total_space() be cached? (possibly using the decorator @cached_method)


For now, yes. But later on, when the computation of the atlas is implemented, one should update the manifold's atlas, maybe using this method.

Best, Michael

comment:31 Changed 10 months ago by git

  • Commit changed from 12bfb7937bc3e978d8bec410929633889b1e487a to 901df657fef7850b041b7a98714fccb2e9b7b669

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

959e152Merge branch 'develop' into t/28159/vector_bundles
901df65Doctest and minor changes

comment:32 in reply to: ↑ 30 ; follow-up: Changed 10 months ago by egourgoulhon

Replying to gh-DeRhamSource:

Comes later, when all the doc is finished. :)

I will ;-)

  • notice that the doctests that output a dictionary, like
    sage: E.changes_of_frame()
    
    in line 105 of trivialization.py, must be marked # random


What do you mean?

When printing a dictionary, the order of elements is random. For instance it depends on the computer. So a doctest that involves a directory output may fail on a different computer. The marker # random ensures that the doctest is skipped (see http://doc.sagemath.org/html/en/developer/coding_basics.html#special-markup-to-influence-doctests).

  • the function TopologicalVectorBundle.atlas() returns a shallow copy of self._atlas; why does it not return directly self._atlas ?


I just borrowed the same code lines from manifold.py.

Ah yes, you are right: that's already there for manifolds.

I guess, this is for convenience: One should not allow to alter the list when getting the atlas through this method.

Yes this could be the reason. Travis, do remember why such a choice was made? On the other side, things are not very consistent here, since frames() returns directly self._frames and we have the same for many other manifold methods.

  • shouldn't the method TopologicalVectorBundle.total_space() be cached? (possibly using the decorator @cached_method)


For now, yes. But later on, when the computation of the atlas is implemented, one should update the manifold's atlas, maybe using this method.

OK, I see.

comment:33 in reply to: ↑ 32 ; follow-up: Changed 10 months ago by tscrim

Replying to egourgoulhon:

Replying to gh-DeRhamSource:

  • the function TopologicalVectorBundle.atlas() returns a shallow copy of self._atlas; why does it not return directly self._atlas ?


I just borrowed the same code lines from manifold.py.

Ah yes, you are right: that's already there for manifolds.

I guess, this is for convenience: One should not allow to alter the list when getting the atlas through this method.

Yes this could be the reason. Travis, do remember why such a choice was made? On the other side, things are not very consistent here, since frames() returns directly self._frames and we have the same for many other manifold methods.

Probably all of them should return copies of the lists because it better protects the data. It is really easy to access stuff and then unintentionally change it, which could then throw your entire system out of balance. Most likely I/we just didn't notice or think about it when doing the initial code.

comment:34 in reply to: ↑ 33 ; follow-up: Changed 10 months ago by egourgoulhon

Replying to tscrim:

Probably all of them should return copies of the lists because it better protects the data. It is really easy to access stuff and then unintentionally change it, which could then throw your entire system out of balance. Most likely I/we just didn't notice or think about it when doing the initial code.

My concern here would be about efficiency. We might have quite often loops like

for chart in M.atlas():
    ...

This implies that a list is created each time such a loop occurs. But maybe the CPU cost is negligible...

comment:35 in reply to: ↑ 34 Changed 10 months ago by tscrim

Replying to egourgoulhon:

Replying to tscrim:

Probably all of them should return copies of the lists because it better protects the data. It is really easy to access stuff and then unintentionally change it, which could then throw your entire system out of balance. Most likely I/we just didn't notice or think about it when doing the initial code.

My concern here would be about efficiency. We might have quite often loops like

for chart in M.atlas():
    ...

This implies that a list is created each time such a loop occurs. But maybe the CPU cost is negligible...

That would be my guess since so much time is spent in doing the computations of the manifold itself. (Python heavily optimizes list manipulations I think too.) Plus, since it is internal, it is much more okay to break encapsulation and just directly call the attribute when you know it is safe (or add a copy flag to the accessor methods).

comment:36 Changed 10 months ago by git

  • Commit changed from 901df657fef7850b041b7a98714fccb2e9b7b669 to 9011e3d645f9ccbe8df35cfa8883f128312d2d9d

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

9011e3dCopy lists and frame restriction

comment:37 in reply to: ↑ description ; follow-up: Changed 10 months ago by gh-DeRhamSource

There was an issue with the restrictions of (vector) frames. Namely, the list of sub- and superframes was not complete and depended on some strange random order. Here is an example:

sage: M = Manifold(2, 'M')
sage: U = M.open_subset('U'); V = M.open_subset('V'); W = U.intersection(V, name='W')
sage: e = M.vector_frame('e')
sage: eV = e.restrict(V); eU = e.restrict(U); eW = e.restrict(W)
sage: eW._superframes
{Vector frame (W, (e_0,e_1)),
 Vector frame (M, (e_0,e_1)),
 Vector frame (V, (e_0,e_1))}
sage: eU._subframes
{Vector frame (U, (e_0,e_1))}
sage: eV._subframes
{Vector frame (W, (e_0,e_1)), Vector frame (V, (e_0,e_1))}

I fixed it by avoiding any recursion and expanding the tree of subframes (and superframes) starting from the reference frame. Please check.

Last edited 10 months ago by gh-DeRhamSource (previous) (diff)

comment:38 in reply to: ↑ 37 Changed 10 months ago by egourgoulhon

Replying to gh-DeRhamSource:

There was an issue with the restrictions of (vector) frames. Namely, the list of sub- and superframes was not complete and depended on some strange random order.

I fixed it by avoiding any recursion and expanding the tree of subframes (and superframes) starting from the reference frame. Please check.

Looks good to me. Thanks for the fix!

comment:39 Changed 10 months ago by git

  • Commit changed from 9011e3d645f9ccbe8df35cfa8883f128312d2d9d to 9600762f72d9a49e408b18aeda17a14ecfad1327

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

17657ecDoctest added
ef03c38Merge branch 'develop' into t/28159/vector_bundles
3b0c7a7minor doctest changes
9600762inverses of automorphisms without SR inforcement

comment:40 Changed 10 months ago by gh-DeRhamSource

I declared ScalarFieldAlgebra as a field so that matrix computations like inverses and determinants make no problems. When ticket #28189 is merged (hopefully in 9b0), this can be undone. But for now, this ticket does not depend on #28189 anymore.

Since this solves some problems in the preexisting code, I removed the SR enforcement in differentiable/automorphismfield.py for more compatibility.

Do you agree?

Last edited 10 months ago by gh-DeRhamSource (previous) (diff)

comment:41 Changed 10 months ago by gh-DeRhamSource

  • Dependencies #28189 deleted

comment:42 Changed 10 months ago by git

  • Commit changed from 9600762f72d9a49e408b18aeda17a14ecfad1327 to 033960f3e3bc2f46cea69ce5ae4e5044718bdc9c

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2e38d47inverses of automorphisms without SR enforcement
7363b9f28189: prefer R in Fields() rather than R.is_field()
033960f28189: fix doctest in judson-abstract-algebra book

comment:43 Changed 10 months ago by gh-DeRhamSource

A merge of #28189 for 9b0 seems imminent. Since this ticket will be merged earliest in 9b0, I added the dependencies again. Imao, it makes things easier and less complicated, especially for characteristic classes.

I am sorry for potential inconveniences.

comment:44 Changed 10 months ago by gh-DeRhamSource

  • Dependencies set to #28189

comment:45 Changed 10 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:46 Changed 10 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:47 Changed 10 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:48 Changed 10 months ago by gh-DeRhamSource

  • Description modified (diff)
  • Milestone changed from sage-8.9 to sage-9.0

comment:49 Changed 10 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:50 Changed 10 months ago by git

  • Commit changed from 033960f3e3bc2f46cea69ce5ae4e5044718bdc9c to 69d05dad4177d25333b85de782ebecea1615baa4

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

5d92db328189: prefer R in Fields() rather than R.is_field()
7767b3028189: fix doctest in judson-abstract-algebra book
4981d44fix accidental merge with #28402
fc18183Merge new commit of branch 't/28189/28189' into t/28159/vector_bundles
69d05daDoctests for section methods finished

comment:51 Changed 9 months ago by git

  • Commit changed from 69d05dad4177d25333b85de782ebecea1615baa4 to a737240ae5f5b6c07bf969e220062135e8955675

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

a737240Doctests finished + default frame + typos

comment:52 Changed 9 months ago by gh-DeRhamSource

So, I guess, all pieces of the puzzles are fit together now. Please take a look, give me a feedback and tell me what is missing.

I have some difficulties to interpret the docbuild errors. Can someone help? Is there a documentation?

It seems, we are almost done. :)

comment:53 Changed 9 months ago by gh-DeRhamSource

By the way: How should the authorship be handled? I copied most of the files and just adapted them. So, the other authors should be mentioned.

Last edited 9 months ago by gh-DeRhamSource (previous) (diff)

comment:54 Changed 9 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:55 Changed 9 months ago by git

  • Commit changed from a737240ae5f5b6c07bf969e220062135e8955675 to f4ef538c162039ea301de1626791405a33bbb65c

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

356147cMerge branch 'develop' into t/28159/vector_bundles
f4ef538Differentiability of total space

comment:56 Changed 9 months ago by gh-DeRhamSource

  • Reviewers set to Eric Gourgoulhon, Travis Scrimshaw
  • Status changed from new to needs_review

comment:57 follow-up: Changed 9 months ago by tscrim

Docbuild errors are notoriously quite difficult. Unfortunately it usually just means going over the file carefully with some general hint from the error message about what you should be looking for. I can also go through it if you haven't been able to find what is going wrong.

As for authorship, I would say just listing yourself as the author because you did write the code, even if it is adapted from others. I don't think they should (or need to) have a copyright. Although it doesn't hurt to say it was based off another file, but I don't have a stake in this. Eric is best to comment on this.

Are there things you can put as methods in the category of vector bundles? At least I think there should be an abstract method for the base space, and maybe also the projection map (i.e., things in this category should have a common way to give the defining properties).

This code doesn't do anything:

        if update_atlas:
            # TODO: if self._atlas not empty, introduce charts
            pass

I would change Moebius to Möbius in the documentation (also make sure to add the utf8 encoding # -*- coding: utf-8 -*- to the first line of such files).

-        desc = "Module "
-        desc += self._name + " "
-        desc += "of sections "
-        desc += "on the {} ".format(self._domain)
-        desc += "with values in the " + self._vbundle.base_field_type() + " "
-        desc += "vector bundle " + self._vbundle._name + " "
-        desc += "of rank {}".format(self._vbundle.rank())
+        desc = "Module {} of sections on the {} with values in the {} vector bundle of rank {}"
+        desc = desc.format(self._name, self._domain, self._vbundle.base_field_type(), self._vbundle.rank())
-Sets the default local frame on ``self``.
+Set the default local frame on ``self``.

I will probably have more changes, but I don't have time right now to look in full detail.

comment:58 Changed 9 months ago by git

  • Commit changed from f4ef538c162039ea301de1626791405a33bbb65c to f484b7702cac56ad495c22735e15876b8232f8bf

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

f484b77Docbuild errors fixed + Minor fixes

comment:59 Changed 9 months ago by git

  • Commit changed from f484b7702cac56ad495c22735e15876b8232f8bf to 1735541337db3f446641782dc6ce719bd417e3b5

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

1735541Very small fix

comment:60 in reply to: ↑ 57 ; follow-up: Changed 9 months ago by gh-DeRhamSource

Thanks for your feedback!

Replying to tscrim:

Docbuild errors are notoriously quite difficult. Unfortunately it usually just means going over the file carefully with some general hint from the error message about what you should be looking for. I can also go through it if you haven't been able to find what is going wrong.

I guess, I have eliminated all doctest errors caused by these files. Please overlook this.

As for authorship, I would say just listing yourself as the author because you did write the code, even if it is adapted from others. I don't think they should (or need to) have a copyright. Although it doesn't hurt to say it was based off another file, but I don't have a stake in this. Eric is best to comment on this.

Done. I kept Eric in the copyrights. Is that appropriate?

This code doesn't do anything:

        if update_atlas:
            # TODO: if self._atlas not empty, introduce charts
            pass

I would change Moebius to Möbius in the documentation (also make sure to add the utf8 encoding # -*- coding: utf-8 -*- to the first line of such files).

-        desc = "Module "
-        desc += self._name + " "
-        desc += "of sections "
-        desc += "on the {} ".format(self._domain)
-        desc += "with values in the " + self._vbundle.base_field_type() + " "
-        desc += "vector bundle " + self._vbundle._name + " "
-        desc += "of rank {}".format(self._vbundle.rank())
+        desc = "Module {} of sections on the {} with values in the {} vector bundle of rank {}"
+        desc = desc.format(self._name, self._domain, self._vbundle.base_field_type(), self._vbundle.rank())
-Sets the default local frame on ``self``.
+Set the default local frame on ``self``.

Done!

Are there things you can put as methods in the category of vector bundles? At least I think there should be an abstract method for the base space, and maybe also the projection map (i.e., things in this category should have a common way to give the defining properties).

What do you mean? :/

comment:61 Changed 9 months ago by git

  • Commit changed from 1735541337db3f446641782dc6ce719bd417e3b5 to 54237ef4815198c80a0f95e949a8debaa7aafd2f

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

54237efPyflakes and Coverage

comment:62 Changed 9 months ago by git

  • Commit changed from 54237ef4815198c80a0f95e949a8debaa7aafd2f to 67cf8df97fe082f277fb54b1d3a0626f9617ac7f

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

67cf8dfMerge branch 'develop' into t/28159/vector_bundles

comment:63 in reply to: ↑ 60 ; follow-up: Changed 9 months ago by egourgoulhon

Replying to gh-DeRhamSource:

Replying to tscrim:

Docbuild errors are notoriously quite difficult. Unfortunately it usually just means going over the file carefully with some general hint from the error message about what you should be looking for. I can also go through it if you haven't been able to find what is going wrong.

I guess, I have eliminated all doctest errors caused by these files. Please overlook this.

Thanks for this painful work. It is true that sphinx messages are not very informative. In particular the line numbers do not refer to the start of the file but to the start of the docstring. For this reason, one should not hesitate to build the doc often, possibly after each modification of a doctring. For this, do not run make doc, which takes ages (*), use instead sage -docbuild reference/manifolds html.

(*) hopefully there is a ticket about this (I don't remember which one at the moment).

As for authorship, I would say just listing yourself as the author because you did write the code, even if it is adapted from others. I don't think they should (or need to) have a copyright. Although it doesn't hurt to say it was based off another file, but I don't have a stake in this. Eric is best to comment on this.

Done. I kept Eric in the copyrights. Is that appropriate?

I think you can remove my name from the copyright, leaving just yours.

I'll have a look at the code soon.


New commits:

54237efPyflakes and Coverage
67cf8dfMerge branch 'develop' into t/28159/vector_bundles

comment:64 in reply to: ↑ 63 Changed 9 months ago by gh-DeRhamSource

Thank you! I fixed pyflakes and coverage errors. I'm looking forward to your review! :)

comment:65 Changed 9 months ago by git

  • Commit changed from 67cf8df97fe082f277fb54b1d3a0626f9617ac7f to 9832d02285f25bdb95a11efcb6e705938a36c3f5

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

9832d02Pyflakes and Coverage

comment:66 Changed 9 months ago by egourgoulhon

  • Status changed from needs_review to needs_work

Thanks for completing the documentation.

With python3 Sage 9.0.beta0, I've got the following errors:

----------------------------------------------------------------------
sage -t --long --warn-long 53.2 src/sage/categories/vector_bundles.py  # 2 doctests failed
sage -t --long --warn-long 53.2 src/sage/manifolds/section.py  # 2 doctests failed
sage -t --long --warn-long 53.2 src/sage/manifolds/chart_func.py  # 1 doctest failed
sage -t --long --warn-long 53.2 src/sage/manifolds/section_module.py  # 10 doctests failed
sage -t --long --warn-long 53.2 src/sage/manifolds/vector_bundle.py  # 1 doctest failed
----------------------------------------------------------------------

They appear on the patchbot report too.

Besides, there is a warning when building the documentation:

[manifolds] <unknown>:1956: DeprecationWarning: invalid escape sequence \c
Last edited 9 months ago by egourgoulhon (previous) (diff)

comment:67 Changed 9 months ago by egourgoulhon

There is also some wrong indentation in the docstring of class TopologicalVectorBundle, starting from line 85 of src/sage/manifolds/vector_bundle.py. This alters the output of the html documentation.

comment:68 Changed 9 months ago by git

  • Commit changed from 9832d02285f25bdb95a11efcb6e705938a36c3f5 to 73e103efc0857f4b37b95ccc13c2edf9ce93f551

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

73e103eDoctest errors fixed

comment:69 Changed 9 months ago by gh-DeRhamSource

This should be fixed now.

comment:70 Changed 9 months ago by gh-DeRhamSource

  • Status changed from needs_work to needs_review

comment:71 follow-up: Changed 9 months ago by gh-DeRhamSource

The error in src/sage/manifolds/chart_func.py should not belong to this ticket. I fixed it anyway.

comment:72 in reply to: ↑ 71 Changed 9 months ago by egourgoulhon

Replying to gh-DeRhamSource:

The error in src/sage/manifolds/chart_func.py should not belong to this ticket.

Yes it does: there is no such error in Sage 9.0.beta0. Anyway, thanks for the fixes.

comment:73 Changed 9 months ago by egourgoulhon

  • Status changed from needs_review to needs_work

I still get some errors:

----------------------------------------------------------------------
sage -t --long --warn-long 54.2 src/sage/manifolds/section.py  # 1 doctest failed
sage -t --long --warn-long 54.2 src/sage/manifolds/chart_func.py  # 2 doctests failed
sage -t --long --warn-long 54.2 src/sage/manifolds/vector_bundle.py  # 1 doctest failed
sage -t --long --warn-long 54.2 src/sage/manifolds/section_module.py  # 1 doctest failed
----------------------------------------------------------------------

comment:74 follow-up: Changed 9 months ago by gh-DeRhamSource

That's pretty strange:

  • For section.py I get no errors. What is your error message?
  • The two chart_func.py errors are not related to this ticket. However, it seems a #random mark is needed here.

The other issues get fixed soon. I'll change the status.

comment:75 Changed 9 months ago by git

  • Commit changed from 73e103efc0857f4b37b95ccc13c2edf9ce93f551 to 15ca7ea8d2bf9f20461fedf21a9ce1c27afc1eef

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

15ca7eaSectionModule representation + doctest fixes

comment:76 Changed 9 months ago by gh-DeRhamSource

  • Status changed from needs_work to needs_review

comment:77 in reply to: ↑ 74 Changed 9 months ago by egourgoulhon

Replying to gh-DeRhamSource:

That's pretty strange:

  • For section.py I get no errors. What is your error message?

Here it is:

sage -t --long src/sage/manifolds/section.py
**********************************************************************
File "src/sage/manifolds/section.py", line 305, in sage.manifolds.section.Section._latex_
Failed example:
    omega = E.section(name='\omega')
Expected nothing
Got:
    doctest:warning
...
    DeprecationWarning: invalid escape sequence \o
**********************************************************************
1 item had failures:
   1 of   6 in sage.manifolds.section.Section._latex_
    [735 tests, 1 failure, 29.78 s]

This is because the raw string marker r is missing in front of '\omega': the line 305 should read

   sage: omega = E.section(name=r'\omega')

Probably the reason why you don't get this error is that you are using Python 2 version of Sage, which is less strict in this respect (however, this is still an error, since the LaTeX output will not be correct). On general ground, I would advise you to use Python 3 version of Sage for development, since Sage 9.0 will be Python 3 based. Installing Python 3 Sage is pretty easy:

git clone https://github.com/sagemath/sage.git
cd sage
git checkout develop
git pull
make configure
./configure --with-python=3
MAKE="make -j8" make

comment:78 follow-up: Changed 9 months ago by gh-DeRhamSource

  • Status changed from needs_review to needs_work

Oh yeah, thank you! :) I am going to try a new test run after the python3 compilation. Till then, I keep this ticket on needs_work.

comment:79 in reply to: ↑ 78 Changed 9 months ago by egourgoulhon

Replying to gh-DeRhamSource:

Oh yeah, thank you! :) I am going to try a new test run after the python3 compilation. Till then, I keep this ticket on needs_work.

OK. On my (Python 3) side, the doctest error in section.py is the last remaining one. However, there are various errors in the html output of the documentation. For instance, in the section "Trivializations", some links are wrong (class names not clickable or expressions as :class:`sage.manifolds.chart_func.ChartFunction appearing in the output) and some :: are missing before doctest blocs. Could you please fix those?

comment:80 Changed 9 months ago by git

  • Commit changed from 15ca7ea8d2bf9f20461fedf21a9ce1c27afc1eef to 3a7d29be713fa75cd3be0f5a14789db12635b879

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

3a7d29bMinor doctest fixes

comment:81 Changed 9 months ago by gh-DeRhamSource

  • Status changed from needs_work to needs_review

Changed. Can you please check the docbuild for me?

comment:82 Changed 9 months ago by git

  • Commit changed from 3a7d29be713fa75cd3be0f5a14789db12635b879 to 116cbfa97a3acf0f53d252e1cea5dfa4f085f4b3

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

116cbfaSectionMOdule added to documentation

comment:83 Changed 9 months ago by git

  • Commit changed from 116cbfa97a3acf0f53d252e1cea5dfa4f085f4b3 to 078b13fd1729eed24521b4561947d8de4a92f2b9

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

078b13fSection modules added to documentation

comment:84 Changed 9 months ago by egourgoulhon

  • Branch changed from u/gh-DeRhamSource/vector_bundles to public/manifolds/vector_bundles
  • Commit changed from 078b13fd1729eed24521b4561947d8de4a92f2b9 to 12a86a4feef85048f97383363fd004f8d1961c44

New commits:

12a86a4Some fixes in the documentation of vector bundles

comment:85 Changed 9 months ago by egourgoulhon

I've gone through the documentation and have introduced some fixes in the above commit (comment:84). In particular, I've added an entry point for the category of vector bundles, as well as for fibers and fiber elements. I've also fixed hyperlinks, quote issues, missing :: and the invalid escape sequence mentioned in comment:66. You can see the full list of changes here. Do you agree with them?

Last edited 9 months ago by egourgoulhon (previous) (diff)

comment:86 follow-up: Changed 9 months ago by gh-DeRhamSource

Thank you so much for overlooking my code and your effort! There was still quite a lot to do.

There's just one thing in differentiable/vector_bundle.py around line 389 in the class TensorBundle:

"OUPUT" -> "OUTPUT"

Apart from that, I agree. :)

comment:87 Changed 9 months ago by git

  • Commit changed from 12a86a4feef85048f97383363fd004f8d1961c44 to b8eba570bccc8db1553bde445342ac716539fa33

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

b8eba57Slight changes in the documentation and code of vector bundles

comment:88 in reply to: ↑ 86 ; follow-up: Changed 9 months ago by egourgoulhon

Replying to gh-DeRhamSource:

There's just one thing in differentiable/vector_bundle.py around line 389 in the class TensorBundle:

"OUPUT" -> "OUTPUT"

Corrected in the above commit. I've also introduced minor changes to improve the documentation (I hope).

I have also performed some changes in the code, replacing self._dest_map by self._dest_map.restrict(domain) in DifferentiableVectorBundle.section() and DifferentiableVectorBundle.local_frame(). Besides, I have made the latter return NotImplementedError when the tensor bundle is not the tangent bundle, because in such a case, a VectorFrame should not be returned. Do you agree? Shall we maintain the alias vector_frame for local_frame?

I have some question regarding the methods atlas(), change_of_frame(), changes_of_frame, coframes(), frames(), set_change_of_frame(), transition(), transitions() of class DifferentiableVectorBundle: are these methods really necessary? As currently implemented, they return only features of the base manifold. In particular, none of them is using the tensor type (k, l) of self.

comment:89 in reply to: ↑ 88 ; follow-up: Changed 9 months ago by gh-DeRhamSource

Replying to egourgoulhon:

I have also performed some changes in the code, replacing self._dest_map by self._dest_map.restrict(domain) in DifferentiableVectorBundle.section() and DifferentiableVectorBundle.local_frame().

Thank you! :)

Besides, I have made the latter return NotImplementedError when the tensor bundle is not the tangent bundle, because in such a case, a VectorFrame should not be returned. Do you agree? Shall we maintain the alias vector_frame for local_frame?

In this case, I do not agree. Altough actual frames of general tensor bundles are not implemented, vector frames induce them canonically. In the whole preexisting code, a vector frame is used for declaration and output of tensor fields. So, in my opinion, we should keep it this way (at least until general tensor product bundles are implemented). However, a corresponding remark in the documentation might be a good idea.

I have some question regarding the methods atlas(), change_of_frame(), changes_of_frame, coframes(), frames(), set_change_of_frame(), transition(), transitions() of class DifferentiableVectorBundle: are these methods really necessary? As currently implemented, they return only features of the base manifold. In particular, none of them is using the tensor type (k, l) of self.

The class TopologicalVectorBundle consists of these methods and TensorBundle inherits from it, so it must carry out these methods as well. But since all its features are already regulated through the manifold's framework (tensor bundles are induced by differentiable charts and their transitions), the vector bundle implementation is obsolete in this case and the methods must be overwritten. Likewise, a remark in the documentation might be useful, here. Do you have a better solution in mind?

Another thing: In a next step, I want to inherit TensorField from Section (and perhaps VectorFrame from LocalFrame) in order to minimize code redundancies. Keeping in mind, when my modifications in #28519 are finished, Section and TrivialSection should be adapted anyways.

Last edited 9 months ago by gh-DeRhamSource (previous) (diff)

comment:90 in reply to: ↑ 89 ; follow-up: Changed 9 months ago by egourgoulhon

Replying to gh-DeRhamSource:

Besides, I have made the latter return NotImplementedError when the tensor bundle is not the tangent bundle, because in such a case, a VectorFrame should not be returned. Do you agree? Shall we maintain the alias vector_frame for local_frame?

In this case, I do not agree. Altough actual frames of general tensor bundles are not implemented, vector frames induce them canonically. In the whole preexisting code, a vector frame is used for declaration and output of tensor fields. So, in my opinion, we should keep it this way (at least until general tensor product bundles are implemented).

OK, I see.

However, a corresponding remark in the documentation might be a good idea.

Yes, there should be a warning that the returned object is actually not a bundle local frame, although it canonically determines it, as you say. The same thing holds for the method trivialization(), which returns a chart on the base manifold and actually not a trivialization in the sense of a vector bundle.

I have some question regarding the methods atlas(), change_of_frame(), changes_of_frame, coframes(), frames(), set_change_of_frame(), transition(), transitions() of class DifferentiableVectorBundle: are these methods really necessary? As currently implemented, they return only features of the base manifold. In particular, none of them is using the tensor type (k, l) of self.

The class TopologicalVectorBundle consists of these methods and TensorBundle inherits from it, so it must carry out these methods as well. But since all its features are already regulated through the manifold's framework (tensor bundles are induced by differentiable charts and their transitions), the vector bundle implementation is obsolete in this case and the methods must be overwritten.

OK.

Likewise, a remark in the documentation might be useful, here.

Indeed.

Do you have a better solution in mind?

Not at the moment.

I let you revert the NotImplementedError and add the relevent warnings.

Another thing: In a next step, I want to inherit TensorField from Section (and perhaps VectorFrame from LocalFrame) in order to minimize code redundancies.

Yes, I guess by "next step", you mean "in another ticket".

comment:91 Changed 9 months ago by git

  • Commit changed from b8eba570bccc8db1553bde445342ac716539fa33 to 770e6999c8e3a0cb8c2f7264dd736628634ad606

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

770e699More detailed documentation

comment:92 in reply to: ↑ 90 Changed 9 months ago by gh-DeRhamSource

Replying to egourgoulhon:

I let you revert the NotImplementedError and add the relevent warnings.

Done.

Would you take a short look for typos etc?

Also, I've changed the name of the base space in the documentation of TensorBundle to N instead of U.

Do you agree so far? Any more suggestions?

Yes, I guess by "next step", you mean "in another ticket".

Of course. :)

Last edited 9 months ago by gh-DeRhamSource (previous) (diff)

comment:93 Changed 9 months ago by gh-DeRhamSource

Ah, I've also fixed the filter algorithms for the frames. (Unfortunately, I missed it in the commit message.)

Last edited 9 months ago by gh-DeRhamSource (previous) (diff)

comment:94 Changed 9 months ago by gh-DeRhamSource

There are some more things I noticed. I'm going to take of it later this day.

comment:95 Changed 9 months ago by git

  • Commit changed from 770e6999c8e3a0cb8c2f7264dd736628634ad606 to 22e6666e641777f26eb165ba4623f6c28fa8e70b

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

22e6666Documentation improvements, i.e. frames along maps

comment:96 follow-up: Changed 9 months ago by gh-DeRhamSource

Please take a look. I made many modifications and fixes in the documentation.

comment:97 Changed 9 months ago by git

  • Commit changed from 22e6666e641777f26eb165ba4623f6c28fa8e70b to 9ecd22a1f537f5392dbad999612c2d433dc2fabc

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

e2c535dMerge branch 'public/manifolds/vector_bundles' of git://trac.sagemath.org/sage into Sage 9.0.beta1
9ecd22aMinor fix in the documentation of vector bundles

comment:98 in reply to: ↑ 96 Changed 9 months ago by egourgoulhon

Replying to gh-DeRhamSource:

Please take a look. I made many modifications and fixes in the documentation.

Thanks for the improvements in the documentation! I've just pushed a minor fix in some :meth: hyperlink. I think the ticket is ready to go.

comment:99 Changed 9 months ago by gh-DeRhamSource

Just wait a moment. I have still some fixes, coming soon.

comment:100 Changed 9 months ago by git

  • Commit changed from 9ecd22a1f537f5392dbad999612c2d433dc2fabc to be0b7d0ff8b2ea739b97bc339e68a2403b74f4df

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

be0b7d0further methods added + 'local_frame' invokes vector field module's method + doc improvements

comment:101 Changed 9 months ago by gh-DeRhamSource

There we go. I had a short discussion with a collegue of mine in which I noticed some more things. But that should be it. Please take a short look.

comment:102 Changed 9 months ago by git

  • Commit changed from be0b7d0ff8b2ea739b97bc339e68a2403b74f4df to f9810b77472ee3c16f4843aca27239e149e48ffa

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

f9810b7unique_tag documentation added

comment:103 Changed 9 months ago by egourgoulhon

  • Status changed from needs_review to needs_work

I've just discovered that, with this ticket branch, the AdS notebook stalls at cell 92, which introduces the transition map from conformal coordinates to Poincaré coordinates. Running the notebook with Sage 9.0.beta1 shows no problem. So the issue is caused by some piece of code in this ticket.

Cell 92 seems to run forever, or at least to take a very long time. After 15 min or so, I halted it by Kernel -> Interrupt in the Jupyter menu. It appeared then that the code was stalled in line 1147 of src/sage/manifolds/differentiable/chart.py:

   ch_basis_inv = ch_basis.inverse()

So I strongly suspect the issue to be due to the change in the computation of inverse automorphism fields introduced in this ticket.

comment:104 Changed 9 months ago by gh-DeRhamSource

This could be related to #28189, especially to the bug reported afterwards which has been partially fixed in #28570 (was this particular ticket merged in your testing?).

If there are no other options left, we can readd the SR enforcement for now. However, I suggest to construct a minimal example where this particular issue fails and report the error, since it seems to be a bigger problem deep in framework of matrices.

Last edited 9 months ago by gh-DeRhamSource (previous) (diff)

comment:105 follow-up: Changed 9 months ago by gh-DeRhamSource

Wait. Why does the program compute the inverse anyway? This should not happen automatically, right? At least, in the next cell the inverse is set manually.

Last edited 9 months ago by gh-DeRhamSource (previous) (diff)

comment:106 in reply to: ↑ 105 Changed 9 months ago by egourgoulhon

Replying to gh-DeRhamSource:

Wait. Why does the program compute the inverse anyway? This should not happen automatically, right? At least, in the next cell the inverse is set manually.

It computes the inverse of the Jacobian matrix, not the inverse of the transition map.

comment:107 Changed 9 months ago by gh-DeRhamSource

Okay, I started some debugging process. And apparently, this has nothing to do with the aforementioned issue.

Since the algebra of scalar fields is not a field, the inversion algorithm is division free using adjugate matrices. I guess, this takes much more time than sophisticated algorithms on fields. My suggestion is to claim (at least for sage) that this algebra is a field (like it is done for the symbolic ring). I already started some testing and the computation time shrunk enormously. I've committed the changes.

Last edited 9 months ago by gh-DeRhamSource (previous) (diff)

comment:108 Changed 9 months ago by git

  • Commit changed from f9810b77472ee3c16f4843aca27239e149e48ffa to aa7dbaf6055abab69c1cb43fb3ef709b9715b7ae

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

aa7dbaf'is_field' method added returning 'True'

comment:109 Changed 9 months ago by gh-DeRhamSource

  • Status changed from needs_work to needs_review

comment:110 Changed 9 months ago by egourgoulhon

Just to tell that I am mostly AFK these days. I'll have a look at the ticket on Friday.

comment:111 follow-up: Changed 9 months ago by egourgoulhon

  • Status changed from needs_review to needs_work

I don't agree with the proposed fix for it goes against mathematical correctness: the method is_field() of ScalarFieldAlgebra shall not return True, the algebra of scalar fields being not a field (in particular, it is not an integral domain).

I would propose that you revert to the original version of AutomorphismFieldParal.__invert__() and open a new ticket to discus the removal of SR enforcement in the inversion of automorphism fields. After all, this is not directly connected to vector bundles and therefore does not belong to the current ticket.

comment:112 in reply to: ↑ 111 ; follow-up: Changed 9 months ago by gh-DeRhamSource

Replying to egourgoulhon:

I don't agree with the proposed fix for it goes against mathematical correctness: the method is_field() of ScalarFieldAlgebra shall not return True, the algebra of scalar fields being not a field (in particular, it is not an integral domain).

Yeah, I feel not comfortable with this solution as well.

I would propose that you revert to the original version of AutomorphismFieldParal.__invert__() and open a new ticket to discus the removal of SR enforcement in the inversion of automorphism fields. After all, this is not directly connected to vector bundles and therefore does not belong to the current ticket.

This is not entirely true. If the vector bundle is similarly complicated as the example in the notebook, it runs into the same problem.

However, forcing sage to use the inversion algorithm for fields would be very nice!

I will open a ticket on that and suggest this in the sage-devel group. Meanwhile, I undo the SR enforcement at least for tensor fields.

comment:113 in reply to: ↑ 112 Changed 9 months ago by egourgoulhon

Replying to gh-DeRhamSource:

Replying to egourgoulhon:

I don't agree with the proposed fix for it goes against mathematical correctness: the method is_field() of ScalarFieldAlgebra shall not return True, the algebra of scalar fields being not a field (in particular, it is not an integral domain).

Yeah, I feel not comfortable with this solution as well.

Besides, it generated a lot of doctest errors as you can check in the patchbot report.

I will open a ticket on that and suggest this in the sage-devel group. Meanwhile, I undo the SR enforcement at least for tensor fields.

OK, very good.

comment:114 Changed 9 months ago by git

  • Commit changed from aa7dbaf6055abab69c1cb43fb3ef709b9715b7ae to 39104657254e432c49bef596337fbae291ca5f77

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

4949f55Revert "'is_field' method added returning 'True'"
3910465SR enforcement removal undone

comment:115 Changed 9 months ago by gh-DeRhamSource

Committed. Please check, again.

comment:116 Changed 9 months ago by gh-DeRhamSource

  • Status changed from needs_work to needs_review

comment:117 follow-up: Changed 9 months ago by egourgoulhon

Again, I don't like so much the methods is_unit() added to the classes ChartFunction and ScalarField. They don't reflect fully the mathematical meaning of being a unit element of a ring. Do you really need these in this ticket? (I don't see clearly the connection with vector bundles).

Side note: self.is_zero(), as invoked in ChartFunction.is_unit(), can be very time consuming for large symbolic expressions, since it is trying to simplify the expression to zero. In this respect, the use of is_trivial_zero() in ScalarField.is_unit() is better.

comment:118 Changed 9 months ago by egourgoulhon

Btw, thanks for having moved the treatment of autormorphism field inverses to #28629.

comment:119 in reply to: ↑ 117 ; follow-up: Changed 9 months ago by gh-DeRhamSource

Replying to egourgoulhon:

Again, I don't like so much the methods is_unit() added to the classes ChartFunction and ScalarField. They don't reflect fully the mathematical meaning of being a unit element of a ring. Do you really need these in this ticket? (I don't see clearly the connection with vector bundles).

I'm not quite sure, but I think this method is mandatory when using the division free algorithm. And the division free algorithm is invoked when changes of frames are established. In general, for vector bundles, frames may be distributed over serveral chart domains, for which the SR enforcement method fails. So yes, this is related to this ticket. I'm not completely satisfied with this either. As soon as the issue in ticket #28629 is solved, this snippet can be deleted again. Should I add a comment in the code about this?

Side note: self.is_zero(), as invoked in ChartFunction.is_unit(), can be very time consuming for large symbolic expressions, since it is trying to simplify the expression to zero. In this respect, the use of is_trivial_zero() in ScalarField.is_unit() is better.

I agree, this is certainly better.

Last edited 9 months ago by gh-DeRhamSource (previous) (diff)

comment:120 Changed 9 months ago by gh-DeRhamSource

Furthermore, I would add a comment in the corresponding ticket, so that this code snippet gets not lost or forgotten.

What do you say?

comment:121 Changed 9 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:122 Changed 9 months ago by git

  • Commit changed from 39104657254e432c49bef596337fbae291ca5f77 to 279f4283bd79bf0a964d971c8aed4d006e174813

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

8d7086a'is_unit' modified
279f428Typo in doc

comment:123 Changed 9 months ago by git

  • Commit changed from 279f4283bd79bf0a964d971c8aed4d006e174813 to 4ae4ca37508374ef9074cdbf9c650bb0d6ae8211

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

4ae4ca3default frame added to vector bundles + doctree modified

comment:124 follow-up: Changed 9 months ago by gh-DeRhamSource

Since default frames are somehow obligatoric for further use, I added them. Please check. Sorry for that. But I think, this modification is very essential.

comment:125 in reply to: ↑ 119 Changed 9 months ago by egourgoulhon

Replying to gh-DeRhamSource:

Replying to egourgoulhon:

Again, I don't like so much the methods is_unit() added to the classes ChartFunction and ScalarField. They don't reflect fully the mathematical meaning of being a unit element of a ring. Do you really need these in this ticket? (I don't see clearly the connection with vector bundles).

I'm not quite sure, but I think this method is mandatory when using the division free algorithm. And the division free algorithm is invoked when changes of frames are established. In general, for vector bundles, frames may be distributed over serveral chart domains, for which the SR enforcement method fails. So yes, this is related to this ticket. I'm not completely satisfied with this either. As soon as the issue in ticket #28629 is solved, this snippet can be deleted again.

Thanks for your answer.

Should I add a comment in the code about this?

I've seen that you have done it; thanks.

comment:126 in reply to: ↑ 124 Changed 9 months ago by egourgoulhon

Replying to gh-DeRhamSource:

Since default frames are somehow obligatoric for further use, I added them. Please check. Sorry for that. But I think, this modification is very essential.

LGTM.

comment:127 Changed 9 months ago by egourgoulhon

  • Status changed from needs_review to positive_review

Thank you very much for this very valuable addition to Sage!

comment:128 Changed 9 months ago by vbraun

  • Branch changed from public/manifolds/vector_bundles to 4ae4ca37508374ef9074cdbf9c650bb0d6ae8211
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.