Opened 18 months ago

Closed 12 days ago

#29581 closed enhancement (fixed)

New Algorithm for Characteristic Classes

Reported by: gh-mjungmath Owned by:
Priority: major Milestone: sage-9.5
Component: manifolds Keywords: characteristic_classes, manifolds
Cc: slelievre, egourgoulhon, tscrim, mkoeppe, gh-tobiasdiez 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:

Status badges

Description (last modified by gh-mjungmath)

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 sub-ring 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 Faddeev-LeVerrier?-like algorithm (cf. #30681).

Change History (126)

comment:1 Changed 18 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/new_algorithm

comment:2 Changed 18 months ago by mkoeppe

  • Commit set to 1a803eba5b9724ebfa93e726a474d957fd6915b4
  • Milestone changed from sage-9.1 to sage-9.2

New commits:

3b36f0dTrac #29570: alternating_form returns correct element
85e7970Trac #29570: Typo fixed, doctest added, returned element preferably non-zero
1a803ebTrac #29581: New Algorithm for Char Classes

comment:3 Changed 18 months ago by slelievre

  • Cc slelievre added
  • Component changed from PLEASE CHANGE to geometry

comment:4 Changed 18 months ago by slelievre

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 18 months ago by git

  • Commit changed from 1a803eba5b9724ebfa93e726a474d957fd6915b4 to cb442eb8919ca33a5db63a17bac52f99f55553dd

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

730046cMerge branch 'develop' into t/29570/diff_form_bug
130ae3fTrac #29570: NotImplementedError for non-generic ring elements
bba21e1Trac #29570: Strange typo fixed...
ba3b4b9Trac #29570: correct parent, vectorfield_module changes reverted
841e1bfMerge branch 't/29570/diff_form_bug' into t/29581/new_algorithm
cb442ebTrac #29570: new algorithm added and code cleaned

comment:6 Changed 18 months ago by git

  • Commit changed from cb442eb8919ca33a5db63a17bac52f99f55553dd to 15c7341d071327a04fd229c3d2f78d583564f476

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

15c7341Trac #29581: new algorithm added and code cleaned

comment:7 Changed 18 months ago by gh-mjungmath

  • Authors set to Michael Jung
  • Cc egourgoulhon added
  • Description modified (diff)
  • Keywords characteristic_classes manifolds added
  • Type changed from PLEASE CHANGE to enhancement

comment:8 Changed 18 months ago by chapoton

Correct syntax for crosslinks is

.. SEEALSO::

comment:9 Changed 18 months ago by chapoton

  • 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 18 months ago by gh-mjungmath

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 18 months ago by gh-mjungmath

Thanks for your advice. :)

comment:12 Changed 18 months ago by git

  • Commit changed from 15c7341d071327a04fd229c3d2f78d583564f476 to d37229341effff46ea2fa4b3287e32997c8422b0

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

d372293Trac #29581: Strange typos reverted

comment:13 Changed 18 months ago by gh-mjungmath

  • Status changed from new to needs_review

comment:14 Changed 18 months ago by gh-mjungmath

  • Status changed from needs_review to needs_info

comment:15 Changed 18 months ago by gh-mjungmath

An example for the application of the new algorithm would be really nice. For instance a Todd class or A-Hat 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 18 months ago by git

  • Commit changed from d37229341effff46ea2fa4b3287e32997c8422b0 to 19815c4257cee7dcf503d94e3b9bb651150bef5f

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

c975bd2Merge branch 'develop' into t/29581/new_algorithm
19815c4Trac #29581: Readability of one line

comment:17 Changed 15 months ago by gh-mjungmath

  • Component changed from geometry to manifolds
  • Dependencies set to #30211

comment:18 Changed 12 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:19 Changed 7 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

comment:20 Changed 3 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

Setting a new milestone for this ticket based on a cursory review.

comment:21 Changed 3 months ago by gh-mjungmath

  • Cc tscrim mkoeppe added
  • Dependencies #30211 deleted
  • Description modified (diff)
  • Status changed from needs_info to needs_work

comment:22 Changed 3 months ago by git

  • Commit changed from 19815c4257cee7dcf503d94e3b9bb651150bef5f to 3c9b80a2551994493f23640344e9ade63240cb01

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

3c9b80aTrac #29581: implement algorithms for chern, pontryagin and euler

