New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make Coxeter3 a new-style spkg #19178
Comments
Author: Travis Scrimshaw |
Commit: |
This comment has been minimized.
This comment has been minimized.
comment:1
IIRC the source code in the spkg version of coxeter3 is a modified version on http://math.univ-lyon1.fr/~ducloux/coxeter/coxeter3/english/coxeter3_e.html. Nicolas, Anne, can one of you confirm this? New commits:
|
comment:2
The bot complains:
|
Attachment: coxeter3-1.1.tar.gz |
comment:4
Hmm...I thought I did pass the gz flag. Perhaps this one will now work. |
This comment has been minimized.
This comment has been minimized.
comment:6
ok, it was just because the link in the description was not an upload link (it was attachment instead of raw-attachment). |
comment:7
I seem to get a lot of doctest failures after installing this package:
Also, there are deprecation warnings when installing this package:
|
comment:9
Changes over the years to the coercion system have made it so that if an element has a I also renamed the |
comment:11
I couldn't resist going in and making some other improvements to the code:
versus previously
In summary, I got a ~2x speedup from not converting to a string as an intermediate object (but I'm probably taking advantage of recent upgrades in cython to do so). I also avoiding creating things in the fraction field for the non-constant coefficient KL polynomials. There were some other micro-optimizations and things I changed for better practices. |
comment:12
I should also note that the non-constant coefficient KL polynomials are now sparse to avoid filling up memory with a lot of long lists full of 0's. |
comment:13
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:24
I've added a patch which gets rid of the deprecation warnings. Note, I have begun taking over the project as upstream and welcome help (I've updated |
Reviewer: Jeroen Demeyer |
comment:25
Replying to @tscrim:
Sorry, not me. Preferably the review should be done by somebody who actually knows the package. |
comment:26
Franco is visiting me for a couple of days, so I am going to try to review this patch with him! |
Changed reviewer from Jeroen Demeyer to none |
comment:28
The weird thing is that when I run
I get errors. But I think this is due to the fact that it only runs the coxeter3 tests and not the other ones. Is this new? I do not remember that this is how it was before. Running
works fine. |
comment:29
In |
comment:30
This looks good to me. Please run pyflakes to detect unused variables (like the Then Anne can set it to positive review on my behalf. :-) |
Reviewer: Franco Saliola |
Branch pushed to git repo; I updated commit sha1. New commits:
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:33
Done. Anne is correct, it was only running the
If you want to limit the optional tests, you can use |
Changed keywords from none to coxeter3 |
Changed reviewer from Franco Saliola to Franco Saliola, Anne Schilling |
comment:35
Looks good now. |
comment:36
Thank you to all. |
Changed branch from u/tscrim/coxeter3_spkg_update-19178 to |
Convert Coxeter3 to a new-style spkg.
tarball
CC: @sagetrac-sage-combinat @nthiery @anneschilling @jdemeyer @vbraun @saliola
Component: packages: optional
Keywords: coxeter3
Author: Travis Scrimshaw
Branch/Commit:
2945b61
Reviewer: Franco Saliola, Anne Schilling
Issue created by migration from https://trac.sagemath.org/ticket/19178
The text was updated successfully, but these errors were encountered: