ExtPowerFreeModule, ExtPowerDualFreeModule: Simplify _repr_

[mkoeppe]

... using ZZ.ordinal_str()

CC: @egourgoulhon @tscrim @mjungmath

Component: linear algebra

Author: Matthias Koeppe

Branch: 47caa8a

Reviewer: Eric Gourgoulhon

Issue created by migration from https://trac.sagemath.org/ticket/30251

[mkoeppe]

Branch: u/mkoeppe/extpowerfreemodule__extpowerdualfreemodule__simplify__repr_

[mkoeppe]

Commit: 56691cd

[mkoeppe]

New commits:

56691cd

ExtPowerFreeModule, ExtPowerDualFreeModule._repr_: Simplify by using the ordinal_str method

[egourgoulhon]

comment:3

Nice simplification! To secure it, one should probably perform

- self._degree = degree
+ self._degree = ZZ(degree)

in the __init__ method of ExtPowerFreeModule and ExtPowerDualFreeModule.

[mkoeppe]

comment:4

Good idea!

[sagetrac-git]

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

47caa8a

ExtPowerFreeModule, ExtPowerDualFreeModule: Make sure _degree is an Integer

[sagetrac-git]

Changed commit from 56691cd to 47caa8a

[sagetrac-git]

comment:6

(pushed to wrong ticket)

[sagetrac-git]

Changed commit from 47caa8a to 6f1bc22

[sagetrac-git]

Changed commit from 6f1bc22 to 47caa8a

[sagetrac-git]

comment:7

(repaired push to wrong ticket)

[mjungmath]

comment:8

This is just a general comment not only related to this ticket (I picked this ticket randomly), but worth to emphasize once again: the deeper you dig and the more comprehensive your modifications are in the tensor module, the more likely it may corrupt the manifolds code, as there is a very strong dependence. Please be aware of that fact. I don't think it will go so far, but it would be very sad if the manifolds module would be unusable in the upcoming version of Sage.

Furthermore, since the manifolds module is similarly structured, it is very likely that analogous improvements can be applied there, too.

[mkoeppe]

comment:9

Replying to @mjungmath:

the deeper you dig and the more comprehensive your modifications are in the tensor module, the more likely it may corrupt the manifolds code, as there is a very strong dependence.

Thanks. Well, that's why we have doctests and peer review. I run the doctests also in the manifolds module -- and the patchbot runs all doctests.

Furthermore, since the manifolds module is similarly structured, it is very likely that analogous improvements can be applied there, too.

Absolutely. I'll hope to do that when I have learned more about the manifolds code (I hope to be using this code soon for some of my research), or when it becomes necessary because of changes in the tensor modules code. Do feel free to make these changes if you would like them earlier.

[egourgoulhon]

comment:10

Replying to @mjungmath:

This is just a general comment not only related to this ticket (I picked this ticket randomly), but worth to emphasize once again: the deeper you dig and the more comprehensive your modifications are in the tensor module, the more likely it may corrupt the manifolds code, as there is a very strong dependence. Please be aware of that fact. I don't think it will go so far, but it would be very sad if the manifolds module would be unusable in the upcoming version of Sage.

Thanks for expressing your concern. However, as Matthias pointed out in his reply (comment:9), doctests shall prevent breakage in the manifold part. It remains true that doctests are not exhaustive and it will be more satisfactory to have a more comprehensive test suite, albeit more CPU consuming. Such a suite could be derived from the manifold examples page. I guess one shall first turn these Jupyter notebooks into rst files, via the export menu. When there is a significant change in the manifold/tensor code, I usually run by hand some selected examples from the above page, typically, the S2 sphere, the S3 one, the Kerr spacetime (good test of parallel computation of the Riemann tensor) and the anti-de Sitter space. An automated test suite would be of great help in this respect.

Another concern one might have is performance, which is not tracked by doctests, but could be in the test suite (note that there is a total CPU time measurement in the Kerr notebook mentioned above).

That being said, I think the changes in the tensor modules proposed by Matthias are great improvements. In particular, they shall make FiniteRankFreeModule usable for other purposes than tensor field computations on smooth manifolds.

Furthermore, since the manifolds module is similarly structured, it is very likely that analogous improvements can be applied there, too.

Indeed!

[mkoeppe]

comment:11

Replying to @egourgoulhon:

It remains true that doctests are not exhaustive and it will be more satisfactory to have a more comprehensive test suite, albeit more CPU consuming. Such a suite could be derived from the manifold examples page. I guess one shall first turn these Jupyter notebooks into rst files, via the export menu.

+1. Could they possibly be added as thematic tutorials in the main sage tree, or are there technical limitations that would prevent that?

Another concern one might have is performance, which is not tracked by doctests, but could be in the test suite (note that there is a total CPU time measurement in the Kerr notebook mentioned above).

Yes, this is an important point - and one for which Sage unfortunately does not have a very good solution. Sure - we notice performance regressions sometimes when doctests suddenly time out. But obviously doctests are not very well suited as benchmarks. (One might think about "# optional - benchmark" annotations though...)

(By the way, some of the doctests in sage.manifolds probably should be marked "long" -- I frequently see them time out in the automated tests on GitHub actions - see for example https://github.com/sagemath/sage/runs/910575697?check_suite_focus=true)