comment:23 Changed 3 months ago by gh-mjungmath

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 3 months ago by gh-mjungmath

  • Description modified (diff)

comment:25 Changed 3 months ago by gh-mjungmath

  • Status changed from needs_work to needs_info

comment:26 Changed 3 months ago by gh-mjungmath

  • Description modified (diff)

comment:27 Changed 3 months ago by git

  • Commit changed from 3c9b80a2551994493f23640344e9ade63240cb01 to 03529f8919357f98b65ee033e8dc5c7d48b2fa42

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

03529f8Trac #29581: return mixed form

comment:28 Changed 2 months ago by git

  • 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
de5863aMerge branch 't/32270/turn_de_rham_cohomology_into_algebra' into t/29581/new_algorithm
d22ee9aTrac #32315: support iteration and enumerated sets
16c4737Trac #32272: allow infinite max degree
6a320a0Trac #32272: fix docstring
c1d4fceTrac #32272: fix rst file + rename module
25e6b1dTrac #32272: fix doctest
5a4c7acTrac #32272: narrow to finite dimensions
3b81085Merge branch 't/32272/finitely_generated_graded_algebras_with_finite_degree' into t/29581/new_algorithm
ab288d6Trac #29581: algorithm for complex vbundles

comment:29 Changed 2 months ago by gh-mjungmath

  • Dependencies set to #32270, #32272

comment:30 Changed 2 months ago by git

  • Commit changed from ab288d62786a40b9f314d40ca932506d7b6932d9 to 6688740017097a79d7995101889cea097ee5c653

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

932e128Trac #29581: element constructor
6688740Trac #29581: naming

comment:31 Changed 2 months ago by git

  • Commit changed from 6688740017097a79d7995101889cea097ee5c653 to 61291f406b886abb9a509955355da749fc99a8d6

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

61291f4Trac #29581: minor fixes

comment:32 Changed 2 months ago by git

  • Commit changed from 61291f406b886abb9a509955355da749fc99a8d6 to 60318775e9c530c32280ba1bfa819cb9ac978bd1

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

6031877Trac #29581: return only list of generators

comment:33 Changed 2 months ago by git

  • Commit changed from 60318775e9c530c32280ba1bfa819cb9ac978bd1 to 0c7587f759b9555404fd417ee042e99fbb4ae02d

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

0c7587fTrac #29581: parent deals with different cases, element stays generic

comment:34 Changed 2 months ago by git

  • Commit changed from 0c7587f759b9555404fd417ee042e99fbb4ae02d to 28d2d84271a3e1516556a8364b1114dd6edaa992

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

28d2d84Trac #29581: revert mistaken merge

comment:35 Changed 2 months ago by gh-mjungmath

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 2 months ago by git

  • Commit changed from 28d2d84271a3e1516556a8364b1114dd6edaa992 to 2cabd839d55e5c0f3ba2f8d0194bcae6ce31397c

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

2cabd83Trac #29581: add some comments

comment:37 Changed 2 months ago by gh-mjungmath

  • Dependencies changed from #32270, #32272 to #32270, #32272, #32396

comment:38 Changed 2 months ago by gh-mjungmath

  • Description modified (diff)

comment:39 Changed 2 months ago by git

  • Commit changed from 2cabd839d55e5c0f3ba2f8d0194bcae6ce31397c to 815392c4bcf2a6177cecbcc24ae3dc73372fe743

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

1a9b7d9Trac #29581: compute Euler forms for arbitrary positively oriented frames
fcd64c9Trac #32396: add __abs__ to chart_func and scalarfield
6d70be4Merge branch 'abs_function_for_scalar_fields' into t/29581/new_algorithm
815392cTrac #29581: fix some bugs + treat Euler case

comment:40 Changed 2 months ago by git

  • Commit changed from 815392c4bcf2a6177cecbcc24ae3dc73372fe743 to 62ff48af88ee24979df68297551a540a47dbc1a3

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

62ff48aTrac #29581: leave comment on to-do

comment:41 Changed 2 months ago by git

  • Commit changed from 62ff48af88ee24979df68297551a540a47dbc1a3 to 5b8632d7b5ac52bfe943660b26ffabe35b2c7a8f

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

5b8632dTrac #29581: euler form algorithm finished

