Opened 4 years ago
Closed 3 years ago
#27784 closed enhancement (fixed)
Characteristic Classes on Vector Bundles
Reported by:  ghDeRhamSource  Owned by:  

Priority:  major  Milestone:  sage9.0 
Component:  geometry  Keywords:  characteristic classes, manifolds 
Cc:  tscrim, egourgoulhon  Merged in:  
Authors:  Michael Jung  Reviewers:  Travis Scrimshaw, Eric Gourgoulhon 
Report Upstream:  N/A  Work issues:  
Branch:  6830d30 (Commits, GitHub, GitLab)  Commit:  6830d30b2c4aad7dd0c7b383eb8befa3d9c8bde8 
Dependencies:  #28159, #28189, #28564, #28716  Stopgaps: 
Description (last modified by )
Characteristic classes on differentiable vector bundles.
Features
 characteristic classes on differentiable manifolds using the ChernWeil homomorphism
 class
BundleConnection
introduced with rudimentary methods  each characteristic class carries a dictionary with bundle connection as keys and mixed forms as values in order to simulate the cohomology equivalence
Planned sometime
 transgression forms
Change History (62)
comment:1 Changed 4 years ago by
Authors:  → Michael Jung 

Cc:  tscrim egourgoulhon added 
Component:  PLEASE CHANGE → geometry 
Keywords:  characteristic classes manifolds added 
Type:  PLEASE CHANGE → enhancement 
comment:2 Changed 4 years ago by
Description:  modified (diff) 

comment:3 Changed 4 years ago by
Branch:  → u/ghDeRhamSource/characteristic_classes_on_manifolds 

comment:4 Changed 4 years ago by
Commit:  → 716fb6975472331688cc108b524b80bebaef2360 

comment:5 Changed 4 years ago by
Branch:  u/ghDeRhamSource/characteristic_classes_on_manifolds → u/ghDeRhamSource/char_classes 

Commit:  716fb6975472331688cc108b524b80bebaef2360 
comment:6 Changed 4 years ago by
Branch:  u/ghDeRhamSource/char_classes → u/ghDeRhamSource/characteristic_classes_on_manifolds 

Commit:  → 716fb6975472331688cc108b524b80bebaef2360 
comment:7 Changed 4 years ago by
Commit:  716fb6975472331688cc108b524b80bebaef2360 → 963dede7873fc21626136606b1755767f7ceb241 

Branch pushed to git repo; I updated commit sha1. New commits:
963dede  is_field implemented

comment:8 Changed 4 years ago by
Milestone:  sage8.8 

As the Sage8.8 release milestone is pending, we should delete the sage8.8 milestone for tickets that are not actively being worked on or that still require significant work to move forward. If you feel that this ticket should be included in the next Sage release at the soonest please set its milestone to the next release milestone (sage8.9).
comment:9 Changed 4 years ago by
Milestone:  → sage8.9 

comment:10 Changed 4 years ago by
Branch:  u/ghDeRhamSource/characteristic_classes_on_manifolds → u/ghDeRhamSource/characteristic_classes 

Commit:  963dede7873fc21626136606b1755767f7ceb241 → 2cabe93c84b8fe5aea82f46269f03e73b026cecd 
Summary:  Characteristic Classes on Manifolds → Characteristic Classes on Vector Bundles 
New commits:
2cabe93  General structure of vector bundles implemented

comment:11 Changed 3 years ago by
Dependencies:  → #28159, #28189 

Description:  modified (diff) 
Milestone:  sage8.9 → sage9.0 
comment:12 Changed 3 years ago by
Description:  modified (diff) 

comment:13 Changed 3 years ago by
Commit:  2cabe93c84b8fe5aea82f46269f03e73b026cecd → b313f562826599d5361067df91606c32803a145a 

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

comment:14 Changed 3 years ago by
Dependencies:  #28159, #28189 → #28159, #28189, #28564 

comment:15 Changed 3 years ago by
Status:  new → needs_review 

comment:16 Changed 3 years ago by
Commit:  b313f562826599d5361067df91606c32803a145a → 0a2190fc1c0f7cf9e74a60f8ea3d590fdded30da 

Branch pushed to git repo; I updated commit sha1. New commits:
0a2190f  Ticket #27784: Wrong naming in Doctest

