Opened 8 months ago

Closed 7 months ago

#27584 closed enhancement (fixed)

Characteristic Classes - MixedAlgebra

Reported by: gh-DeRhamSource Owned by:
Priority: major Milestone: sage-8.8
Component: geometry Keywords: characteristic classes, manifolds
Cc: egourgoulhon, tscrim Merged in:
Authors: Michael Jung Reviewers: Eric Gourgoulhon, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: cdfaf6d (Commits) Commit: cdfaf6d16322ef4c7d014680ebf025f272f62eb6
Dependencies: Stopgaps:

Description (last modified by gh-DeRhamSource)

This project is my master thesis. I want to implement a computation of characteristic classes of vector bundles out of their curvature matrices. The algorithm is based on this short script.

In this first step, the graded algebra of "mixed" differential forms of a manifold will be introduced so one can use the matrix framework of sage in the following steps.

A demo notebook is provided at https://github.com/DeRhamSource/MixedFormNotebook

Change History (75)

comment:1 Changed 8 months ago by gh-DeRhamSource

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

comment:2 Changed 8 months ago by gh-DeRhamSource

  • Cc tscrim added
  • Description modified (diff)
  • Summary changed from Characteristic Classes to Characteristic Classes - MixedAlgebra

comment:3 Changed 8 months ago by gh-DeRhamSource

  • Branch set to u/DeRhamSource/char_class_algebra

comment:4 Changed 8 months ago by gh-DeRhamSource

  • Branch changed from u/DeRhamSource/char_class_algebra to u/gh-DeRhamSource/char_class_algebra

comment:5 Changed 8 months ago by gh-DeRhamSource

  • Branch u/gh-DeRhamSource/char_class_algebra deleted

comment:6 Changed 8 months ago by gh-DeRhamSource

  • Branch set to u/gh-DeRhamSource/characteristic_classes___mixedalgebra

comment:7 Changed 8 months ago by git

  • Commit set to 49f21e50d792f4000c0b0b6cf64c93fd5eaf2da2

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

49f21e5Initial version (without doctests)

comment:8 Changed 8 months ago by egourgoulhon

Here is the answer to the question raised in https://groups.google.com/d/msg/sage-devel/hwsQWuLN0nc/1_8LjLL4AgAJ, namely when a is a differential form (from previously existing DiffForm class) and A is a mixed form (as introduced in this ticket), why do we get

sage: a.__mul__(A)
Mixed differential form A/\a on the 2-dimensional differentiable manifold M

I think this results from a clash between (i) the operator __mul__ being used to denote the wedge product for mixed differential forms and (ii) the operator __mul__ being used to denote tensor product for differential forms. Indeed a.__mul__(A) calls the method __mul__ implemented in line 2169 of src/sage/manifolds/differentiable/tensorfield.py, which is devoted to the tensor product (the differential form a being considered as a tensor field of type (0,p), where p=degree(a)). As you can see in line 2269, if A does not belong to the class TensorField, one assumes that A is a scalar field (the only possible case until this ticket) and the code return A*a, i.e. A.wedge(a).

A possible solution could be to change the lines 2269-2271 of tensorfield.py to

if isinstance(other, MixedForm):
    return other.parent()(self)._mul_(other)
elif not isinstance(other, TensorField):
    # Multiplication by a scalar field or a number
    return other * self

A better way would be to redefine __mul__ in DiffForm, falling back to the tensor field version if other is not a mixed form.

comment:9 Changed 8 months ago by git

  • Commit changed from 49f21e50d792f4000c0b0b6cf64c93fd5eaf2da2 to 4ba165d85dff93b5cbad4fb7af3b403e2e968077

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

4ba165dSage Compatibility

comment:10 Changed 8 months ago by egourgoulhon

Your code looks nice, as well as the demo notebook!

Just a remark at this stage: you should not put any Jupyter notebook in the ticket's git branch. Indeed, when the ticket gets a positive review, the branch is merged into Sage's main branch, which must contain only source code (it is already very large just with sources). It is of course a good idea to share notebooks during the development process, but you should put them to a public repository (e.g. GitHub) and simply add a link to them in some comment on the ticket.

comment:11 Changed 8 months ago by tscrim

The other thing you could do is add the notebook file as an attachment (but note that you cannot delete it afterwards).

comment:12 follow-up: Changed 8 months ago by gh-DeRhamSource

Oh I'm sorry, I didn't realize that. How can I delete this file from the ticket?

In the next step, I will translate the notebook to english and add Doctests to the main files. Then, I guess, it's ready for review.

Last edited 8 months ago by gh-DeRhamSource (previous) (diff)

comment:13 in reply to: ↑ 12 Changed 8 months ago by egourgoulhon

Replying to gh-DeRhamSource:

Oh I'm sorry, I didn't realize that. How can I delete this file from the ticket?

Since the file is not very large, I guess it suffices to run

git rm scripts/MixedAlgebra.ipynb

before your next commit. Otherwise (i.e. for a large file), I would have recommended to delete the branch and to start a brand new one.

In the next step, I will translate the notebook to english and add Doctests to the main files. Then, I guess, it's ready for review.

OK very good!

comment:14 Changed 8 months ago by git

  • Commit changed from 4ba165d85dff93b5cbad4fb7af3b403e2e968077 to 3ca14f2de40c60ec79036c03931dc5b30729cee6

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

3ca14f2Notebook removed + Minor Changes

comment:15 Changed 8 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:16 Changed 8 months ago by git

  • Commit changed from 3ca14f2de40c60ec79036c03931dc5b30729cee6 to c13bd1dde3a4c5c881c73d734a2b46285ed42074

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

c13bd1dSome doctest examples added

comment:17 Changed 8 months ago by gh-DeRhamSource

I added a link to my public github repo for the demo notebook in the description.

comment:18 Changed 8 months ago by git

  • Commit changed from c13bd1dde3a4c5c881c73d734a2b46285ed42074 to eea8b18ed4e00f01a1f67bd2694200c9a2919006

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

eea8b18Added some doctests + minor changes

comment:19 Changed 8 months ago by gh-DeRhamSource

There is now a full doctest implemented in mixed_form_algebra.py. However, I've encountered another problem while coding the doctest of mixed_form.py. Namely an issue with coercions from the symbolic ring:

sage: M = Manifold(2, 'M')
sage: X.<x,y> = M.chart()
sage: A = M.mixed_form_algebra()
sage: A._element_constructor_(x).disp(X.frame())
[x] + [0] + [0]
sage: A(x).disp(X.frame())
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-84-7bd889258762> in <module>()
----> 1 A(x).disp(X.frame())

AttributeError: 'NoneType' object has no attribute 'disp'

How is that possible? Shouldn't A(.) invoke the method A._element_constructor_?

comment:20 Changed 8 months ago by tscrim

It does, but only after checking for a coercion. So my guess is something is going strange with the coercion. Posting the full traceback would be useful.

A few unrelated comments:

isinstance(comp, (int, Integer)) -> comp in ZZ because it also catches cases like 2/1 (which is in QQ).

A few of your example blocks should be TESTS:: and EXAMPLES:: when followed immediately by doctests, which should also be indentent 1 level.

comment:21 Changed 8 months ago by gh-DeRhamSource

Thanks! :)

The snippet isinstance(comp, (int, Integer)) is adapted from diff_form_module.py. Should I apply your suggestion in that file as well for this branch, then?

More feedback and suggestions are very welcome! :)

Unfortunately, there is no error message or traceback:

sage: M = Manifold(2, 'M')
sage: X.<x,y> = M.chart()
sage: A = M.mixed_form_algebra()
sage: A(x)

The coercion is already known:

sage: M.mixed_form_algebra().has_coerce_map_from(SR)
True

So, I have really no idea why it won't work. But for scalar fields, it does:

sage: f = M.scalar_field_algebra()(x); f
Scalar field on the 2-dimensional differentiable manifold M
Last edited 8 months ago by gh-DeRhamSource (previous) (diff)

comment:22 Changed 8 months ago by tscrim

Hmm...so the output of A(x) really is None...that is very strange. What does coercion_model.explain(x, A) result in?

comment:23 Changed 8 months ago by gh-DeRhamSource

sage: M = Manifold(2, 'M')
sage: X.<x,y> = M.chart()
sage: A = M.mixed_form_algebra()
sage: coercion_model.explain(x, A)
Coercion on left operand via
    Generic morphism:
      From: Symbolic Ring
      To:   Graded algebra Omega^*(M) of mixed differential forms on the 2-dimensional differentiable manifold M
Arithmetic performed after coercions.
Result lives in Graded algebra Omega^*(M) of mixed differential forms on the 2-dimensional differentiable manifold M

comment:24 Changed 8 months ago by tscrim

What is the result of

sage: phi = A.coerce_map_from_(SR)
sage: phi
sage: type(phi)
sage: phi(x)

comment:25 Changed 8 months ago by gh-DeRhamSource

sage: M = Manifold(2, 'M')
sage: X.<x,y> = M.chart()
sage: A = M.mixed_form_algebra()
sage: phi = A.coerce_map_from(SR)
sage: phi
Generic morphism:
  From: Symbolic Ring
  To:   Graded algebra Omega^*(M) of mixed differential forms on the 2-dimensional differentiable manifold M
sage: type(phi)
<type 'sage.categories.morphism.SetMorphism'>
sage: phi(x)
sage: type(phi(x))
<type 'NoneType'>

