Opened 4 years ago
Closed 3 years ago
#27784 closed enhancement (fixed)
Characteristic Classes on Vector Bundles
Reported by: | gh-DeRhamSource | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.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 Chern-Weil 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/gh-DeRhamSource/characteristic_classes_on_manifolds |
---|
comment:4 Changed 4 years ago by
Commit: | → 716fb6975472331688cc108b524b80bebaef2360 |
---|
comment:5 Changed 4 years ago by
Branch: | u/gh-DeRhamSource/characteristic_classes_on_manifolds → u/gh-DeRhamSource/char_classes |
---|---|
Commit: | 716fb6975472331688cc108b524b80bebaef2360 |
comment:6 Changed 4 years ago by
Branch: | u/gh-DeRhamSource/char_classes → u/gh-DeRhamSource/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: | sage-8.8 |
---|
As the Sage-8.8 release milestone is pending, we should delete the sage-8.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 (sage-8.9).
comment:9 Changed 4 years ago by
Milestone: | → sage-8.9 |
---|
comment:10 Changed 4 years ago by
Branch: | u/gh-DeRhamSource/characteristic_classes_on_manifolds → u/gh-DeRhamSource/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: | sage-8.9 → sage-9.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: 0-th 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 follow-up: 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 follow-up: 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/full-stop 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 over-indented.
A general comment (not something you need to address here, but I noticed it now): we should have a little mix-in 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 1-forms 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 if-else
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 self-documents 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 top-level.
-- ``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_classes-27784
|
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 follow-ups: 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 1-form `\omega^i_j in the given frame, as an instance of + connection 1-form `\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 follow-up: 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 follow-up: 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 gh-DeRhamSource:
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 gh-DeRhamSource:
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 follow-up: 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 1-forms 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 follow-up: 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 gh-DeRhamSource:
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 1-forms 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 follow-up: 50 Changed 3 years ago by
I hope, I took the renaming everywhere into account...
comment:50 Changed 3 years ago by
Replying to gh-DeRhamSource:
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_classes-27784
|
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 follow-up: 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 gh-DeRhamSource:
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_family-28716' 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/gh-DeRhamSource/characteristic_classes → 6830d30b2c4aad7dd0c7b383eb8befa3d9c8bde8 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Branch pushed to git repo; I updated commit sha1. New commits: