#30251 closed enhancement (fixed)
ExtPowerFreeModule, ExtPowerDualFreeModule: Simplify _repr_
Reported by:  mkoeppe  Owned by:  

Priority:  trivial  Milestone:  sage9.2 
Component:  linear algebra  Keywords:  
Cc:  egourgoulhon, tscrim, ghmjungmath  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Eric Gourgoulhon 
Report Upstream:  N/A  Work issues:  
Branch:  47caa8a (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description
... using ZZ.ordinal_str()
Change History (19)
comment:1 Changed 7 months ago by
 Branch set to u/mkoeppe/extpowerfreemodule__extpowerdualfreemodule__simplify__repr_
comment:2 Changed 7 months ago by
 Commit set to 56691cd0fce13943886267de00217e7f64815484
 Status changed from new to needs_review
comment:3 Changed 7 months ago by
Nice simplification! To secure it, one should probably perform
 self._degree = degree + self._degree = ZZ(degree)
in the __init__
method of ExtPowerFreeModule
and ExtPowerDualFreeModule
.
comment:4 Changed 7 months ago by
Good idea!
comment:5 Changed 7 months ago by
 Commit changed from 56691cd0fce13943886267de00217e7f64815484 to 47caa8aa7a3c4c65c553661730d7c9d2cdd88455
Branch pushed to git repo; I updated commit sha1. New commits:
47caa8a  ExtPowerFreeModule, ExtPowerDualFreeModule: Make sure _degree is an Integer

comment:6 Changed 7 months ago by
 Commit changed from 47caa8aa7a3c4c65c553661730d7c9d2cdd88455 to 6f1bc22335560496f7a09d72b9a0635bb6b21ee5
(pushed to wrong ticket)
comment:7 Changed 7 months ago by
 Commit changed from 6f1bc22335560496f7a09d72b9a0635bb6b21ee5 to 47caa8aa7a3c4c65c553661730d7c9d2cdd88455
(repaired push to wrong ticket)
comment:8 followups: ↓ 9 ↓ 10 Changed 7 months ago by
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.
comment:9 in reply to: ↑ 8 Changed 7 months ago by
Replying to ghmjungmath:
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.
comment:10 in reply to: ↑ 8 ; followups: ↓ 11 ↓ 12 ↓ 18 Changed 7 months ago by
Replying to ghmjungmath:
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 themanifolds
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 antide 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!
comment:11 in reply to: ↑ 10 ; followup: ↓ 16 Changed 7 months ago by
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)
comment:12 in reply to: ↑ 10 Changed 7 months ago by
Thanks for pointing this out in such detail, Eric. I was not sure how "deep" the doctesting of the patchbot actually goes.
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.
I didn't mean to be dismissive, sorry. I think the modifications and the effort by Matthias are extremely beneficial to the project. :)
comment:13 Changed 7 months ago by
No worries!
comment:14 Changed 7 months ago by
 Reviewers set to Eric Gourgoulhon
 Status changed from needs_review to positive_review
Good to go. Thanks!
comment:15 Changed 7 months ago by
Thank you!
comment:16 in reply to: ↑ 11 Changed 7 months ago by
Replying to mkoeppe:
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?
The large CPU times could prevent to add these examples to the thematic tutorials, which are doctested at each run of make ptestlong
. For instance, if you look at the last cell of the Kerr notebook, you see that it takes about 6 min when running in parallel on 8 processes. NB: most of this CPU time is not spent in the actual tensor calculations, but in the simplifications of the involved symbolic expressions.
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)
Yes, we should open a ticket for this.
comment:17 Changed 7 months ago by
 Branch changed from u/mkoeppe/extpowerfreemodule__extpowerdualfreemodule__simplify__repr_ to 47caa8aa7a3c4c65c553661730d7c9d2cdd88455
 Resolution set to fixed
 Status changed from positive_review to closed
comment:18 in reply to: ↑ 10 ; followup: ↓ 19 Changed 7 months ago by
 Commit 47caa8aa7a3c4c65c553661730d7c9d2cdd88455 deleted
Replying to egourgoulhon:
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.
Or use nbval.
comment:19 in reply to: ↑ 18 Changed 7 months ago by
Replying to slelievre:
Replying to egourgoulhon:
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.
Or use nbval.
Thank you Samuel for the tip! I was not aware of nbval.
New commits:
ExtPowerFreeModule, ExtPowerDualFreeModule._repr_: Simplify by using the ordinal_str method