comment:42 Changed 2 months ago by git

  • Commit changed from 5b8632d7b5ac52bfe943660b26ffabe35b2c7a8f to 4afc9f0d0ce1212516c67f6d04617151ef4d0ea3

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

4afc9f0Trac #29581: compute forms only on demand

comment:43 follow-up: Changed 2 months ago by gh-mjungmath

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 Pontryagin-Euler algorithm when the characteristic cohomology class contains no Euler or Pontryagin part.

Is there any lazy-stuff to make this a bit nicer maybe?

comment:44 Changed 2 months ago by git

  • Commit changed from 4afc9f0d0ce1212516c67f6d04617151ef4d0ea3 to 201831f1338973a6c5eefb6c9ee61592f9e38f0f

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

201831fTrac #29581: commenting improved

comment:45 Changed 2 months ago by gh-mjungmath

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 2 months ago by tscrim

Replying to gh-mjungmath:

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 Pontryagin-Euler 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 2 months ago by gh-mjungmath

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 odd-rank case, or for non-Levi-Civita 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.

Last edited 2 months ago by gh-mjungmath (previous) (diff)

comment:48 Changed 2 months ago by git

  • Commit changed from 201831f1338973a6c5eefb6c9ee61592f9e38f0f to e17fe37d0ea5ecdd1e9932145b908037ed0261cd

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

e17fe37Trac #29581: comments further improved + _repr_ method

comment:49 follow-up: Changed 2 months ago by tscrim

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 ; follow-up: Changed 2 months ago by gh-mjungmath

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 2 months ago by git

  • Commit changed from e17fe37d0ea5ecdd1e9932145b908037ed0261cd to 8e3263c69f8099d8dcbf9faf07999304642c7222

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

8e3263cTrac #29581: deprecate alias and more

comment:52 in reply to: ↑ 50 ; follow-up: Changed 2 months ago by tscrim

Replying to gh-mjungmath:

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_ of DeRhamCohomologyRing. 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 2 months ago by gh-mjungmath

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 cup-products. 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 the attribute _representative in DeRhamCohomologyClass 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?

Last edited 2 months ago by gh-mjungmath (previous) (diff)

comment:54 Changed 8 weeks ago by git

  • Commit changed from 8e3263c69f8099d8dcbf9faf07999304642c7222 to 74d324f37e20c80488d38f1f4e8c22c0a3e02108

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

74d324fTrac #29851: add sequences

comment:55 Changed 8 weeks ago by git

  • Commit changed from 74d324f37e20c80488d38f1f4e8c22c0a3e02108 to 1f51ca73c5e5ab0ac1a5e5361b1100198c765dd1

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

1f51ca7Trac #29851: sequences as static functions

comment:56 Changed 8 weeks ago by git

  • Commit changed from 1f51ca73c5e5ab0ac1a5e5361b1100198c765dd1 to ffa79bb0e94248699da3b73946310aa8eefa9516

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

ffa79bbTrac #29851: improve doc

comment:57 Changed 7 weeks ago by git

  • Commit changed from ffa79bb0e94248699da3b73946310aa8eefa9516 to 54cf11fa0f701f76523b1da5bf0e1434d9ab1723

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

54cf11fTrac #29851: factory class

comment:58 Changed 7 weeks ago by git

  • Commit changed from 54cf11fa0f701f76523b1da5bf0e1434d9ab1723 to d0f14dc7caeb6fb7a225d73939af6f5a95e06ca7

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

ba71d70Trac #29581: turn polynomial into dictionary
5c0f165Merge branch 'develop' into t/29581/new_algorithm
d0f14dcTrac #29581: constructor class finished

comment:59 Changed 7 weeks ago by gh-mjungmath

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 7 weeks ago by git

  • Commit changed from d0f14dc7caeb6fb7a225d73939af6f5a95e06ca7 to 4b371bd075fc8c82328cffeff08c7cee91bfa9b5

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

4b371bdTrac #29581: add AHat class + minor fixes

comment:61 Changed 7 weeks ago by git

  • Commit changed from 4b371bd075fc8c82328cffeff08c7cee91bfa9b5 to e929409c7709091b1d3627614b7042f7b432274e

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

e929409Trac #29581: remove cached_function due to dynamic behavior w.r.t. has_orientation