comment:17 Changed 3 years ago by
Commit:  0a2190fc1c0f7cf9e74a60f8ea3d590fdded30da → 1bae0ab620ca652edcf75979583626c4bf82f438 

Branch pushed to git repo; I updated commit sha1. New commits:
1bae0ab  Trac #27784: Documentation grammar checkup

comment:18 Changed 3 years ago by
Commit:  1bae0ab620ca652edcf75979583626c4bf82f438 → 32ecd26f6f0172c3a3ab0ec0f2928a37a9d72862 

Branch pushed to git repo; I updated commit sha1. New commits:
32ecd26  Trac #27784: 0th element of Pfaffian class is always zero

comment:19 Changed 3 years ago by
I know, you are really busy right now, Eric. However, this ticket is the essence and foundation of my master thesis and I'd highly appreciate at least a short look whether the structure is okay.
Travis, could you take a short look, too?
Details can be changed later, but the rough structure should be fine before I start writing my thesis.
Thank you both! I really appreciate your effort so far. :)
comment:20 Changed 3 years ago by
Commit:  32ecd26f6f0172c3a3ab0ec0f2928a37a9d72862 → cacd4131916ff872077ef17f516a58a99f63474e 

Branch pushed to git repo; I updated commit sha1. New commits:
cacd413  Trac #27784: Pyflake errors

comment:21 Changed 3 years ago by
Commit:  cacd4131916ff872077ef17f516a58a99f63474e → 2ec3a435fc3742a583f01a897ab393a4f514cf9c 

Branch pushed to git repo; I updated commit sha1. New commits:
2ec3a43  Trac #27784: Whitspace removed

comment:22 Changed 3 years ago by
Commit:  2ec3a435fc3742a583f01a897ab393a4f514cf9c → a78e1eca7caa78caf5d592ff9ed93843496294e1 

Branch pushed to git repo; I updated commit sha1. New commits:
a78e1ec  Trac #27784: Documentation improved

comment:23 followup: 24 Changed 3 years ago by
I won't be able to look at it for at least a few days. However, I will make it a high priority.
comment:24 Changed 3 years ago by
Replying to tscrim:
I won't be able to look at it for at least a few days. However, I will make it a high priority.
Thank you so much! :)
comment:25 followup: 40 Changed 3 years ago by
Here are my comments (mostly on the technical details):
Don't feel the strict need to adhere to PEP8 when it degrades readability. In particular, don't have a line break here:
from sage.manifolds.differentiable.vector_bundle\  import DifferentiableVectorBundle +from sage.manifolds.differentiable.vector_bundle import DifferentiableVectorBundle
and similarly here
sage: from sage.manifolds.differentiable.bundle_connection import \ BundleConnection
In contrast, you can break .. MATH::
lines.
Remove the period/fullstop at the end here:
 ``latex_name``  (default: ``None``) LaTeX symbol to denote the bundle connection; if ``None``, it is set to ``name``.
