Opened 3 years ago
Closed 3 years 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, GitHub, GitLab) | Commit: | 4ae4ca37508374ef9074cdbf9c650bb0d6ae8211 |
Dependencies: | #28189 | Stopgaps: |
Description (last modified by )
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 fromSection
,VectorFrame
inherits fromLocalFrame
- total space comes with induced charts
Change History (128)
comment:1 Changed 3 years ago by
- 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 3 years ago by
- Component changed from PLEASE CHANGE to geometry
comment:3 Changed 3 years ago by
- Branch set to u/gh-DeRhamSource/vector_bundles
comment:4 follow-up: ↓ 6 Changed 3 years ago by
- Commit set to 5d5520d19abace16d1eced2d67a28e2f1fbaa9fb
comment:5 Changed 3 years ago by
- Description modified (diff)
comment:6 in reply to: ↑ 4 Changed 3 years ago by
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:8 Changed 3 years ago by
- Commit changed from 5d5520d19abace16d1eced2d67a28e2f1fbaa9fb to ed60a5d8c5c03acf2627265bae19c8139376b126
Branch pushed to git repo; I updated commit sha1. New commits:
ed60a5d | Trivializations almost finished
|
comment:9 Changed 3 years ago by
- Commit changed from ed60a5d8c5c03acf2627265bae19c8139376b126 to b23b6768f4ea50f7f9aedaa117ec13ca6a84d576
comment:10 Changed 3 years ago by
- Dependencies set to #28189
comment:11 Changed 3 years ago by
- Commit changed from b23b6768f4ea50f7f9aedaa117ec13ca6a84d576 to bc1fac17d30a5f60f61e33a4e54aec2d216d64de
Branch pushed to git repo; I updated commit sha1. New commits:
bc1fac1 | Fibers and vectors of fibers implemented
|
comment:12 follow-up: ↓ 14 Changed 3 years ago by
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 3 years ago by
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.
comment:14 in reply to: ↑ 12 Changed 3 years ago by
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)
comment:15 Changed 3 years ago by
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 3 years ago by
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: ↓ 18 Changed 3 years ago by
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 3 years ago by
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 atotal_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: ↓ 20 Changed 3 years ago by
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! :)
comment:20 in reply to: ↑ 19 Changed 3 years ago by
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: ↓ 22 Changed 3 years ago by
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?
comment:22 in reply to: ↑ 21 Changed 3 years ago by
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 3 years ago by
- Commit changed from bc1fac17d30a5f60f61e33a4e54aec2d216d64de to 94cdc74abfac7f4f3d03a43121eebbabe95cc8a9
comment:24 Changed 3 years ago by
- Commit changed from 94cdc74abfac7f4f3d03a43121eebbabe95cc8a9 to c59f443dd7259aca2214639ab0e2f6e1dc4f4151
Branch pushed to git repo; I updated commit sha1. New commits:
c59f443 | Minor issues
|
comment:25 Changed 3 years ago by
- Commit changed from c59f443dd7259aca2214639ab0e2f6e1dc4f4151 to 12bfb7937bc3e978d8bec410929633889b1e487a
comment:26 follow-up: ↓ 28 Changed 3 years ago by
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
comment:27 Changed 3 years ago by
- Dependencies #28189 deleted
comment:28 in reply to: ↑ 26 ; follow-up: ↓ 30 Changed 3 years ago by
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
andsrc/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 withsage -docbuild reference/manifolds html
- notice that the doctests that output a dictionary, like
sage: E.changes_of_frame()
in line 105 oftrivialization.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 byp
. - the function
TopologicalVectorBundle.atlas()
returns a shallow copy ofself._atlas
; why does it not return directlyself._atlas
? - shouldn't the method
TopologicalVectorBundle.total_space()
be cached? (possibly using the decorator@cached_method
)
comment:29 Changed 3 years ago by
- Dependencies set to #28189
comment:30 in reply to: ↑ 28 ; follow-up: ↓ 32 Changed 3 years ago by
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:
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 withsage -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 oftrivialization.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 byp
.
Corrected. The projection compatibility was missing, too.
- the function
TopologicalVectorBundle.atlas()
returns a shallow copy ofself._atlas
; why does it not return directlyself._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 3 years ago by
- Commit changed from 12bfb7937bc3e978d8bec410929633889b1e487a to 901df657fef7850b041b7a98714fccb2e9b7b669
comment:32 in reply to: ↑ 30 ; follow-up: ↓ 33 Changed 3 years ago by
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 oftrivialization.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 ofself._atlas
; why does it not return directlyself._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: ↓ 34 Changed 3 years ago by
Replying to egourgoulhon:
Replying to gh-DeRhamSource:
- the function
TopologicalVectorBundle.atlas()
returns a shallow copy ofself._atlas
; why does it not return directlyself._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 directlyself._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: ↓ 35 Changed 3 years ago by
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 3 years ago by
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 3 years ago by
- Commit changed from 901df657fef7850b041b7a98714fccb2e9b7b669 to 9011e3d645f9ccbe8df35cfa8883f128312d2d9d
Branch pushed to git repo; I updated commit sha1. New commits:
9011e3d | Copy lists and frame restriction
|
comment:37 in reply to: ↑ description ; follow-up: ↓ 38 Changed 3 years ago by
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.
comment:38 in reply to: ↑ 37 Changed 3 years ago by
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 3 years ago by
- Commit changed from 9011e3d645f9ccbe8df35cfa8883f128312d2d9d to 9600762f72d9a49e408b18aeda17a14ecfad1327
comment:40 Changed 3 years ago by
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?
comment:41 Changed 3 years ago by
- Dependencies #28189 deleted
comment:42 Changed 3 years ago by
- Commit changed from 9600762f72d9a49e408b18aeda17a14ecfad1327 to 033960f3e3bc2f46cea69ce5ae4e5044718bdc9c
comment:43 Changed 3 years ago by
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 3 years ago by
- Dependencies set to #28189
comment:45 Changed 3 years ago by
- Description modified (diff)
comment:46 Changed 3 years ago by
- Description modified (diff)
comment:47 Changed 3 years ago by
- Description modified (diff)
comment:48 Changed 3 years ago by
- Description modified (diff)
- Milestone changed from sage-8.9 to sage-9.0
comment:49 Changed 3 years ago by
- Description modified (diff)
comment:50 Changed 3 years ago by
- Commit changed from 033960f3e3bc2f46cea69ce5ae4e5044718bdc9c to 69d05dad4177d25333b85de782ebecea1615baa4
Branch pushed to git repo; I updated commit sha1. New commits:
5d92db3 | 28189: prefer R in Fields() rather than R.is_field()
|
7767b30 | 28189: fix doctest in judson-abstract-algebra book
|
4981d44 | fix accidental merge with #28402
|
fc18183 | Merge new commit of branch 't/28189/28189' into t/28159/vector_bundles
|
69d05da | Doctests for section methods finished
|
comment:51 Changed 3 years ago by
- Commit changed from 69d05dad4177d25333b85de782ebecea1615baa4 to a737240ae5f5b6c07bf969e220062135e8955675
Branch pushed to git repo; I updated commit sha1. New commits:
a737240 | Doctests finished + default frame + typos
|
comment:52 Changed 3 years ago by
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 3 years ago by
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.
comment:54 Changed 3 years ago by
- Description modified (diff)
comment:55 Changed 3 years ago by
- Commit changed from a737240ae5f5b6c07bf969e220062135e8955675 to f4ef538c162039ea301de1626791405a33bbb65c
comment:56 Changed 3 years ago by
- Reviewers set to Eric Gourgoulhon, Travis Scrimshaw
- Status changed from new to needs_review
comment:57 follow-up: ↓ 60 Changed 3 years ago by
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 3 years ago by
- Commit changed from f4ef538c162039ea301de1626791405a33bbb65c to f484b7702cac56ad495c22735e15876b8232f8bf
Branch pushed to git repo; I updated commit sha1. New commits:
f484b77 | Docbuild errors fixed + Minor fixes
|
comment:59 Changed 3 years ago by
- Commit changed from f484b7702cac56ad495c22735e15876b8232f8bf to 1735541337db3f446641782dc6ce719bd417e3b5
Branch pushed to git repo; I updated commit sha1. New commits:
1735541 | Very small fix
|
comment:60 in reply to: ↑ 57 ; follow-up: ↓ 63 Changed 3 years ago by
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 passI would change
Moebius
toMö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 3 years ago by
- Commit changed from 1735541337db3f446641782dc6ce719bd417e3b5 to 54237ef4815198c80a0f95e949a8debaa7aafd2f
Branch pushed to git repo; I updated commit sha1. New commits:
54237ef | Pyflakes and Coverage
|
comment:62 Changed 3 years ago by
- Commit changed from 54237ef4815198c80a0f95e949a8debaa7aafd2f to 67cf8df97fe082f277fb54b1d3a0626f9617ac7f
Branch pushed to git repo; I updated commit sha1. New commits:
67cf8df | Merge branch 'develop' into t/28159/vector_bundles
|
comment:63 in reply to: ↑ 60 ; follow-up: ↓ 64 Changed 3 years ago by
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:
54237ef | Pyflakes and Coverage
|
67cf8df | Merge branch 'develop' into t/28159/vector_bundles
|
comment:64 in reply to: ↑ 63 Changed 3 years ago by
Thank you! I fixed pyflakes and coverage errors. I'm looking forward to your review! :)
comment:65 Changed 3 years ago by
- Commit changed from 67cf8df97fe082f277fb54b1d3a0626f9617ac7f to 9832d02285f25bdb95a11efcb6e705938a36c3f5
Branch pushed to git repo; I updated commit sha1. New commits:
9832d02 | Pyflakes and Coverage
|
comment:66 Changed 3 years ago by
- 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
comment:67 Changed 3 years ago by
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 3 years ago by
- Commit changed from 9832d02285f25bdb95a11efcb6e705938a36c3f5 to 73e103efc0857f4b37b95ccc13c2edf9ce93f551
Branch pushed to git repo; I updated commit sha1. New commits:
73e103e | Doctest errors fixed
|
comment:69 Changed 3 years ago by
This should be fixed now.
comment:70 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:71 follow-up: ↓ 72 Changed 3 years ago by
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 3 years ago by
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 3 years ago by
- 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: ↓ 77 Changed 3 years ago by
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 3 years ago by
- Commit changed from 73e103efc0857f4b37b95ccc13c2edf9ce93f551 to 15ca7ea8d2bf9f20461fedf21a9ce1c27afc1eef
Branch pushed to git repo; I updated commit sha1. New commits:
15ca7ea | SectionModule representation + doctest fixes
|
comment:76 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:77 in reply to: ↑ 74 Changed 3 years ago by
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: ↓ 79 Changed 3 years ago by
- 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 3 years ago by
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 3 years ago by
- Commit changed from 15ca7ea8d2bf9f20461fedf21a9ce1c27afc1eef to 3a7d29be713fa75cd3be0f5a14789db12635b879
Branch pushed to git repo; I updated commit sha1. New commits:
3a7d29b | Minor doctest fixes
|
comment:81 Changed 3 years ago by
- Status changed from needs_work to needs_review
Changed. Can you please check the docbuild for me?
comment:82 Changed 3 years ago by
- Commit changed from 3a7d29be713fa75cd3be0f5a14789db12635b879 to 116cbfa97a3acf0f53d252e1cea5dfa4f085f4b3
Branch pushed to git repo; I updated commit sha1. New commits:
116cbfa | SectionMOdule added to documentation
|
comment:83 Changed 3 years ago by
- Commit changed from 116cbfa97a3acf0f53d252e1cea5dfa4f085f4b3 to 078b13fd1729eed24521b4561947d8de4a92f2b9
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
078b13f | Section modules added to documentation
|
comment:84 Changed 3 years ago by
- Branch changed from u/gh-DeRhamSource/vector_bundles to public/manifolds/vector_bundles
- Commit changed from 078b13fd1729eed24521b4561947d8de4a92f2b9 to 12a86a4feef85048f97383363fd004f8d1961c44
New commits:
12a86a4 | Some fixes in the documentation of vector bundles
|
comment:85 Changed 3 years ago by
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?
comment:86 follow-up: ↓ 88 Changed 3 years ago by
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 3 years ago by
- Commit changed from 12a86a4feef85048f97383363fd004f8d1961c44 to b8eba570bccc8db1553bde445342ac716539fa33
Branch pushed to git repo; I updated commit sha1. New commits:
b8eba57 | Slight changes in the documentation and code of vector bundles
|
comment:88 in reply to: ↑ 86 ; follow-up: ↓ 89 Changed 3 years ago by
Replying to gh-DeRhamSource:
There's just one thing in
differentiable/vector_bundle.py
around line 389 in the classTensorBundle
:"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: ↓ 90 Changed 3 years ago by
Replying to egourgoulhon:
I have also performed some changes in the code, replacing
self._dest_map
byself._dest_map.restrict(domain)
inDifferentiableVectorBundle.section()
andDifferentiableVectorBundle.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, aVectorFrame
should not be returned. Do you agree? Shall we maintain the aliasvector_frame
forlocal_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 classDifferentiableVectorBundle
: 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)
ofself
.
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.
comment:90 in reply to: ↑ 89 ; follow-up: ↓ 92 Changed 3 years ago by
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, aVectorFrame
should not be returned. Do you agree? Shall we maintain the aliasvector_frame
forlocal_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 classDifferentiableVectorBundle
: 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)
ofself
.The class
TopologicalVectorBundle
consists of these methods andTensorBundle
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
fromSection
(and perhapsVectorFrame
fromLocalFrame
) in order to minimize code redundancies.
Yes, I guess by "next step", you mean "in another ticket".
comment:91 Changed 3 years ago by
- Commit changed from b8eba570bccc8db1553bde445342ac716539fa33 to 770e6999c8e3a0cb8c2f7264dd736628634ad606
Branch pushed to git repo; I updated commit sha1. New commits:
770e699 | More detailed documentation
|
comment:92 in reply to: ↑ 90 Changed 3 years ago by
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. :)
comment:93 Changed 3 years ago by
Ah, I've also fixed the filter algorithms for the frames. (Unfortunately, I missed it in the commit message.)
comment:94 Changed 3 years ago by
There are some more things I noticed. I'm going to take of it later this day.
comment:95 Changed 3 years ago by
- Commit changed from 770e6999c8e3a0cb8c2f7264dd736628634ad606 to 22e6666e641777f26eb165ba4623f6c28fa8e70b
Branch pushed to git repo; I updated commit sha1. New commits:
22e6666 | Documentation improvements, i.e. frames along maps
|
comment:96 follow-up: ↓ 98 Changed 3 years ago by
Please take a look. I made many modifications and fixes in the documentation.
comment:97 Changed 3 years ago by
- Commit changed from 22e6666e641777f26eb165ba4623f6c28fa8e70b to 9ecd22a1f537f5392dbad999612c2d433dc2fabc
comment:98 in reply to: ↑ 96 Changed 3 years ago by
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 3 years ago by
Just wait a moment. I have still some fixes, coming soon.
comment:100 Changed 3 years ago by
- Commit changed from 9ecd22a1f537f5392dbad999612c2d433dc2fabc to be0b7d0ff8b2ea739b97bc339e68a2403b74f4df
Branch pushed to git repo; I updated commit sha1. New commits:
be0b7d0 | further methods added + 'local_frame' invokes vector field module's method + doc improvements
|
comment:101 Changed 3 years ago by
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 3 years ago by
- Commit changed from be0b7d0ff8b2ea739b97bc339e68a2403b74f4df to f9810b77472ee3c16f4843aca27239e149e48ffa
Branch pushed to git repo; I updated commit sha1. New commits:
f9810b7 | unique_tag documentation added
|
comment:103 Changed 3 years ago by
- 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 3 years ago by
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.
comment:105 follow-up: ↓ 106 Changed 3 years ago by
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.
comment:106 in reply to: ↑ 105 Changed 3 years ago by
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 3 years ago by
Okay, I started some debug 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 down enormously. I committed the changes.
comment:108 Changed 3 years ago by
- 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 3 years ago by
- Status changed from needs_work to needs_review
comment:110 Changed 3 years ago by
Just to tell that I am mostly AFK these days. I'll have a look at the ticket on Friday.
comment:111 follow-up: ↓ 112 Changed 3 years ago by
- 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: ↓ 113 Changed 3 years ago by
Replying to egourgoulhon:
I don't agree with the proposed fix for it goes against mathematical correctness: the method
is_field()
ofScalarFieldAlgebra
shall not returnTrue
, 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 ofSR
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 3 years ago by
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()
ofScalarFieldAlgebra
shall not returnTrue
, 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 3 years ago by
- Commit changed from aa7dbaf6055abab69c1cb43fb3ef709b9715b7ae to 39104657254e432c49bef596337fbae291ca5f77
comment:115 Changed 3 years ago by
Committed. Please check, again.
comment:116 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:117 follow-up: ↓ 119 Changed 3 years ago by
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 3 years ago by
Btw, thanks for having moved the treatment of autormorphism field inverses to #28629.
comment:119 in reply to: ↑ 117 ; follow-up: ↓ 125 Changed 3 years ago by
Replying to egourgoulhon:
Again, I don't like so much the methods
is_unit()
added to the classesChartFunction
andScalarField
. 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 inChartFunction.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 ofis_trivial_zero()
inScalarField.is_unit()
is better.
I agree, this is certainly better.
comment:120 Changed 3 years ago by
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 3 years ago by
- Description modified (diff)
comment:122 Changed 3 years ago by
- Commit changed from 39104657254e432c49bef596337fbae291ca5f77 to 279f4283bd79bf0a964d971c8aed4d006e174813
comment:123 Changed 3 years ago by
- Commit changed from 279f4283bd79bf0a964d971c8aed4d006e174813 to 4ae4ca37508374ef9074cdbf9c650bb0d6ae8211
Branch pushed to git repo; I updated commit sha1. New commits:
4ae4ca3 | default frame added to vector bundles + doctree modified
|
comment:124 follow-up: ↓ 126 Changed 3 years ago by
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 3 years ago by
Replying to gh-DeRhamSource:
Replying to egourgoulhon:
Again, I don't like so much the methods
is_unit()
added to the classesChartFunction
andScalarField
. 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 3 years ago by
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 3 years ago by
- Status changed from needs_review to positive_review
Thank you very much for this very valuable addition to Sage!
comment:128 Changed 3 years ago by
- Branch changed from public/manifolds/vector_bundles to 4ae4ca37508374ef9074cdbf9c650bb0d6ae8211
- Resolution set to fixed
- Status changed from positive_review to closed
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:
Basic structure