comment:62 Changed 7 weeks ago by git

  • 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 7 weeks ago by git

  • Commit changed from 4b371bd075fc8c82328cffeff08c7cee91bfa9b5 to e82168f67b13aec1f32208eef2a2bf17b7e6fc04

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

e82168fTrac #29581: minor improvements

comment:64 Changed 7 weeks ago by git

  • Commit changed from e82168f67b13aec1f32208eef2a2bf17b7e6fc04 to 9f53f25b684540bf08e1d64d8a31b9420ebcce25

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

9f53f25Trac #29581: adapt names

comment:65 in reply to: ↑ 52 Changed 7 weeks ago by gh-mjungmath

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 7 weeks ago by git

  • Commit changed from 9f53f25b684540bf08e1d64d8a31b9420ebcce25 to 86b4d28a2aceb68e36b6fed8ca4ee1d8dcbc754c

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

86b4d28Trac #29581: add coercion and further improvements

comment:67 Changed 7 weeks ago by git

  • Commit changed from 86b4d28a2aceb68e36b6fed8ca4ee1d8dcbc754c to f81b25d80d4b0b9c692b25af037f1fe7623e96e8

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

f81b25dTrac #39581: add example for coercion

comment:68 Changed 7 weeks ago by git

  • Commit changed from f81b25d80d4b0b9c692b25af037f1fe7623e96e8 to 69dd5301343ea310bbdec34b65585a837c18be1b

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

69dd530Trac #39581: improve doc

comment:69 Changed 7 weeks ago by git

  • Commit changed from 69dd5301343ea310bbdec34b65585a837c18be1b to 00203e394d5afd7b61802a39dcf6cde13f73efb7

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

7328d50Trac #32272: remove unused imports
970d295Trac #30473: unicode -> latex for cup product
5021707Merge branch 't/32272/finitely_generated_graded_algebras_with_finite_degree' into t/29581/new_algorithm
00203e3Trac #29581: use cup-product for mul_symbol

comment:70 Changed 6 weeks ago by git

  • Commit changed from 00203e394d5afd7b61802a39dcf6cde13f73efb7 to 7555772e7b26039de4796997cd6f812a287ba49e

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

7555772Trac #29581: copy doc from old file + add some doc for CharacteristicCohomologyRing

comment:71 Changed 6 weeks ago by git

  • Commit changed from 7555772e7b26039de4796997cd6f812a287ba49e to 5f542e15b88799d5e37df351d32a6389e237e913

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

5f542e1Trac #29581: improve doc CharacteristicCohomologyClassRing

comment:72 Changed 6 weeks ago by git

  • Commit changed from 5f542e15b88799d5e37df351d32a6389e237e913 to 0ca6eb9fac729f9cfe68f61a5a41bf8e3d1f6504

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

0ca6eb9Trac #29581: add doc to sequences

comment:73 Changed 6 weeks ago by git

  • Commit changed from 0ca6eb9fac729f9cfe68f61a5a41bf8e3d1f6504 to e7169f81d27242663c168350b203c2d8d51861ef

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

e7169f8Trac #29581: doc of fast_wedge_power

comment:74 Changed 6 weeks ago by git

  • Commit changed from e7169f81d27242663c168350b203c2d8d51861ef to 114b483325526810c504935048e87b1e813985df

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

114b483Trac #29581: add more docstring

comment:75 Changed 6 weeks ago by git

  • Commit changed from 114b483325526810c504935048e87b1e813985df to bafd78dcbb8e5b385051d64cc0a68a2e14908ad6

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

bafd78dTrac #29581: remove old file + add more doc

comment:76 Changed 6 weeks ago by git

  • Commit changed from bafd78dcbb8e5b385051d64cc0a68a2e14908ad6 to 265f918b8045a6b93142130f9fa4147fc7bd1f4c

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

265f918Trac #29581: fix sign mistake

comment:77 Changed 6 weeks ago by git

  • Commit changed from 265f918b8045a6b93142130f9fa4147fc7bd1f4c to a3555e13332e0879adae188179120f4311462c6e

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

a3555e1Trac #29581: more docstring

comment:78 Changed 6 weeks ago by git

  • Commit changed from a3555e13332e0879adae188179120f4311462c6e to 939cfd9d21342527dc0442f653dc50791ce73ad8

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

939cfd9Trac #29581: more doc EulerAlgorithm

comment:79 Changed 6 weeks ago by git

  • Commit changed from 939cfd9d21342527dc0442f653dc50791ce73ad8 to e8d8393314a45980c25d110734a57ac4bda64191

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

e8d8393Trac #29581: correct doc for EulerAlgorithm

comment:80 Changed 6 weeks ago by git

  • Commit changed from e8d8393314a45980c25d110734a57ac4bda64191 to 974c3cc866d51d884de7b666335a8c5d36517666

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

974c3ccTrac #29851: finish doc so far

comment:81 Changed 6 weeks ago by gh-mjungmath

  • Cc gh-tobiasdiez added
  • Status changed from needs_info to needs_review

Ticket should be finally ready for review now. :)

comment:82 Changed 6 weeks ago by gh-mjungmath

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 6 weeks ago by git

  • Commit changed from 974c3cc866d51d884de7b666335a8c5d36517666 to 5830550c072e7ba16215dfe80e3985cee9e7d320

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

5830550Trac #29581: fix :: mistake and add Singleton example

comment:84 Changed 6 weeks ago by gh-mjungmath

Patchbot is morally green.

comment:85 Changed 6 weeks ago by git

  • Commit changed from 5830550c072e7ba16215dfe80e3985cee9e7d320 to 9a73cad1f5c1504b62c1890b28cc12c753cf9b23

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

9a73cadTrac #29581: correct ideal in doc + add long time tags for 30s or more

comment:86 Changed 6 weeks ago by git

  • Commit changed from 9a73cad1f5c1504b62c1890b28cc12c753cf9b23 to 4c1bc44977bbf506a415c9635bf884ae0519e229

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

4c1bc44Trac #29581: remove f-string without placeholder

comment:87 Changed 5 weeks ago by git

  • Commit changed from 4c1bc44977bbf506a415c9635bf884ae0519e229 to e8e0a0b3040b3a2e750eec48cf7ffbd4d3cfb65f

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

e8e0a0bMerge branch 'develop' into t/29581/new_algorithm

comment:88 follow-up: Changed 5 weeks ago by tscrim

  • Reviewers set to Travis Scrimshaw

Since CharacteristicCohomologyClass is not a function nor user-facing, 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 code-association 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 module-level doc as a starting point).

comment:89 Changed 5 weeks ago by git

  • Commit changed from e8e0a0b3040b3a2e750eec48cf7ffbd4d3cfb65f to ec282634db5fbb61c7731ba8998c356814887f70

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

ec28263Trac #29581: tie construuctor to ring

comment:90 in reply to: ↑ 88 ; follow-up: Changed 5 weeks ago by gh-mjungmath

Replying to tscrim:

Since CharacteristicCohomologyClass is not a function nor user-facing, 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 code-association 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 module-level doc as a starting point).

module-level doc?

comment:91 Changed 5 weeks ago by git

  • Commit changed from ec282634db5fbb61c7731ba8998c356814887f70 to c5370ec74eb85fd267c558ea8bad0e74512402b1

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

c5370ecTrac #29581: remove line breaks due to hard wrap in editor

comment:92 in reply to: ↑ 90 ; follow-up: Changed 4 weeks ago by tscrim

Replying to gh-mjungmath:

Replying to tscrim:

Since CharacteristicCohomologyClass is not a function nor user-facing, 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 code-association 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 module-level doc as a starting point).

module-level doc?

The documentation at the top of the characteristic_cohomology_class.py file (a Python module).

comment:93 in reply to: ↑ 92 ; follow-up: Changed 4 weeks ago by gh-mjungmath

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 ZZ-cohomology, 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 module-level doc as a starting point).

module-level doc?

The documentation at the top of the characteristic_cohomology_class.py file (a Python module).

Right. But this is indeed something for a follow-up. However, don't you think this fits better into a general tutorial?

Last edited 4 weeks ago by gh-mjungmath (previous) (diff)

comment:94 in reply to: ↑ 93 Changed 4 weeks ago by tscrim

  • Status changed from needs_review to positive_review

Replying to gh-mjungmath:

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 ZZ-cohomology, 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 module-level doc as a starting point).

Right. But this is indeed something for a follow-up. 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 4 weeks ago by gh-mjungmath

Thank you for the review Travis! :)

comment:96 Changed 4 weeks ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:97 Changed 4 weeks ago by git

  • Commit changed from c5370ec74eb85fd267c558ea8bad0e74512402b1 to 7777803bf6c305a8ed117972c8f08495aef90b7e

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

7777803Merge branch 'develop' into t/29581/new_algorithm

comment:98 Changed 4 weeks ago by gh-mjungmath

I have no idea what caused the merge conflict. I hope it works now...

comment:99 Changed 4 weeks ago by gh-mjungmath

  • Status changed from needs_work to needs_review

comment:100 Changed 3 weeks ago by tscrim

  • Status changed from needs_review to positive_review

comment:101 Changed 3 weeks ago by egourgoulhon

  • Status changed from positive_review to needs_work

Sorry to interfere here, but the patchbot says

sage -t --long --random-seed=0 src/sage/manifolds/differentiable/characteristic_cohomology_class.py  # 9 doctests failed

comment:102 Changed 3 weeks ago by tscrim

Looks like an import location changed:

ImportError: cannot import name 'I' from 'sage.libs.pynac.pynac'

comment:103 follow-up: Changed 3 weeks ago by gh-mjungmath

This is probably due to #32386. How do I import the imaginary unit now?

comment:104 in reply to: ↑ 103 ; follow-up: Changed 3 weeks ago by egourgoulhon

Replying to gh-mjungmath:

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 3 weeks ago by git

  • Commit changed from 7777803bf6c305a8ed117972c8f08495aef90b7e to 4e2e8b85ee02476a51574d1e04e87b0e6c0eaa00

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

4e2e8b8Trac #29581: new location of imaginary unit

comment:106 Changed 3 weeks ago by gh-mjungmath

  • Status changed from needs_work to needs_review

Here we go. Let's hope this works now.


New commits:

4e2e8b8Trac #29581: new location of imaginary unit

comment:107 follow-up: Changed 3 weeks ago by tscrim

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 3 weeks ago by egourgoulhon

Replying to egourgoulhon:

Replying to gh-mjungmath:

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

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 3 weeks ago by egourgoulhon

Replying to tscrim:

From #32386, they made this change:

-from sage.libs.pynac.pynac import I
+from sage.symbolic.expression import I

Here we have:

sage: from sage.symbolic.expression import I as I4
sage: I4 is I
False

comment:110 Changed 3 weeks ago by egourgoulhon

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
Last edited 3 weeks ago by egourgoulhon (previous) (diff)

comment:111 Changed 3 weeks ago by gh-mjungmath

But sage.symbolic.constants and sage.symbolic.expression give the same I, don't they?

comment:112 Changed 3 weeks ago by egourgoulhon

#18036 is probably relevant here.

comment:113 follow-up: Changed 3 weeks ago by 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

comment:114 in reply to: ↑ 113 Changed 3 weeks ago by egourgoulhon

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 3 weeks ago by egourgoulhon

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).

Last edited 3 weeks ago by egourgoulhon (previous) (diff)

comment:116 Changed 3 weeks ago by tscrim

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 3 weeks ago by git

  • Commit changed from 4e2e8b85ee02476a51574d1e04e87b0e6c0eaa00 to 44f8cfa8d960c1ec2db3a097f95728efcc86fcf4

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

44f8cfaTrac #29581: import I from symbolic.constants

comment:118 Changed 3 weeks ago by gh-mjungmath

Okay, there we go. Thank you for this feedback!

comment:119 Changed 3 weeks ago by gh-mjungmath

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Eric Gourgoulhon

comment:120 Changed 3 weeks ago by tscrim

  • Status changed from needs_review to positive_review

Green bot. LGTM.

comment:121 Changed 3 weeks ago by vbraun

  • 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 3 weeks ago by egourgoulhon

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 3 weeks ago by git

  • Commit changed from 44f8cfa8d960c1ec2db3a097f95728efcc86fcf4 to ac5cbbf0072380022c32466aab95fdfb31c4ecee

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

ac5cbbfTrac #29581: remove unicode character

comment:124 Changed 3 weeks ago by gh-mjungmath

  • Status changed from needs_work to needs_review

That should solve it. Hopefully.

comment:125 Changed 3 weeks ago by tscrim

  • Status changed from needs_review to positive_review

That is one way to solve it.

comment:126 Changed 12 days ago by vbraun

  • Branch changed from u/gh-mjungmath/new_algorithm to ac5cbbf0072380022c32466aab95fdfb31c4ecee
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.