I don't see the point of this note:
.. NOTE:: Notice that this is a *very* rudimentary form of bundle connections. A more detailed implementation is devoted to :trac:`28640`.
The only people who will care are developers, it is irrelevant to the use of this class, and it makes it come across as less polished.
The block after They certainly obey the structure equation::
is overindented.
A general comment (not something you need to address here, but I noticed it now): we should have a little mixin class for _latex_name
objects (or perhaps more generally NamedObject
) so we don't have so much repeated boilerplate code.
Instead of _hash_value
, the more standard naming is _hash
.
Error messages should follow the general Python convention:
raise ValueError("A frame must be provided!") +raise ValueError("a frame must be provided")
Is this relevant? # TODO: Compute transformations
Should it instead raise a NotImplementedError
?
Is there some way we can automate this?
.. WARNING:: If the connection has already forms in other frames, it is the user's responsibility to make sure that the 1forms to be added are consistent with them.
computation in SAGE. +computation in Sage.
Although really I would suggest just deleting that sentence (you already are using Sage and such an internal working, if you insist on including it, should be in an ALGORITHM:
section).
I generally dislike these things:
Class Documentation 
They don't convey any information and just are a bit of a distraction IMO.
_get_predefined_class
is not a static method (it is just a separate function). Also `arg`
> ``arg``
This should just be an ifelse
statement:
c_dict = {} c_dict['ChernChar'] = ('complex', 'additive', 'ch', r'\mathrm{ch}', x.exp()) c_dict['Todd'] = ('complex', 'additive', 'Td', r'\mathrm{Td}', x / (1  (x).exp())) c_dict['Chern'] = ('complex', 'multiplicative', 'c', 'c', 1 + x) c_dict['Pontryagin'] = ('real', 'multiplicative', 'p', 'p', 1 + x) c_dict['AHat'] = ('real', 'multiplicative', 'A^', r'\hat{A}', x.sqrt() / (2 * (x.sqrt() / 2).sinh())) c_dict['Hirzebruch'] = ('real', 'multiplicative', 'L', 'L', (x.sqrt() / x.sqrt().tanh())) c_dict['Euler'] = ('real', 'Pfaffian', 'e', 'e', x) # Get class from arg try: res = c_dict[arg] except KeyError: raise ValueError("the characteristic class '{}' is ".format(arg) + "not predefined yet.")
unless you were hoping to cache c_dict
, but I don't see the utility in that as this should not be a bottleneck.
Do not abbreviate class names and UniqueRepresentation
is better being first in the MRO:
class CharClass(SageObject, UniqueRepresentation): +class CharacteristicClass(UniqueRepresentation, SageObject):
IMO, generally you should avoid abbreviated names for inputs/function names/etc. for the main implementation (although it is okay to have an abbreviated alias). (For another example, get_form(self, con, cmatrices=None):
and use connection
instead of con
and def char_class
to def characteristic_class
) This better selfdocuments the code and can make things more discoverable with tab completion. For example, I might type .charact<tab>
and then be surprised that nothing comes up.
In _get_coeff_list
, there is no need for the coef
variable (just return the result).
Also
(self._base_space._dim / 2).floor() +self._base_space._dim // 2
and make from sage.symbolic.ring import SR
at the toplevel.
 ``cmatrix`` curvature matrix + ``cmatrix``  curvature matrix
comment:26 Changed 3 years ago by
Commit:  a78e1eca7caa78caf5d592ff9ed93843496294e1 → 0857ec4dfd6027a8725712631d908a62ac49eef2 

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
47e2f51  copy method with naming

f55b881  Doctest adapted

f85de87  Merge branch 'develop' into t/28564/tensor_fields__consistent_naming

eb92e6a  Doctest fixed

5b2eee2  'copy' with individual naming

38aa7f5  Merge branch 'develop' into t/28564/tensor_fields__consistent_naming

9d39cb5  Merge branch 'develop' into t/28564/tensor_fields__consistent_naming

67e5cdc  Trac #28564: One line doctest error fixed

38eeb4a  Merge branch 't/28564/tensor_fields__consistent_naming' into char_classes27784

0857ec4  Trac #27784: Typos and minor code improvements

comment:27 Changed 3 years ago by
Hopefully, I took everything into account you mentioned.
Thank you for the extensive feedback! :)
comment:28 Changed 3 years ago by
Commit:  0857ec4dfd6027a8725712631d908a62ac49eef2 → 1db2c89cc0833f3889a97bcc73cad056872434d9 

Branch pushed to git repo; I updated commit sha1. New commits:
1db2c89  Trac #27784: Line breaks

comment:29 followups: 30 31 Changed 3 years ago by
I gave a quik look. It looks nice! Thanks for having taken into account Travis' remarks. I fully agree with them. Here a few remarks from my side:
 There is a failed doctest:
File "src/sage/manifolds/differentiable/char_class.py", line 685, in sage.manifolds.differentiable.char_class.CharacteristicClass.get_form Failed example: ch_form.display() Expected: ch(E, nabla^E) = ch_0(E, nabla^E) + zero + ch_1(E, nabla^E) Got: ch(E, nabla^E) = ch_0.0(E, nabla^E) + zero + ch_1.0(E, nabla^E)
 There are two warnings when generating the documentation:
[manifolds] <unknown>:2566: DeprecationWarning: invalid escape sequence \c [manifolds] <unknown>:2565: DeprecationWarning: invalid escape sequence \c
 in the OUTPUT section of