comment:26 Changed 8 months ago by tscrim

That was what I was expecting. Basically what is happening is when it goes through the coercion framework, the result is None. So you need to look into that coercion morphism and what function it is calling and why that is not returning the proper object.

comment:27 Changed 8 months ago by gh-DeRhamSource

I'm sorry: How do I manage that?

comment:28 Changed 8 months ago by tscrim

You will have to explore the properties of the morphism, such as phi._function, and maybe also add a print() statement or two for debugging in the _element_constructor_ to see what is happening.

comment:29 follow-ups: Changed 7 months ago by egourgoulhon

I think the issue arises because SR is the base ring of the algebra A:

sage: A.base_ring()
Symbolic Ring

If I am correct, in such a case, the coercion SR --> A is implemented via the algebra operation x * A.one(), not via _element_constructor_. In the present case, x * A.one() fails:

sage: x * A.one()
...
RuntimeError: BUG in map, returned None x <type 'sage.categories.morphism.SetMorphism'> Generic morphism:
  From: Symbolic Ring
  To:   Graded algebra Omega^*(M) of mixed differential forms on the 2-dimensional differentiable manifold M

To make it work, you should implement the method _lmul_ in class MixedForm and add the line

    if other is SR:
        return True

in MixedFormAlgebra._coerce_map_from_. At least, this is what is done for the algebras of scalar fields: see line 512 of src/sage/manifolds/scalarfield_algebra.py and the definition of _lmul_ in line 2449 of src/sage/manifolds/scalarfield.py.

comment:30 Changed 7 months ago by gh-DeRhamSource

Yes, perfect. That solved the problem. Thanks! :)

comment:31 in reply to: ↑ 29 ; follow-up: Changed 7 months ago by tscrim

Replying to egourgoulhon:

To make it work, you should implement the method _lmul_ in class MixedForm and add the line

    if other is SR:
        return True

in MixedFormAlgebra._coerce_map_from_. At least, this is what is done for the algebras of scalar fields: see line 512 of src/sage/manifolds/scalarfield_algebra.py and the definition of _lmul_ in line 2449 of src/sage/manifolds/scalarfield.py.

I am a little worried about this. What about _rmul_? This also makes the dependence too strong on the base ring being SR. Can we use if other is self.base_ring() or something like that?

comment:32 in reply to: ↑ 31 ; follow-up: Changed 7 months ago by egourgoulhon

Replying to tscrim:

I am a little worried about this. What about _rmul_?

I am puzzled now about the scalar field algebra implementation. Why do we have _lmul_ and not _rmul_? Indeed, __rmul__ is the standard Python operator for reflected operands and __lmul__ does not exist in Python. What's exactly the role of _lmul_ in Sage? I guess it is there for noncommutative base rings but I could not find a good place in the documentation explaining this... In the present case, both for scalar field and mixed form algebras, we are dealing with a base ring that is commutative (since it is a field), so shouldn't we have only _rmul_?

comment:33 Changed 7 months ago by git

  • Commit changed from eea8b18ed4e00f01a1f67bd2694200c9a2919006 to 0d772ae29e7971ab8f5549eb5e647dde66b2ba9d

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

0d772aeFull Doctest added

comment:34 Changed 7 months ago by gh-DeRhamSource

  • Status changed from new to needs_review

comment:35 in reply to: ↑ 29 ; follow-up: Changed 7 months ago by gh-DeRhamSource

Replying to egourgoulhon:

To make it work, you should implement the method _lmul_ in class MixedForm and add the line

    if other is SR:
        return True

in MixedFormAlgebra._coerce_map_from_. At least, this is what is done for the algebras of scalar fields: see line 512 of src/sage/manifolds/scalarfield_algebra.py and the definition of _lmul_ in line 2449 of src/sage/manifolds/scalarfield.py.

I managed the coercion detection via

        if self._domain.scalar_field_algebra().has_coerce_map_from(S):
            return True

Does that also eliminate your doubts concerning the strong dependencies on SR, Travis?

comment:36 in reply to: ↑ 32 ; follow-up: Changed 7 months ago by egourgoulhon

Replying to egourgoulhon:

Replying to tscrim:

I am a little worried about this. What about _rmul_?

I am puzzled now about the scalar field algebra implementation. Why do we have _lmul_ and not _rmul_? Indeed, __rmul__ is the standard Python operator for reflected operands and __lmul__ does not exist in Python. What's exactly the role of _lmul_ in Sage? I guess it is there for noncommutative base rings but I could not find a good place in the documentation explaining this... In the present case, both for scalar field and mixed form algebras, we are dealing with a base ring that is commutative (since it is a field), so shouldn't we have only _rmul_?

I see in line 2394 of src/sage/structure/element.pyx that _rmul_ falls back to _lmul_, which is not implemented at the ModuleElement level (actually returns None). This leaves the impression that when implementing a new module element class over a commutative ring, one shall actually implement _lmul_ and not _rmul_... Accordingly, my understanding at the moment is that in Sage, we have

  • _mul_(self, other) when self and other have the same parent
  • _lmul_(self, other) when self and other have distinct parents
  • _rmul_(self, other) is inherited from ModuleElement (if not redefined) and falls back to _lmul_

Travis, do you confirm?

Last edited 7 months ago by egourgoulhon (previous) (diff)

comment:37 in reply to: ↑ 36 Changed 7 months ago by tscrim

Replying to egourgoulhon:

Replying to egourgoulhon:

Replying to tscrim:

I am a little worried about this. What about _rmul_?

I am puzzled now about the scalar field algebra implementation. Why do we have _lmul_ and not _rmul_? Indeed, __rmul__ is the standard Python operator for reflected operands and __lmul__ does not exist in Python. What's exactly the role of _lmul_ in Sage? I guess it is there for noncommutative base rings but I could not find a good place in the documentation explaining this... In the present case, both for scalar field and mixed form algebras, we are dealing with a base ring that is commutative (since it is a field), so shouldn't we have only _rmul_?

I see in line 2394 of src/sage/structure/element.pyx that _rmul_ falls back to _lmul_, which is not implemented at the ModuleElement level (actually returns None). This leaves the impression that when implementing a new module element class over a commutative ring, one shall actually implement _lmul_ and not _rmul_... Accordingly, my understanding at the moment is that in Sage, we have

  • _mul_(self, other) when self and other have the same parent
  • _lmul_(self, other) when self and other have distinct parents
  • _rmul_(self, other) is inherited from ModuleElement (if not redefined) and falls back to _lmul_

Travis, do you confirm?

Ah, right, I forgot that generic _rmul_ was implemented in ModuleElement.

So Sage's _rmul_ should not be confused with Python's __rmul__.

Python's __rmul__ is for when self knows how to multiply with other when self is on the right, but other does not. For instance, it handles 5 * self because we cannot override the int type (which means the "standard int.__mul__" fails), but self knows what to do.

Sage's _rmul_ and _lmul_ play the same role, just on different sides. They are there to implement actions of other on self on the right or left, and thus are used by the coercion framework (in particular, they check if coercions can be applied).

comment:38 in reply to: ↑ 35 Changed 7 months ago by tscrim

Replying to gh-DeRhamSource:

Replying to egourgoulhon:

To make it work, you should implement the method _lmul_ in class MixedForm and add the line

    if other is SR:
        return True

in MixedFormAlgebra._coerce_map_from_. At least, this is what is done for the algebras of scalar fields: see line 512 of src/sage/manifolds/scalarfield_algebra.py and the definition of _lmul_ in line 2449 of src/sage/manifolds/scalarfield.py.

I managed the coercion detection via

        if self._domain.scalar_field_algebra().has_coerce_map_from(S):
            return True

Does that also eliminate your doubts concerning the strong dependencies on SR, Travis?

Yes, I think that is better. Generally, I do not think it is a good idea to hardcode a ring unless you really only want to work with that ring (and my understanding is that this could be made more flexible in the future).

comment:39 Changed 7 months ago by egourgoulhon

  • Branch changed from u/gh-DeRhamSource/characteristic_classes___mixedalgebra to public/manifolds/mixed_differential_forms
  • Commit changed from 0d772ae29e7971ab8f5549eb5e647dde66b2ba9d to 506ef37791cdd89b1b2fa9b8009827197205c17e

There was a merge conflict with Sage 8.8.beta1 in the file src/sage/manifolds/differentiable/manifold.py. This was due to #27581 (initialization of the components of a tensor field while declaring it), which changed that file. The conflict is solved in the attached branch (since I am the author of #27581, it was probably easier that I solved this conflict, hence the new branch).


New commits:

506ef37Merge branch 'u/gh-DeRhamSource/characteristic_classes___mixedalgebra' of git://trac.sagemath.org/sage into Sage 8.8.beta1
Last edited 7 months ago by egourgoulhon (previous) (diff)

comment:40 Changed 7 months ago by egourgoulhon

  • Reviewers set to Eric Gourgoulhon, Travis Scrimshaw

If you click on the patchbot button (marked "8.8.beta1") located at the right of the ticket title, you will access to the patchbot reports. One of them is useless, due to some patchbot issue (the one marked "BuildFailed"). The most relevant one is from the patchbot "zancara". It indicates "TestFailed", but if you click either on "log" or "shortlog", you'll see that the failure occurs in a source file that has not been touched by this ticket: src/sage/tests/books/computational-mathematics-with-sagemath/graphique_doctest.py. So this is more an issue on the patchbot size. The only relevant issue is the pyflakes error marked by a red cross in the plugin list at the right of the summary of zancara's report: if you click on "diff", you get

src/sage/manifolds/differentiable/mixed_form_algebra.py:31: 
'sage.rings.integer.Integer' imported but unused

Can you please correct this? Note that you shall update your Sage version to 8.8.beta1. If this is not done already, it suffices to pull the new ticket branch and run MAKE="make -j8" make.

comment:41 follow-up: Changed 7 months ago by egourgoulhon

Regarding the documentation: you have added doctests in the Python sources but the documentation is not generated since there is no entry to it in the sources of the reference manual. You shall modify/add files in src/doc/en/reference/manifolds, for instance in a way similar to what is done for differential forms, i.e. create a file mixed_diff_form.rst and add a pointer to it in diff_manifold.rst. To generate the documentation in a fast way, i.e. without having Sage process the whole reference manual, run

sage -docbuild reference/manifolds html

comment:42 Changed 7 months ago by egourgoulhon

Side remark: the bug that you've reported in https://groups.google.com/d/msg/sage-devel/EfLYpAxl_jU/ShlCS9L4BgAJ is fixed in Sage 8.8.beta1 (since #25576 has been merged in it).

comment:43 in reply to: ↑ 41 Changed 7 months ago by gh-DeRhamSource

Replying to egourgoulhon:

Regarding the documentation: you have added doctests in the Python sources but the documentation is not generated since there is no entry to it in the sources of the reference manual. You shall modify/add files in src/doc/en/reference/manifolds, for instance in a way similar to what is done for differential forms, i.e. create a file mixed_diff_form.rst and add a pointer to it in diff_manifold.rst. To generate the documentation in a fast way, i.e. without having Sage process the whole reference manual, run

sage -docbuild reference/manifolds html

Unfortunately, I get some errors:

[manifolds] /home/michi/GitProjects/sage/src/sage_setup/docbuild/ext/sage_autodoc.py:1170: RemovedInSphinx20Warning: formatargspec() is now deprecated.  Please use sphinx.util.inspect.Signature instead.
[manifolds]   return formatargspec(initmeth, *argspec)
[manifolds] /home/michi/GitProjects/sage/src/sage_setup/docbuild/ext/sage_autodoc.py:1406: RemovedInSphinx20Warning: formatargspec() is now deprecated.  Please use sphinx.util.inspect.Signature instead.
[manifolds]   args = formatargspec(self.object, *argspec)
[manifolds] /home/michi/GitProjects/sage/src/sage_setup/docbuild/ext/sage_autodoc.py:1072: RemovedInSphinx20Warning: formatargspec() is now deprecated.  Please use sphinx.util.inspect.Signature instead.
[manifolds]   args = formatargspec(self.object, *argspec)
...
OSError: /home/michi/GitProjects/sage/src/doc/en/reference/manifolds/index.rst:4: WARNING: undefined label: tensors-on-free-modules (if the link has no caption the label must precede a section header)

However, what comes next?

Last edited 7 months ago by gh-DeRhamSource (previous) (diff)

comment:44 Changed 7 months ago by git

  • Commit changed from 506ef37791cdd89b1b2fa9b8009827197205c17e to 970ccce6b788861b77e8cb9b6502f74bd369ab1d

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

970ccceMinor fixes

comment:45 Changed 7 months ago by git

  • Commit changed from 970ccce6b788861b77e8cb9b6502f74bd369ab1d to 9516d69ee612b353ff4e4a60f5b02c14601ff61b

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

9516d69Sorry!

comment:46 Changed 7 months ago by git

  • Commit changed from 9516d69ee612b353ff4e4a60f5b02c14601ff61b to 3a731b678258d93c13d1ba3190afcb8def3a69a6

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

3a731b6Damn! :D

comment:47 Changed 7 months ago by tscrim

Some debugging will be needed (if you haven't fixed them already). Also you should rebase this (and force push) to have more professional commit messages. ;)

comment:48 follow-up: Changed 7 months ago by gh-DeRhamSource

What should be debugged? I mean, the error above does not seem to have to do something with my implementation. Or does it?

Long story short: There is no error occuring anymore regarding the mixed algebra while building the doc.

Last edited 7 months ago by gh-DeRhamSource (previous) (diff)

comment:49 in reply to: ↑ 48 Changed 7 months ago by egourgoulhon

Replying to gh-DeRhamSource:

What should be debugged? I mean, the error above does not seem to have to do something with my implementation. Or does it?

Long story short: There is no error occuring anymore regarding the mixed algebra while building the doc.

Yes but some parts of the documentation require some debugging:

  • a formula is not displayed because .. MATH: should be .. MATH:: in line 45 of mixed_form.py
  • in line 60 of the same file, there is an extra backquote around comp
  • at the top of mixed_form.py you may want to replace \Phi by \varphi to be consistent with the rest of the text, as well as with mixed_form_algebra.py

comment:50 Changed 7 months ago by git

  • Commit changed from 3a731b678258d93c13d1ba3190afcb8def3a69a6 to 177cfff96044aa16d00a95ffcc7798f63cf29ec3

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

177cfffMinor fixes

comment:51 Changed 7 months ago by git

  • Commit changed from 177cfff96044aa16d00a95ffcc7798f63cf29ec3 to 4d9967aa4640d600131d17c57d4683c8568bbcc5

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

38b36edMinor fixes
4d9967aMinor doc fixes

comment:52 Changed 7 months ago by gh-DeRhamSource

My apologies for these confusing commits. I'm quite new to it.

However, I fixed the aforementioned issues.

Last edited 7 months ago by gh-DeRhamSource (previous) (diff)

comment:53 Changed 7 months ago by tscrim

I have a couple more comments, most of them are fairly minor:

In _richcmp_:

        from sage.structure.richcmp import richcmp

        # Compare all elements separately:
        for j in range(0, self._max_deg + 1):

move the import statement to the top-level (this is for speed reasons) and make range(0, foo) -> range(foo).

Make _mul_ = wedge and maybe combine or reduce the doctests.

degree $k$ along `\varphi` replace & with `.

Instead of using self.__class__, I have been told it is better to use type(self).

Instead of \, my understanding is it is better to use () as that is guaranteed to generate an error if misused (as opposed to possibly a strange bug). For example

-                error_msg = "input must be a " \
-                            "differential form of degree {}".format(deg)
+                error_msg = ("input must be a "
+                             "differential form of degree {}".format(deg))

Typo: datailed -> detailed

I don't think you need the self._populate_coercion_lists_().

comment:54 Changed 7 months ago by git

  • Commit changed from 4d9967aa4640d600131d17c57d4683c8568bbcc5 to cebde90d71134ea8c83ab3e96028cf123a4b7ad3

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

cebde90Typos and Minor Issues Fixed

comment:55 Changed 7 months ago by git

  • Commit changed from cebde90d71134ea8c83ab3e96028cf123a4b7ad3 to b78c7a9f8a928250b60278eb510d4f041d540849

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

b78c7a9Whitespace Correction

comment:56 Changed 7 months ago by git

  • Commit changed from b78c7a9f8a928250b60278eb510d4f041d540849 to 407505d3827641a010caf3fcd11bed4df88b78b3

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

407505dComponents from scratch

comment:57 Changed 7 months ago by git

  • Commit changed from 407505d3827641a010caf3fcd11bed4df88b78b3 to f03d4a0f22df2c6abcb41f3afa5b74bd392f3cf1

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

f03d4a0Minor: Whitespace + Comment

comment:58 Changed 7 months ago by git

  • Commit changed from f03d4a0f22df2c6abcb41f3afa5b74bd392f3cf1 to b4a92196526d32e171c896e59e90a90d46865d6e

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

b4a9219Error Message obsolete

comment:59 Changed 7 months ago by gh-DeRhamSource

Thanks for your patience so far. Again, I changed a lot. Please check.

However, I noticed another error message that should not occur:

sage: M = Manifold(2, 'M')
sage: X.<x,y> = M.chart()
sage: f = M.scalar_field(x, name='f')
sage: f in M.diff_form_module(1)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-38-9654b754d2bc> in <module>()
      2 X = M.chart(names=('x', 'y',)); (x, y,) = X._first_ngens(2)
      3 f = M.scalar_field(x, name='f')
----> 4 f in M.diff_form_module(Integer(1))

/home/michi/GitProjects/sage/local/lib/python2.7/site-packages/sage/structure/parent.pyx in sage.structure.parent.Parent.__contains__ (build/cythonized/sage/structure/parent.c:9885)()
   1090             return True
   1091         try:
-> 1092             x2 = self(x)
   1093             EQ = (x2 == x)
   1094             if EQ is True:

/home/michi/GitProjects/sage/local/lib/python2.7/site-packages/sage/structure/parent.pyx in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9219)()
    898         if mor is not None:
    899             if no_extra_args:
--> 900                 return mor._call_(x)
    901             else:
    902                 return mor._call_with_args(x, args, kwds)

/home/michi/GitProjects/sage/local/lib/python2.7/site-packages/sage/structure/coerce_maps.pyx in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4551)()
    160                 print(type(C), C)
    161                 print(type(C._element_constructor), C._element_constructor)
--> 162             raise
    163 
    164     cpdef Element _call_with_args(self, x, args=(), kwds={}):

/home/michi/GitProjects/sage/local/lib/python2.7/site-packages/sage/structure/coerce_maps.pyx in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4443)()
    155         cdef Parent C = self._codomain
    156         try:
--> 157             return C._element_constructor(x)
    158         except Exception:
    159             if print_warnings:

/home/michi/GitProjects/sage/local/lib/python2.7/site-packages/sage/manifolds/differentiable/diff_form_module.pyc in _element_constructor_(self, comp, frame, name, latex_name)
    818         resu = self.element_class(self._fmodule, self._degree, name=name,
    819                                   latex_name=latex_name)
--> 820         if comp != []:
    821             resu.set_comp(frame)[:] = comp
    822         return resu

/home/michi/GitProjects/sage/local/lib/python2.7/site-packages/sage/manifolds/scalarfield.pyc in __ne__(self, other)
   1285 
   1286         """
-> 1287         return not (self == other)
   1288 
   1289     ####### End of required methods for an algebra element (beside arithmetic) #######

/home/michi/GitProjects/sage/local/lib/python2.7/site-packages/sage/manifolds/scalarfield.pyc in __eq__(self, other)
   1244                 return False
   1245             try:
-> 1246                 other = self.parent()(other)  # conversion to a scalar field
   1247             except TypeError:
   1248                 return False

/home/michi/GitProjects/sage/local/lib/python2.7/site-packages/sage/structure/parent.pyx in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9219)()
    898         if mor is not None:
    899             if no_extra_args:
--> 900                 return mor._call_(x)
    901             else:
    902                 return mor._call_with_args(x, args, kwds)

/home/michi/GitProjects/sage/local/lib/python2.7/site-packages/sage/structure/coerce_maps.pyx in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4551)()
    160                 print(type(C), C)
    161                 print(type(C._element_constructor), C._element_constructor)
--> 162             raise
    163 
    164     cpdef Element _call_with_args(self, x, args=(), kwds={}):

/home/michi/GitProjects/sage/local/lib/python2.7/site-packages/sage/structure/coerce_maps.pyx in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4443)()
    155         cdef Parent C = self._codomain
    156         try:
--> 157             return C._element_constructor(x)
    158         except Exception:
    159             if print_warnings:

/home/michi/GitProjects/sage/local/lib/python2.7/site-packages/sage/manifolds/scalarfield_algebra.pyc in _element_constructor_(self, coord_expression, chart, name, latex_name)
    470                                       coord_expression=coord_expression,
    471                                       name=name, latex_name=latex_name,
--> 472                                       chart=chart)
    473         return resu
    474 

/home/michi/GitProjects/sage/local/lib/python2.7/site-packages/sage/manifolds/differentiable/scalarfield.pyc in __init__(self, parent, coord_expression, chart, name, latex_name)
    630         """
    631         ScalarField.__init__(self, parent, coord_expression=coord_expression,
--> 632                              chart=chart, name=name, latex_name=latex_name)
    633         self._tensor_type = (0,0)
    634 

/home/michi/GitProjects/sage/local/lib/python2.7/site-packages/sage/manifolds/scalarfield.pyc in __init__(self, parent, coord_expression, chart, name, latex_name)
   1101                         self._express[ch] = ch.function(coord_expression)
   1102                 else:
-> 1103                     self._express[chart] = chart.function(coord_expression)
   1104         self._init_derived()   # initialization of derived quantities
   1105 

/home/michi/GitProjects/sage/local/lib/python2.7/site-packages/sage/manifolds/chart.pyc in function(self, expression, calc_method, expansion_symbol, order)
   1087         return parent.element_class(parent, expression, calc_method=calc_method,
   1088                                     expansion_symbol=expansion_symbol,
-> 1089                                     order=order)
   1090 
   1091     def zero_function(self):

/home/michi/GitProjects/sage/local/lib/python2.7/site-packages/sage/manifolds/chart_func.pyc in __init__(self, parent, expression, calc_method, expansion_symbol, order)
    360                 calc_method = self._calc_method._current
    361             self._express[calc_method] = self._calc_method._tranf[calc_method](
--> 362                                                                     expression)
    363         # Derived quantities:
    364         self._der = None  # list of partial derivatives (to be set by diff()

/home/michi/GitProjects/sage/local/lib/python2.7/site-packages/sage/manifolds/calculus_method.pyc in _Sympy_to_SR(expression)
    106         # If SR cannot transform a sympy expression this is because it is a
    107         # sympy abstract function
--> 108         a = expression._sage_()
    109         # As all sage objects have a ._sage_ operator, they have to be
    110         # catched

AttributeError: 'list' object has no attribute '_sage_'

comment:60 Changed 7 months ago by git

  • Commit changed from b4a92196526d32e171c896e59e90a90d46865d6e to 8036fb0b38fb4add88e5c54b1c1d071074fbc9b8

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

8036fb0Components from scratch - Fixes

comment:61 Changed 7 months ago by git

  • Commit changed from 8036fb0b38fb4add88e5c54b1c1d071074fbc9b8 to 6d1c88cb09a31613dd921d2bef7c71cfbfd27ed6

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

6d1c88cTiny doc typos fixed

comment:62 follow-up: Changed 7 months ago by tscrim

Does that second to last commit fix the issue in comment:59?

comment:63 in reply to: ↑ 62 ; follow-up: Changed 7 months ago by gh-DeRhamSource

Replying to tscrim:

Does that second to last commit fix the issue in comment:59?

No. But now, I guess this has nothing to do with the mixed algebra. I already reported it in sage-devel.

Last edited 7 months ago by gh-DeRhamSource (previous) (diff)

comment:64 in reply to: ↑ 63 ; follow-up: Changed 7 months ago by egourgoulhon

Replying to gh-DeRhamSource:

Replying to tscrim:

Does that second to last commit fix the issue in comment:59?

No. But now, I guess this has nothing to do with the mixed algebra. I already reported it in sage-devel.

I confirm the error. I've opened #27658 for it.

comment:65 in reply to: ↑ 64 Changed 7 months ago by egourgoulhon

Replying to egourgoulhon:

Replying to gh-DeRhamSource:

Replying to tscrim:

Does that second to last commit fix the issue in comment:59?

No. But now, I guess this has nothing to do with the mixed algebra. I already reported it in sage-devel.

I confirm the error. I've opened #27658 for it.

While this is definitely a bug, you can get rid of it by replacing

f in M.diff_form_module(n)

by

f.domain() is M and f.degree() == n

for any integer n. Moreover the second form is must faster.

Last edited 7 months ago by egourgoulhon (previous) (diff)

comment:66 follow-up: Changed 7 months ago by gh-DeRhamSource

Fortunately, this issue is not directly relevant for my code anymore (since the error message was deleted). I've encountered the problem while debugging. :)

Last edited 7 months ago by gh-DeRhamSource (previous) (diff)

comment:67 in reply to: ↑ 66 Changed 7 months ago by egourgoulhon

Replying to gh-DeRhamSource:

Fortunately, this issue is not directly relevant for my code anymore (since the error message was deleted). I've encountered the problem while debugging. :)

Good!

comment:68 Changed 7 months ago by gh-DeRhamSource

Patchbot says no. :(

Though, there is no obvious error. So, what's the matter?

Any further suggestions?

comment:69 follow-up: Changed 7 months ago by tscrim

That error from the patchbot looks unrelated, so I would treat the patchbot as coming back green.

Sorry, one more nitpick for readability:

-            return self._domain.is_subset(
-                S._domain) and self._ambient_domain.is_subset(S._ambient_domain)
+            return (self._domain.is_subset(S._domain)
+                    and self._ambient_domain.is_subset(S._ambient_domain))

Eric, any other comments?

comment:70 in reply to: ↑ 69 Changed 7 months ago by egourgoulhon

Replying to tscrim:

Eric, any other comments?

Yes, there is a doctest error with Python3-built Sage:

sage -t --long --warn-long 48.2 src/sage/manifolds/differentiable/mixed_form.py
**********************************************************************
File "src/sage/manifolds/differentiable/mixed_form.py", line 479, in sage.manifolds.differentiable.mixed_form.MixedForm.set_name
Failed example:
    F.set_name(name='fancy', latex_name='\eta'); F
Expected:
    Mixed differential form fancy on the 4-dimensional differentiable
     manifold M
Got:
    doctest:warning
    ...
    DeprecationWarning: invalid escape sequence \e
    Mixed differential form fancy on the 4-dimensional differentiable manifold M
**********************************************************************

You should replace '\eta' by r'\eta'.

Apart from this, everything is OK for me.

Last edited 7 months ago by egourgoulhon (previous) (diff)

comment:71 Changed 7 months ago by git

  • Commit changed from 6d1c88cb09a31613dd921d2bef7c71cfbfd27ed6 to cdfaf6d16322ef4c7d014680ebf025f272f62eb6

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

cdfaf6dDoctest fix + Readability

comment:72 follow-up: Changed 7 months ago by gh-DeRhamSource

Anything else? :)

Otherwise I'd start programming the next part - namely characteristic classes.

comment:73 in reply to: ↑ 72 Changed 7 months ago by egourgoulhon

Replying to gh-DeRhamSource:

Anything else? :)

Otherwise I'd start programming the next part - namely characteristic classes.

Thanks for the update and your work on this. I am OK for the positive review. Travis, do you agree?

comment:74 Changed 7 months ago by tscrim

  • Status changed from needs_review to positive_review

I concur.

comment:75 Changed 7 months ago by vbraun

  • Branch changed from public/manifolds/mixed_differential_forms to cdfaf6d16322ef4c7d014680ebf025f272f62eb6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.