Opened 20 months ago
Closed 2 months ago
#29581 closed enhancement (fixed)
New Algorithm for Characteristic Classes
Reported by:  ghmjungmath  Owned by:  

Priority:  major  Milestone:  sage9.5 
Component:  manifolds  Keywords:  characteristic_classes, manifolds 
Cc:  slelievre, egourgoulhon, tscrim, mkoeppe, ghtobiasdiez  Merged in:  
Authors:  Michael Jung  Reviewers:  Travis Scrimshaw, Eric Gourgoulhon 
Report Upstream:  N/A  Work issues:  
Branch:  ac5cbbf (Commits, GitHub, GitLab)  Commit:  ac5cbbf0072380022c32466aab95fdfb31c4ecee 
Dependencies:  #32270, #32272, #32396  Stopgaps: 
Description (last modified by )
The current algorithm for characteristic forms is comparably slow. The worst case scenario showed computations times about 1h in dimension four.
With this ticket, we want to replace the current algorithm and implement characteristic cohomology classes as subring of the de Rham cohomology ring (cf. #31691).
The idea is that characteristic (cohomology) classes are generated by Chern/Pontryagin/Euler? classes. The generators can be computed by an FaddeevLeVerrier?like algorithm (cf. #30681).
Change History (126)
comment:1 Changed 20 months ago by
 Branch set to u/ghmjungmath/new_algorithm
comment:2 Changed 20 months ago by
 Commit set to 1a803eba5b9724ebfa93e726a474d957fd6915b4
 Milestone changed from sage9.1 to sage9.2
comment:3 Changed 20 months ago by
 Cc slelievre added
 Component changed from PLEASE CHANGE to geometry
comment:4 Changed 20 months ago by
Please add a short description in the "Description" field of the ticket,
and the author's full name in the "Authors" field of the ticket. Don't forget
to set to needs_review
when this is ready for review.
comment:5 Changed 20 months ago by
 Commit changed from 1a803eba5b9724ebfa93e726a474d957fd6915b4 to cb442eb8919ca33a5db63a17bac52f99f55553dd
Branch pushed to git repo; I updated commit sha1. New commits:
730046c  Merge branch 'develop' into t/29570/diff_form_bug

130ae3f  Trac #29570: NotImplementedError for nongeneric ring elements

bba21e1  Trac #29570: Strange typo fixed...

ba3b4b9  Trac #29570: correct parent, vectorfield_module changes reverted

841e1bf  Merge branch 't/29570/diff_form_bug' into t/29581/new_algorithm

cb442eb  Trac #29570: new algorithm added and code cleaned

comment:6 Changed 20 months ago by
 Commit changed from cb442eb8919ca33a5db63a17bac52f99f55553dd to 15c7341d071327a04fd229c3d2f78d583564f476
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
15c7341  Trac #29581: new algorithm added and code cleaned

comment:7 Changed 20 months ago by
 Cc egourgoulhon added
 Description modified (diff)
 Keywords characteristic_classes manifolds added
 Type changed from PLEASE CHANGE to enhancement
comment:8 Changed 20 months ago by
Correct syntax for crosslinks is
.. SEEALSO::
comment:9 Changed 19 months ago by
 do not use this:
if distinct_real == False
but
if not distinct_real
(at least in two places)
 The following change is wrong:
  ``cmatrix``  curvature matrix  +  ``cmatrix``  curvature matrix OUTPUT:   ``I/(2*pi)*cmatrix`` +  ``I/(2*pi)*cmatrix``
as one should not indent inside INPUT or OUTPUT blocks (because they end with only one colon). And moreover, one does want empty lines to separate these things.
This is also incorrect:
 self._dual_exterior_powers[p] = ExtPowerDualFreeModule(self, p) + self._dual_exterior_powers[p] = ExtPowerDualFreeModule(self, p)
Why did you change the indentation ? it was ok.
comment:10 Changed 19 months ago by
I am sorry. At this stage, this is just a draft. The indentations are typos coming from using alt+tab (at least I guess so). I will fix this soon.
comment:11 Changed 19 months ago by
Thanks for your advice. :)
comment:12 Changed 19 months ago by
 Commit changed from 15c7341d071327a04fd229c3d2f78d583564f476 to d37229341effff46ea2fa4b3287e32997c8422b0
Branch pushed to git repo; I updated commit sha1. New commits:
d372293  Trac #29581: Strange typos reverted

comment:13 Changed 19 months ago by
 Status changed from new to needs_review
comment:14 Changed 19 months ago by
 Status changed from needs_review to needs_info
comment:15 Changed 19 months ago by
An example for the application of the new algorithm would be really nice. For instance a Todd class or AHat class. But I have no idea for a suitable one. If anyone does, or at least knows someone who does, I would really appreciate it. Thanks! :)
comment:16 Changed 19 months ago by
 Commit changed from d37229341effff46ea2fa4b3287e32997c8422b0 to 19815c4257cee7dcf503d94e3b9bb651150bef5f
comment:17 Changed 17 months ago by
 Component changed from geometry to manifolds
 Dependencies set to #30211
comment:18 Changed 14 months ago by
 Milestone changed from sage9.2 to sage9.3
comment:19 Changed 9 months ago by
 Milestone changed from sage9.3 to sage9.4
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
comment:20 Changed 5 months ago by
 Milestone changed from sage9.4 to sage9.5
Setting a new milestone for this ticket based on a cursory review.
comment:21 Changed 5 months ago by
 Cc tscrim mkoeppe added
 Dependencies #30211 deleted
 Description modified (diff)
 Status changed from needs_info to needs_work
comment:22 Changed 5 months ago by
 Commit changed from 19815c4257cee7dcf503d94e3b9bb651150bef5f to 3c9b80a2551994493f23640344e9ade63240cb01
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
3c9b80a  Trac #29581: implement algorithms for chern, pontryagin and euler

comment:23 Changed 5 months ago by
Do you think it is necessary to give those cohomology classes an own parent or would DeRhamCohomologyRing
as parent suffice?
The point is that addition and multiplication is performed differently among those elements. Moreover, it is possible to compare them.
comment:24 Changed 5 months ago by
 Description modified (diff)
comment:25 Changed 5 months ago by
 Status changed from needs_work to needs_info
comment:26 Changed 5 months ago by
 Description modified (diff)
comment:27 Changed 5 months ago by
 Commit changed from 3c9b80a2551994493f23640344e9ade63240cb01 to 03529f8919357f98b65ee033e8dc5c7d48b2fa42
Branch pushed to git repo; I updated commit sha1. New commits:
03529f8  Trac #29581: return mixed form

comment:28 Changed 4 months ago by
 Commit changed from 03529f8919357f98b65ee033e8dc5c7d48b2fa42 to ab288d62786a40b9f314d40ca932506d7b6932d9
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
fb9c661  #32270: de rham cohomology as algebra

de5863a  Merge branch 't/32270/turn_de_rham_cohomology_into_algebra' into t/29581/new_algorithm

d22ee9a  Trac #32315: support iteration and enumerated sets

16c4737  Trac #32272: allow infinite max degree

6a320a0  Trac #32272: fix docstring

c1d4fce  Trac #32272: fix rst file + rename module

25e6b1d  Trac #32272: fix doctest

5a4c7ac  Trac #32272: narrow to finite dimensions

3b81085  Merge branch 't/32272/finitely_generated_graded_algebras_with_finite_degree' into t/29581/new_algorithm

ab288d6  Trac #29581: algorithm for complex vbundles

comment:29 Changed 4 months ago by
 Dependencies set to #32270, #32272
comment:30 Changed 4 months ago by
 Commit changed from ab288d62786a40b9f314d40ca932506d7b6932d9 to 6688740017097a79d7995101889cea097ee5c653
comment:31 Changed 4 months ago by
 Commit changed from 6688740017097a79d7995101889cea097ee5c653 to 61291f406b886abb9a509955355da749fc99a8d6
Branch pushed to git repo; I updated commit sha1. New commits:
61291f4  Trac #29581: minor fixes

comment:32 Changed 4 months ago by
 Commit changed from 61291f406b886abb9a509955355da749fc99a8d6 to 60318775e9c530c32280ba1bfa819cb9ac978bd1
Branch pushed to git repo; I updated commit sha1. New commits:
6031877  Trac #29581: return only list of generators

comment:33 Changed 4 months ago by
 Commit changed from 60318775e9c530c32280ba1bfa819cb9ac978bd1 to 0c7587f759b9555404fd417ee042e99fbb4ae02d
Branch pushed to git repo; I updated commit sha1. New commits:
0c7587f  Trac #29581: parent deals with different cases, element stays generic

comment:34 Changed 4 months ago by
 Commit changed from 0c7587f759b9555404fd417ee042e99fbb4ae02d to 28d2d84271a3e1516556a8364b1114dd6edaa992
Branch pushed to git repo; I updated commit sha1. New commits:
28d2d84  Trac #29581: revert mistaken merge

comment:35 Changed 4 months ago by
The ticket is not ready for review yet; the structure should be fixed though. Feedback at this stage is very much appreciated. :)
comment:36 Changed 4 months ago by
 Commit changed from 28d2d84271a3e1516556a8364b1114dd6edaa992 to 2cabd839d55e5c0f3ba2f8d0194bcae6ce31397c
Branch pushed to git repo; I updated commit sha1. New commits:
2cabd83  Trac #29581: add some comments

comment:37 Changed 4 months ago by
 Dependencies changed from #32270, #32272 to #32270, #32272, #32396
comment:38 Changed 4 months ago by
 Description modified (diff)
comment:39 Changed 4 months ago by
 Commit changed from 2cabd839d55e5c0f3ba2f8d0194bcae6ce31397c to 815392c4bcf2a6177cecbcc24ae3dc73372fe743
Branch pushed to git repo; I updated commit sha1. New commits:
1a9b7d9  Trac #29581: compute Euler forms for arbitrary positively oriented frames

fcd64c9  Trac #32396: add __abs__ to chart_func and scalarfield

6d70be4  Merge branch 'abs_function_for_scalar_fields' into t/29581/new_algorithm

815392c  Trac #29581: fix some bugs + treat Euler case

comment:40 Changed 4 months ago by
 Commit changed from 815392c4bcf2a6177cecbcc24ae3dc73372fe743 to 62ff48af88ee24979df68297551a540a47dbc1a3
Branch pushed to git repo; I updated commit sha1. New commits:
62ff48a  Trac #29581: leave comment on todo

comment:41 Changed 4 months ago by
 Commit changed from 62ff48af88ee24979df68297551a540a47dbc1a3 to 5b8632d7b5ac52bfe943660b26ffabe35b2c7a8f
Branch pushed to git repo; I updated commit sha1. New commits:
5b8632d  Trac #29581: euler form algorithm finished

comment:42 Changed 4 months ago by
 Commit changed from 5b8632d7b5ac52bfe943660b26ffabe35b2c7a8f to 4afc9f0d0ce1212516c67f6d04617151ef4d0ea3
Branch pushed to git repo; I updated commit sha1. New commits:
4afc9f0  Trac #29581: compute forms only on demand

comment:43 followup: ↓ 46 Changed 4 months ago by
I am particularly curious what you think about my last commit. I want that the corresponding characteristic forms are only computed if really necessary. This essentially only matters for the PontryaginEuler algorithm when the characteristic cohomology class contains no Euler or Pontryagin part.
Is there any lazystuff to make this a bit nicer maybe?
comment:44 Changed 4 months ago by
 Commit changed from 4afc9f0d0ce1212516c67f6d04617151ef4d0ea3 to 201831f1338973a6c5eefb6c9ee61592f9e38f0f
Branch pushed to git repo; I updated commit sha1. New commits:
201831f  Trac #29581: commenting improved

comment:45 Changed 4 months ago by
Since I have implemented fast_wedge_power
, which can be used separately of course, do we want a power method for differential forms or tensor fields respectively?
comment:46 in reply to: ↑ 43 Changed 4 months ago by
Replying to ghmjungmath:
I am particularly curious what you think about my last commit. I want that the corresponding characteristic forms are only computed if really necessary. This essentially only matters for the PontryaginEuler algorithm when the characteristic cohomology class contains no Euler or Pontryagin part.
You would have to know that ahead of time I think. Naïvely, it seems like you need to know the answer before you can tell if the answer is the form you want.
comment:47 Changed 4 months ago by
Well, what I need to know is already encoded in the characteristic cohomology class itself. For example, if it contains no Euler class generator, then I don't need to invoke EulerAlgorithm().get(nab)
. Like I said, this only matters for the "mixed" PontryaginEuler? case because in any other case I have to do the full computation anyway.
One reason why this is important: in the oddrank case, or for nonLeviCivita connections, this algorithm for the Euler class is not working. But one may still want to compute the Pontryagin classes.
The current commit makes sure of that by asking for the power of a certain generator and returning the one scalar field right away if the exponent is zero. This works, but it feels rather dirty to me, and I ponder whether there might be a nicer solution to this.
comment:48 Changed 4 months ago by
 Commit changed from 201831f1338973a6c5eefb6c9ee61592f9e38f0f to e17fe37d0ea5ecdd1e9932145b908037ed0261cd
Branch pushed to git repo; I updated commit sha1. New commits:
e17fe37  Trac #29581: comments further improved + _repr_ method

comment:49 followup: ↓ 50 Changed 4 months ago by
Unfortunately this is over my head mathematically, so I don't think I can really help with possible solutions at this point.
comment:50 in reply to: ↑ 49 ; followup: ↓ 52 Changed 4 months ago by
Replying to tscrim:
Unfortunately this is over my head mathematically, so I don't think I can really help with possible solutions at this point.
Mh okay. If you like, we can talk about it privately at some point.
I have another question though. I tried to establish a coercion from the characteristic cohomology class ring to the de Rham cohomology ring. For that, I need a representative which I only get if I've inserted a connection. Now I've read that coercions must always suceed. So throwing an error if no representative is available is not an option then?
Otherwise, we could return the same instance of CharacteristicCohomologyClass
if we pass it to the _element_constructor_
of DeRhamCohomologyRing
. But I suppose that is against the coercion principles either, right?
comment:51 Changed 4 months ago by
 Commit changed from e17fe37d0ea5ecdd1e9932145b908037ed0261cd to 8e3263c69f8099d8dcbf9faf07999304642c7222
Branch pushed to git repo; I updated commit sha1. New commits:
8e3263c  Trac #29581: deprecate alias and more

comment:52 in reply to: ↑ 50 ; followup: ↓ 65 Changed 3 months ago by
Replying to ghmjungmath:
Replying to tscrim:
Unfortunately this is over my head mathematically, so I don't think I can really help with possible solutions at this point.
Mh okay. If you like, we can talk about it privately at some point.
If you are still stuck with something, perhaps we can. Perhaps Éric can offer some wisdom here.
I have another question though. I tried to establish a coercion from the characteristic cohomology class ring to the de Rham cohomology ring. For that, I need a representative which I only get if I've inserted a connection. Now I've read that coercions must always suceed. So throwing an error if no representative is available is not an option then?
Probably. Although you can say there is a coercion, but the data used to initialize it has not been specified. However, this map must also be unique too (in a mathematical sense; I think different representatives for the same element is fine), which means you cannot change the connection. This might be a place where we can justify having an exception because of the surrounding framework, but it is likely better to avoid that and make the user explicitly ask for a conversion.
Otherwise, we could return the same instance of
CharacteristicCohomologyClass
if we pass it to the_element_constructor_
ofDeRhamCohomologyRing
. But I suppose that is against the coercion principles either, right?
If we are passing it to the _element_cosntructor_
, then it is a conversion, and there are essentially no rules at that point.
comment:53 Changed 3 months ago by
Thank you Travis for your comments. My original idea was to inherit from DeRhamCohomologyClass
. Because both classes have the representative
method in common, which is crucial to perform operations such as additions and cupproducts. My "dream" would be that an element of CharacteristicCohomologyClassRing
can be seen as member of DeRhamCohomologyRing
, and whenever representative
is invoked (even after coercions), you can still choose a connection.
One way to achieve this could be to allow _representative
to refer to a method. But I suppose this is rather spurious for the user?
Alternatively, if we drop my "dream" and still allow coercion: we can modify DeRhamCohomologyClass
in such a way that we can give it a name and allow to leave the _representative
attribute empty. Similar to scalar fields whose expressions are not set yet. Afterwards, the representative can be set dynamically, and we can make the instance immutable if desired.
What would you prefer Travis? Eric, do you have an opinion, too?
comment:54 Changed 3 months ago by
 Commit changed from 8e3263c69f8099d8dcbf9faf07999304642c7222 to 74d324f37e20c80488d38f1f4e8c22c0a3e02108
Branch pushed to git repo; I updated commit sha1. New commits:
74d324f  Trac #29851: add sequences

comment:55 Changed 3 months ago by
 Commit changed from 74d324f37e20c80488d38f1f4e8c22c0a3e02108 to 1f51ca73c5e5ab0ac1a5e5361b1100198c765dd1
Branch pushed to git repo; I updated commit sha1. New commits:
1f51ca7  Trac #29851: sequences as static functions

comment:56 Changed 3 months ago by
 Commit changed from 1f51ca73c5e5ab0ac1a5e5361b1100198c765dd1 to ffa79bb0e94248699da3b73946310aa8eefa9516
Branch pushed to git repo; I updated commit sha1. New commits:
ffa79bb  Trac #29851: improve doc

comment:57 Changed 3 months ago by
 Commit changed from ffa79bb0e94248699da3b73946310aa8eefa9516 to 54cf11fa0f701f76523b1da5bf0e1434d9ab1723
Branch pushed to git repo; I updated commit sha1. New commits:
54cf11f  Trac #29851: factory class

comment:58 Changed 3 months ago by
 Commit changed from 54cf11fa0f701f76523b1da5bf0e1434d9ab1723 to d0f14dc7caeb6fb7a225d73939af6f5a95e06ca7
comment:59 Changed 3 months ago by
I think I made the constructor class CharacteristicCohomologyClass
unnecessarily complicated. Do you have an idea to make it more clean?
Otherwise, the main parts should be finished except for coercion support w.r.t. de Rham cohomology (see comment:53).
comment:60 Changed 3 months ago by
 Commit changed from d0f14dc7caeb6fb7a225d73939af6f5a95e06ca7 to 4b371bd075fc8c82328cffeff08c7cee91bfa9b5
Branch pushed to git repo; I updated commit sha1. New commits:
4b371bd  Trac #29581: add AHat class + minor fixes

comment:61 Changed 3 months ago by
 Commit changed from 4b371bd075fc8c82328cffeff08c7cee91bfa9b5 to e929409c7709091b1d3627614b7042f7b432274e
Branch pushed to git repo; I updated commit sha1. New commits:
e929409  Trac #29581: remove cached_function due to dynamic behavior w.r.t. has_orientation

comment:62 Changed 3 months ago by
 Commit changed from e929409c7709091b1d3627614b7042f7b432274e to 4b371bd075fc8c82328cffeff08c7cee91bfa9b5
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
comment:63 Changed 3 months ago by
 Commit changed from 4b371bd075fc8c82328cffeff08c7cee91bfa9b5 to e82168f67b13aec1f32208eef2a2bf17b7e6fc04
Branch pushed to git repo; I updated commit sha1. New commits:
e82168f  Trac #29581: minor improvements

comment:64 Changed 3 months ago by
 Commit changed from e82168f67b13aec1f32208eef2a2bf17b7e6fc04 to 9f53f25b684540bf08e1d64d8a31b9420ebcce25
Branch pushed to git repo; I updated commit sha1. New commits:
9f53f25  Trac #29581: adapt names

comment:65 in reply to: ↑ 52 Changed 3 months ago by
Replying to tscrim:
Probably. Although you can say there is a coercion, but the data used to initialize it has not been specified. However, this map must also be unique too (in a mathematical sense; I think different representatives for the same element is fine), which means you cannot change the connection.
Wait. The cohomology class is independent of the connection. It just gives different representatives. Then it should be fine.
comment:66 Changed 3 months ago by
 Commit changed from 9f53f25b684540bf08e1d64d8a31b9420ebcce25 to 86b4d28a2aceb68e36b6fed8ca4ee1d8dcbc754c
Branch pushed to git repo; I updated commit sha1. New commits:
86b4d28  Trac #29581: add coercion and further improvements

comment:67 Changed 3 months ago by
 Commit changed from 86b4d28a2aceb68e36b6fed8ca4ee1d8dcbc754c to f81b25d80d4b0b9c692b25af037f1fe7623e96e8
Branch pushed to git repo; I updated commit sha1. New commits:
f81b25d  Trac #39581: add example for coercion

comment:68 Changed 3 months ago by
 Commit changed from f81b25d80d4b0b9c692b25af037f1fe7623e96e8 to 69dd5301343ea310bbdec34b65585a837c18be1b
Branch pushed to git repo; I updated commit sha1. New commits:
69dd530  Trac #39581: improve doc

comment:69 Changed 3 months ago by
 Commit changed from 69dd5301343ea310bbdec34b65585a837c18be1b to 00203e394d5afd7b61802a39dcf6cde13f73efb7
Branch pushed to git repo; I updated commit sha1. New commits:
7328d50  Trac #32272: remove unused imports

970d295  Trac #30473: unicode > latex for cup product

5021707  Merge branch 't/32272/finitely_generated_graded_algebras_with_finite_degree' into t/29581/new_algorithm

00203e3  Trac #29581: use cupproduct for mul_symbol

comment:70 Changed 3 months ago by
 Commit changed from 00203e394d5afd7b61802a39dcf6cde13f73efb7 to 7555772e7b26039de4796997cd6f812a287ba49e
Branch pushed to git repo; I updated commit sha1. New commits:
7555772  Trac #29581: copy doc from old file + add some doc for CharacteristicCohomologyRing

comment:71 Changed 3 months ago by
 Commit changed from 7555772e7b26039de4796997cd6f812a287ba49e to 5f542e15b88799d5e37df351d32a6389e237e913
Branch pushed to git repo; I updated commit sha1. New commits:
5f542e1  Trac #29581: improve doc CharacteristicCohomologyClassRing

comment:72 Changed 3 months ago by
 Commit changed from 5f542e15b88799d5e37df351d32a6389e237e913 to 0ca6eb9fac729f9cfe68f61a5a41bf8e3d1f6504
Branch pushed to git repo; I updated commit sha1. New commits:
0ca6eb9  Trac #29581: add doc to sequences

comment:73 Changed 3 months ago by
 Commit changed from 0ca6eb9fac729f9cfe68f61a5a41bf8e3d1f6504 to e7169f81d27242663c168350b203c2d8d51861ef
Branch pushed to git repo; I updated commit sha1. New commits:
e7169f8  Trac #29581: doc of fast_wedge_power

comment:74 Changed 3 months ago by
 Commit changed from e7169f81d27242663c168350b203c2d8d51861ef to 114b483325526810c504935048e87b1e813985df
Branch pushed to git repo; I updated commit sha1. New commits:
114b483  Trac #29581: add more docstring

comment:75 Changed 3 months ago by
 Commit changed from 114b483325526810c504935048e87b1e813985df to bafd78dcbb8e5b385051d64cc0a68a2e14908ad6
Branch pushed to git repo; I updated commit sha1. New commits:
bafd78d  Trac #29581: remove old file + add more doc

comment:76 Changed 3 months ago by
 Commit changed from bafd78dcbb8e5b385051d64cc0a68a2e14908ad6 to 265f918b8045a6b93142130f9fa4147fc7bd1f4c
Branch pushed to git repo; I updated commit sha1. New commits:
265f918  Trac #29581: fix sign mistake

comment:77 Changed 3 months ago by
 Commit changed from 265f918b8045a6b93142130f9fa4147fc7bd1f4c to a3555e13332e0879adae188179120f4311462c6e
Branch pushed to git repo; I updated commit sha1. New commits:
a3555e1  Trac #29581: more docstring

comment:78 Changed 3 months ago by
 Commit changed from a3555e13332e0879adae188179120f4311462c6e to 939cfd9d21342527dc0442f653dc50791ce73ad8
Branch pushed to git repo; I updated commit sha1. New commits:
939cfd9  Trac #29581: more doc EulerAlgorithm

comment:79 Changed 3 months ago by
 Commit changed from 939cfd9d21342527dc0442f653dc50791ce73ad8 to e8d8393314a45980c25d110734a57ac4bda64191
Branch pushed to git repo; I updated commit sha1. New commits:
e8d8393  Trac #29581: correct doc for EulerAlgorithm

comment:80 Changed 3 months ago by
 Commit changed from e8d8393314a45980c25d110734a57ac4bda64191 to 974c3cc866d51d884de7b666335a8c5d36517666
Branch pushed to git repo; I updated commit sha1. New commits:
974c3cc  Trac #29851: finish doc so far

comment:81 Changed 3 months ago by
 Cc ghtobiasdiez added
 Status changed from needs_info to needs_review
Ticket should be finally ready for review now. :)
comment:82 Changed 3 months ago by
By the way, comparing wall times within the notebook https://nbviewer.jupyter.org/github/sagemanifolds/SageManifolds/blob/master/Notebooks/SM_char_classes.ipynb yields very good results!
comment:83 Changed 3 months ago by
 Commit changed from 974c3cc866d51d884de7b666335a8c5d36517666 to 5830550c072e7ba16215dfe80e3985cee9e7d320
Branch pushed to git repo; I updated commit sha1. New commits:
5830550  Trac #29581: fix :: mistake and add Singleton example

comment:84 Changed 3 months ago by
Patchbot is morally green.
comment:85 Changed 3 months ago by
 Commit changed from 5830550c072e7ba16215dfe80e3985cee9e7d320 to 9a73cad1f5c1504b62c1890b28cc12c753cf9b23
Branch pushed to git repo; I updated commit sha1. New commits:
9a73cad  Trac #29581: correct ideal in doc + add long time tags for 30s or more

comment:86 Changed 3 months ago by
 Commit changed from 9a73cad1f5c1504b62c1890b28cc12c753cf9b23 to 4c1bc44977bbf506a415c9635bf884ae0519e229
Branch pushed to git repo; I updated commit sha1. New commits:
4c1bc44  Trac #29581: remove fstring without placeholder

comment:87 Changed 3 months ago by
 Commit changed from 4c1bc44977bbf506a415c9635bf884ae0519e229 to e8e0a0b3040b3a2e750eec48cf7ffbd4d3cfb65f
Branch pushed to git repo; I updated commit sha1. New commits:
e8e0a0b  Merge branch 'develop' into t/29581/new_algorithm

comment:88 followup: ↓ 90 Changed 3 months ago by
 Reviewers set to Travis Scrimshaw
Since CharacteristicCohomologyClass
is not a function nor userfacing, you should follow Python naming conventions and write it as characteristic_cohomology_class
, perhaps adding build_
in front. However, I think you are better off tying that to the ring, perhaps as a (cached) helper method. This ties the cache of classes to the lifecycle of the ring rather than globally (which also nails the rings in memory) and better links everything from a codeassociation perspective.
This isn't necessarily specific to this ticket (but maybe now with these changes this is now possible), but do we have an example where the entire cohomology ring of a manifold is computed, such as the sphere or torus?
Perhaps as a followup ticket, a thematic tutorial on how to compute with the cohomology classes of the manifold might be good (using the modulelevel doc as a starting point).
comment:89 Changed 3 months ago by
 Commit changed from e8e0a0b3040b3a2e750eec48cf7ffbd4d3cfb65f to ec282634db5fbb61c7731ba8998c356814887f70
Branch pushed to git repo; I updated commit sha1. New commits:
ec28263  Trac #29581: tie construuctor to ring

comment:90 in reply to: ↑ 88 ; followup: ↓ 92 Changed 3 months ago by
Replying to tscrim:
Since
CharacteristicCohomologyClass
is not a function nor userfacing, you should follow Python naming conventions and write it ascharacteristic_cohomology_class
, perhaps addingbuild_
in front. However, I think you are better off tying that to the ring, perhaps as a (cached) helper method. This ties the cache of classes to the lifecycle of the ring rather than globally (which also nails the rings in memory) and better links everything from a codeassociation perspective.
Thank you Travis for this feedback. I also tied it to the element constructor now. Is that what you had in mind?
This isn't necessarily specific to this ticket (but maybe now with these changes this is now possible), but do we have an example where the entire cohomology ring of a manifold is computed, such as the sphere or torus?
Take for example CP^1 = S^2
whose 2nd cohomology is generated by the first Chern class of the tautological bundle. But this is a rather special example.
Perhaps as a followup ticket, a thematic tutorial on how to compute with the cohomology classes of the manifold might be good (using the modulelevel doc as a starting point).
modulelevel doc?
comment:91 Changed 3 months ago by
 Commit changed from ec282634db5fbb61c7731ba8998c356814887f70 to c5370ec74eb85fd267c558ea8bad0e74512402b1
Branch pushed to git repo; I updated commit sha1. New commits:
c5370ec  Trac #29581: remove line breaks due to hard wrap in editor

comment:92 in reply to: ↑ 90 ; followup: ↓ 93 Changed 3 months ago by
Replying to ghmjungmath:
Replying to tscrim:
Since
CharacteristicCohomologyClass
is not a function nor userfacing, you should follow Python naming conventions and write it ascharacteristic_cohomology_class
, perhaps addingbuild_
in front. However, I think you are better off tying that to the ring, perhaps as a (cached) helper method. This ties the cache of classes to the lifecycle of the ring rather than globally (which also nails the rings in memory) and better links everything from a codeassociation perspective.Thank you Travis for this feedback. I also tied it to the element constructor now. Is that what you had in mind?
Yea, that looks good. Thank you.
This isn't necessarily specific to this ticket (but maybe now with these changes this is now possible), but do we have an example where the entire cohomology ring of a manifold is computed, such as the sphere or torus?
Take for example
CP^1 = S^2
whose 2nd cohomology is generated by the first Chern class of the tautological bundle. But this is a rather special example.
Perhaps that is a bit special. The torus is a little more interesting example. Mainly what I am thinking is if can we compute the Betti numbers without necessarily knowing what the manifold is in advance.
Perhaps as a followup ticket, a thematic tutorial on how to compute with the cohomology classes of the manifold might be good (using the modulelevel doc as a starting point).
modulelevel doc?
The documentation at the top of the characteristic_cohomology_class.py
file (a Python module).
comment:93 in reply to: ↑ 92 ; followup: ↓ 94 Changed 3 months ago by
Replying to tscrim:
Yea, that looks good. Thank you.
So, what's missing for a positive review then?
Perhaps that is a bit special. The torus is a little more interesting example. Mainly what I am thinking is if can we compute the Betti numbers without necessarily knowing what the manifold is in advance.
I don't see how that should be possible with this implementation. Characteristic cohomology classes usually don't tell you anything about the cohomology of your underlying base space. For once, the characteristic cohomology classes in general don't generate the cohomology groups. Moreover, if a characteristic cohomology class has torsion in the ZZcohomology, its corresponding class in the de Rham cohomology vanishes. So you lose a lot of information on the way.
Perhaps as a followup ticket, a thematic tutorial on how to compute with the cohomology classes of the manifold might be good (using the modulelevel doc as a starting point).
modulelevel doc?
The documentation at the top of the
characteristic_cohomology_class.py
file (a Python module).
Right. But this is indeed something for a followup. However, don't you think this fits better into a general tutorial?
comment:94 in reply to: ↑ 93 Changed 3 months ago by
 Status changed from needs_review to positive_review
Replying to ghmjungmath:
Replying to tscrim:
Yea, that looks good. Thank you.
So, what's missing for a positive review then?
You've answered all of my other questions, so nothing now. Thank you.
Perhaps that is a bit special. The torus is a little more interesting example. Mainly what I am thinking is if can we compute the Betti numbers without necessarily knowing what the manifold is in advance.
I don't see how that should be possible with this implementation. Characteristic cohomology classes usually don't tell you anything about the cohomology of your underlying base space. For once, the characteristic cohomology classes in general don't generate the cohomology groups. Moreover, if a characteristic cohomology class has torsion in the ZZcohomology, its corresponding class in the de Rham cohomology vanishes. So you lose a lot of information on the way.
Okay. Thank you for the explanation.
Perhaps as a followup ticket, a thematic tutorial on how to compute with the cohomology classes of the manifold might be good (using the modulelevel doc as a starting point).
Right. But this is indeed something for a followup. However, don't you think this fits better into a general tutorial?
I agree that it should be a followup. Yet, it is a bit specialized, so I think a thematic tutorial is better.
comment:95 Changed 3 months ago by
Thank you for the review Travis! :)
comment:97 Changed 2 months ago by
 Commit changed from c5370ec74eb85fd267c558ea8bad0e74512402b1 to 7777803bf6c305a8ed117972c8f08495aef90b7e
Branch pushed to git repo; I updated commit sha1. New commits:
7777803  Merge branch 'develop' into t/29581/new_algorithm

comment:98 Changed 2 months ago by
I have no idea what caused the merge conflict. I hope it works now...
comment:99 Changed 2 months ago by
 Status changed from needs_work to needs_review
comment:100 Changed 2 months ago by
 Status changed from needs_review to positive_review
comment:101 Changed 2 months ago by
 Status changed from positive_review to needs_work
Sorry to interfere here, but the patchbot says
sage t long randomseed=0 src/sage/manifolds/differentiable/characteristic_cohomology_class.py # 9 doctests failed
comment:102 Changed 2 months ago by
Looks like an import location changed:
ImportError: cannot import name 'I' from 'sage.libs.pynac.pynac'
comment:103 followup: ↓ 104 Changed 2 months ago by
This is probably due to #32386. How do I import the imaginary unit now?
comment:104 in reply to: ↑ 103 ; followup: ↓ 108 Changed 2 months ago by
Replying to ghmjungmath:
This is probably due to #32386. How do I import the imaginary unit now?
sage: import_statements("I")
says
from sage.rings.imaginary_unit import I
Indeed:
sage: from sage.rings.imaginary_unit import I as I2 sage: I2 is I True
comment:105 Changed 2 months ago by
 Commit changed from 7777803bf6c305a8ed117972c8f08495aef90b7e to 4e2e8b85ee02476a51574d1e04e87b0e6c0eaa00
Branch pushed to git repo; I updated commit sha1. New commits:
4e2e8b8  Trac #29581: new location of imaginary unit

comment:106 Changed 2 months ago by
 Status changed from needs_work to needs_review
Here we go. Let's hope this works now.
New commits:
4e2e8b8  Trac #29581: new location of imaginary unit

comment:107 followup: ↓ 109 Changed 2 months ago by
From #32386, they made this change:
from sage.libs.pynac.pynac import I +from sage.symbolic.expression import I
comment:108 in reply to: ↑ 104 Changed 2 months ago by
Replying to egourgoulhon:
Replying to ghmjungmath:
This is probably due to #32386. How do I import the imaginary unit now?
sage: import_statements("I")
saysfrom sage.rings.imaginary_unit import IIndeed:
sage: from sage.rings.imaginary_unit import I as I2 sage: I2 is I True
BTW, I've just noticed that
sage: from sage.symbolic.constants import I as I3 sage: I3 is I False
However,
sage: bool(I3 == I) True sage: I3^2 1 sage: I + I3 2*I
comment:109 in reply to: ↑ 107 Changed 2 months ago by
comment:110 Changed 2 months ago by
To summarize, there are at least 3 sources for I
: sage.rings.imaginary_unit
, sage.symbolic.constants
, sage.symbolic.expression
, but only the first one gives the same I
as the Python variable predefined in the interactive shell. Moreover, we have
sage: I.parent() Number Field in I with defining polynomial x^2 + 1 with I = 1*I sage: from sage.symbolic.constants import I as I1 sage: I1.parent() Symbolic Ring sage: from sage.symbolic.expression import I as I2 sage: I2.parent() Symbolic Ring sage: I2 is I1 True
comment:111 Changed 2 months ago by
But sage.symbolic.constants
and sage.symbolic.expression
give the same I
, don't they?
comment:112 Changed 2 months ago by
#18036 is probably relevant here.
comment:113 followup: ↓ 114 Changed 2 months ago by
I think you want to follow the change from #32386 since you are wanting to do symbolic ring computations. Otherwise you end up with
sage: I.parent() Number Field in I with defining polynomial x^2 + 1 with I = 1*I
comment:114 in reply to: ↑ 113 Changed 2 months ago by
Replying to tscrim:
I think you want to follow the change from #32386 since you are wanting to do symbolic ring computations. Otherwise you end up with
sage: I.parent() Number Field in I with defining polynomial x^2 + 1 with I = 1*I
In src/sage/symbolic/all.py
there is a deprecation warning against from sage.symbolic.expression import I
:
lazy_import("sage.symbolic.expression", "I", deprecation=(18036, "import I from sage.symbolic.constants for the imaginary unit viewed as an element of SR, or from sage.rings.imaginary_unit for the element of ZZ[i]"))
However, this warning does not seem effective...
comment:115 Changed 2 months ago by
In a fresh Sage session, we have (since #18036 I guess)
sage: I.parent() Number Field in I with defining polynomial x^2 + 1 with I = 1*I sage: from sage.symbolic.constants import I as I_symb sage: I_symb.parent() Symbolic Ring sage: SR(I) is I_symb False
I would have expected True
for the last output (i.e. to have I
unique as a symbolic expression).
comment:116 Changed 2 months ago by
It's not so unnatural to have copies being made. It would also be a lot of work for SR to check that some potentially very complicated expression is equal to I
.
Okay, then follow the deprecation and import from sage.symbolic.constants
. The main thing is to not use the one from from sage.rings.imaginary_unit import I
.
comment:117 Changed 2 months ago by
 Commit changed from 4e2e8b85ee02476a51574d1e04e87b0e6c0eaa00 to 44f8cfa8d960c1ec2db3a097f95728efcc86fcf4
Branch pushed to git repo; I updated commit sha1. New commits:
44f8cfa  Trac #29581: import I from symbolic.constants

comment:118 Changed 2 months ago by
Okay, there we go. Thank you for this feedback!
comment:119 Changed 2 months ago by
 Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Eric Gourgoulhon
comment:120 Changed 2 months ago by
 Status changed from needs_review to positive_review
Green bot. LGTM.
comment:121 Changed 2 months ago by
 Status changed from positive_review to needs_work
[docpdf] ! Package inputenc Error: Unicode character ϑ (U+03D1) [docpdf] (inputenc) not set up for use with LaTeX. [docpdf] [docpdf] See the inputenc package documentation for explanation. [docpdf] Type H <return> for immediate help. [docpdf] ... [docpdf] [docpdf] l.73283 ...1}{\PYGZsh{} use spherical coordinates} [docpdf] [docpdf] ? [docpdf] ! Emergency stop.
comment:122 Changed 2 months ago by
Indeed, any new Unicode character has to be declared in
src/sage/docs/conf.py
for the pdf doc to be generated successfully.
comment:123 Changed 2 months ago by
 Commit changed from 44f8cfa8d960c1ec2db3a097f95728efcc86fcf4 to ac5cbbf0072380022c32466aab95fdfb31c4ecee
Branch pushed to git repo; I updated commit sha1. New commits:
ac5cbbf  Trac #29581: remove unicode character

comment:124 Changed 2 months ago by
 Status changed from needs_work to needs_review
That should solve it. Hopefully.
comment:125 Changed 2 months ago by
 Status changed from needs_review to positive_review
That is one way to solve it.
comment:126 Changed 2 months ago by
 Branch changed from u/ghmjungmath/new_algorithm to ac5cbbf0072380022c32466aab95fdfb31c4ecee
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Trac #29570: alternating_form returns correct element
Trac #29570: Typo fixed, doctest added, returned element preferably nonzero
Trac #29581: New Algorithm for Char Classes