BundleConnection.add_connection_form()
, a back quote is missing: connection 1form `\omega^i_j in the given frame, as an instance of + connection 1form `\omega^i_j` in the given frame, as an instance of
 in the introduction to characteristic classes,
N
is not defined in the sentencefor any continuous map `f: M \to N`
 in the doctest example of
in DifferentiableVectorBundle.characteristic_class()
: sage: g = M.metric('g') + sage: g = M.metric()
(since M
has been defined a Lorentzian manifold, g
has already been initialized, with the name 'g')
comment:30 followup: 32 Changed 3 years ago by
Replying to egourgoulhon:
I gave a quik look. It looks nice! Thanks for having taken into account Travis' remarks. I fully agree with them. Here a few remarks from my side:
 There is a failed doctest:
File "src/sage/manifolds/differentiable/char_class.py", line 685, in sage.manifolds.differentiable.char_class.CharacteristicClass.get_form Failed example: ch_form.display() Expected: ch(E, nabla^E) = ch_0(E, nabla^E) + zero + ch_1(E, nabla^E) Got: ch(E, nabla^E) = ch_0.0(E, nabla^E) + zero + ch_1.0(E, nabla^E)
That's strange...I don't get this error. Travis?
The rest is in progress...
comment:31 followup: 33 Changed 3 years ago by
Ah, that might come from using Python 2.7. Hang on there.
comment:32 Changed 3 years ago by
Replying to ghDeRhamSource:
That's strange...I don't get this error. Travis?
Are you using Python 3 Sage ?
comment:33 Changed 3 years ago by
Replying to ghDeRhamSource:
Ah, that might come from using Python 2.7. Hang on there.
Yes most probably
comment:34 Changed 3 years ago by
Commit:  1db2c89cc0833f3889a97bcc73cad056872434d9 → ebbfb16bb285b1cb3a9430eae2a5165ba7e891e2 

comment:35 Changed 3 years ago by
The issue about the DeprecationWarning
referred to #28564. That has been fixed.
The other issues has been solved, too.
However, I'll build Sage from scratch, now.
comment:36 Changed 3 years ago by
With the latest version, there are two failed doctests in char_class.py
. I got them on my machine and they are reported by the patchbot as well.
comment:37 Changed 3 years ago by
Commit:  ebbfb16bb285b1cb3a9430eae2a5165ba7e891e2 → 310cf9a302e28500c77f3cb0a2b23f440c43a155 

Branch pushed to git repo; I updated commit sha1. New commits:
310cf9a  Trac #27784: set_restriction placed wrong

comment:38 Changed 3 years ago by
I guess that should be it. However, my build is not finished yet. I will start an extensive test as soon as possible.
Thanks so far! :)
comment:39 Changed 3 years ago by
My tests did all pass. Patchbot says so, too.
By the way: Why is the patchbot yellow and not green though all tests passed?
comment:40 followup: 46 Changed 3 years ago by
Replying to tscrim:
Is there some way we can automate this?
.. WARNING:: If the connection has already forms in other frames, it is the user's responsibility to make sure that the 1forms to be added are consistent with them.
The concept and warning is just copied from affine_connection.py
. At least it should be possible to return an error message when some incompatibilities occur on the intersection. But that's a topic for another ticket, I'd say.
comment:41 followup: 42 Changed 3 years ago by
Patchbot says:
sage t long src/sage/parallel/map_reduce.py # Timed out sage t long src/doc/en/reference/references/index.rst # Tab character found
comment:42 Changed 3 years ago by
Replying to chapoton:
Patchbot says:
sage t long src/sage/parallel/map_reduce.py # Timed out sage t long src/doc/en/reference/references/index.rst # Tab character found
The first one has nothing to do with the changes in this ticket.
However, the second has been fixed now.
Thanks! :)
comment:43 Changed 3 years ago by
Commit:  310cf9a302e28500c77f3cb0a2b23f440c43a155 → 3fa8d85ed4e6a27388fcbe629e0dcea8017dbf47 

Branch pushed to git repo; I updated commit sha1. New commits:
3fa8d85  Trac 27784: Tab character removed

comment:44 Changed 3 years ago by
Also, it seems there is a doctest missing in map_reduce.py
. What's going on there? :/
comment:46 Changed 3 years ago by
Replying to ghDeRhamSource:
Replying to tscrim:
Is there some way we can automate this?
.. WARNING:: If the connection has already forms in other frames, it is the user's responsibility to make sure that the 1forms to be added are consistent with them.The concept and warning is just copied from
affine_connection.py
. At least it should be possible to return an error message when some incompatibilities occur on the intersection. But that's a topic for another ticket, I'd say.
I agree: this could be postponed to another ticket; in particular, checking the compatibility can be computationaly demanding and one shall discuss the appropriate way to do this, with possibly an option check=True
or check=False
.
comment:47 Changed 3 years ago by
Replying to tscrim:
Patchbot has come back green.
From my side, I think we are almost done here. Two last remarks:
 in the introduction to characteristic classes,
\mathfrak{g}
is not defined  could the file
char_class.py
be renamedcharacteristic_class.py
? (this would improve the readability of the directorysrc/sage/manifolds/differentiable
and would match with the name of the classCharacteristicClass
)
comment:48 Changed 3 years ago by
Commit:  3fa8d85ed4e6a27388fcbe629e0dcea8017dbf47 → e4bb884b04dd0ad2ebcbb2db4df72d3eb300ef50 

Branch pushed to git repo; I updated commit sha1. New commits:
e4bb884  Trac #27784: File renamed + documentation

comment:49 followup: 50 Changed 3 years ago by
I hope, I took the renaming everywhere into account...
comment:50 Changed 3 years ago by
Replying to ghDeRhamSource:
I hope, I took the renaming everywhere into account...
Thanks for the changes. I am OK for a positive review. Travis?
comment:51 Changed 3 years ago by
Reviewers:  → Travis Scrimshaw, Eric Gourgoulhon 

Status:  needs_review → positive_review 
Yep, I am also good with it.
comment:52 Changed 3 years ago by
Commit:  e4bb884b04dd0ad2ebcbb2db4df72d3eb300ef50 → 3c93ab9fb44cf243f824373e6cc5c9707faae7e1 

Status:  positive_review → needs_review 
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
3c93ab9  Trac 27784: Merge branch 'develop' into char_classes27784

comment:54 Changed 3 years ago by
Status:  needs_review → positive_review 

The 9.0.beta6 patchbot is all green.
comment:55 Changed 3 years ago by
Commit:  3c93ab9fb44cf243f824373e6cc5c9707faae7e1 → dba0660fb3319fadcc1482a1b8db637a47678c03 

Status:  positive_review → needs_review 
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
dba0660  Trac #27784: Rank defined in documentation

comment:56 followup: 58 Changed 3 years ago by
That was just a very very minor thing I noticed during my thesis preparation. Sorry.
By the way: Am I allowed to switch the review status back to "positive" by myself in such cases?
comment:57 Changed 3 years ago by
Status:  needs_review → positive_review 

comment:58 Changed 3 years ago by
Replying to ghDeRhamSource:
That was just a very very minor thing I noticed during my thesis preparation. Sorry.
No problem. It's always good to improve the documentation!
By the way: Am I allowed to switch the review status back to "positive" by myself in such cases?
I don't think we have very precise rules about this. IMHO, it seems better to have an external eye, even for seemingly minor changes.
comment:59 Changed 3 years ago by
Commit:  dba0660fb3319fadcc1482a1b8db637a47678c03 → 6830d30b2c4aad7dd0c7b383eb8befa3d9c8bde8 

Status:  positive_review → needs_review 
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
a331374  Trac #27784: `g(0)=0` condition for real additive classes added in doc

013fb8b  Add construction of a vector frame from a family of vector fields

71b5f0f  Replace keyword argument 'from_family' by positional argument in vector_frame()

d0ef4d7  Add construction of a local frame from a set of vector fields in TensorBundle.local_frame()

6635fad  Construction of a local frame on a vector bundle from a family of sections

5c9c516  Merge branch 'public/manifolds/vector_frame_from_family28716' of git://trac.sagemath.org/sage into Sage 9.0.beta6

81e2f60  Minor fix in the documentation of TopologicalVectorBundle.local_frame()

6830d30  Trac #27784: Merge trac #28716 into trac #27784

comment:60 Changed 3 years ago by
Dependencies:  #28159, #28189, #28564 → #28159, #28189, #28564, #28716 

Please check the merge and doc change.
comment:61 Changed 3 years ago by
Status:  needs_review → positive_review 

Just checked that everything is in order with the new depedency #28716 and that all doctests are passed.
comment:62 Changed 3 years ago by
Branch:  u/ghDeRhamSource/characteristic_classes → 6830d30b2c4aad7dd0c7b383eb8befa3d9c8bde8 

Resolution:  → fixed 
Status:  positive_review → closed 
Branch pushed to git repo; I updated commit sha1. New commits: