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
Cythonise path algebra elements #17435
Comments
Changed keywords from none to path algebra elements |
This comment has been minimized.
This comment has been minimized.
Author: Simon King |
Dependencies: #16453 |
Commit: |
Last 10 new commits:
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:6
I just noticed a problem, most likely in
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:9
Fixed. The problem was that under certain circumstances, the computation Now the tests work. I also included a comparison with the letterplace implementation of free associative algebras. Letterplace is faster, but it is restricted to homogeneous elements. |
comment:10
PS: I wonder whether I should implement Karatsuba multiplication... |
comment:12
I did two changes:
Now, at least according to few benchmarks, the arithmetic for path algebra elements is faster than the arithmetic for both available implementations of free associative algebra elements (one of them, letterplace, is available for homogeneous elements only). Hence, in the long run, it might be worth while to implement free associative algebras as quiver algebras! At least with |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:14
I forgot to remove some lines that enabled c-profiling (which was useful to get my implementation to better speed). Without it, it becomes a bit faster. |
comment:15
Replying to @simon-king-jena:
... and thus I also updated the timings appearing in the docs. |
comment:138
Replying to @jdemeyer:
I didn't revert all changes, but most of them. Also I changed the signal handling according to your advices. These changes did hardly change the timings for the few examples that I did. As mentioned in comment:136, profiling indicated that I should use more care for deallocation resp. for the freelist. I did as follows:
Results
The difference in the profiling between biseq_init_concat and mon_mul_path indicates how much time is spent for memory allocation of That would be similar to what is done for biseq_t and seemed to yield an improvement. I couldn't do the same trick for terms, because I have linked lists of terms. But for monomials it would perhaps make sense. If someone likes to finish reviewing: Go ahead! If changing the monomials has a good effect, then I can still open a new ticket, or post it here if the review isn't finished yet. |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:140
It was easy to move the monomials from heap to stack. With the latest commit, it is still interruptible, arithmetic still runs without memory leak, and the timing becomes
So, the small example stays as it was, but the large example improved by as much as 22%!! The profile:
The difference between biseq_init_concat and mon_mul_path has disappeared, which obviously is a consequence of using the stack (before, there was an expensive Also, tests still pass. I think I am done now! At least unless I find that after all these changes I find a benefit in inlining some monomial-related functions... |
comment:141
Replying to @simon-king-jena:
I just noticed that all Therefore, I will abstain from playing with inline now. I will stop coding now, so, please continue reviewing! |
comment:142
I would like to have a patchbot's green light on the latest version.. |
comment:143
There are two failing doctests, see patchbot report. |
comment:144
Why all the changes inside |
comment:145
Replying to @jdemeyer:
I'll have a look (after #12103, this here is the next I want to finally finish). Question: How should I change it? By rebasing and force-push? Or by a commit that reverses the changes and is perhaps reversed again on another ticket? Do you think the little changes in |
comment:146
Replying to @simon-king-jena:
Another commit; it's good to have history which shows things you did (because you might want to look at/use them later). Plus you would not be changing history. |
comment:147
Replying to @tscrim:
... IF it there is some likelyhood that other people are already using the branch and thus needed to rebase their work if I rebase the branch. I asked because I am not sure if people do use the branch. I do, but I think I don't count here. Anyway, let's do it with a new commit. |
comment:150
Not to mention that unless they also modify your code in their branch (and not just "use it") then their rebase would be totally trivial. Nathann |
comment:151
ok, my patchbot has given a green light, and I have no further comments on the code. Any further enhancement could be done in another ticket. So positive review. |
Reviewer: Frédéric Chapoton |
comment:152
It took a very long time to finish this ticket (I had the first version of the code on my laptop two years ago), but the comments of the referees have been very helpful and resulted in major improvements. Jeroen, shouldn't your name be added to the list of reviewers as well? Thank you very much! |
Changed branch from public/17435/cythonise_path_algebra_elements to |
#16453 provides a Cython version of quiver paths. The purpose of this ticket is to introduce a Cython implementation of path algebra elements.
Note that the arithmetic appears to be faster than the current default implementation of free associative algebras. So, it might make sense to use (the more general) path algebras to become the default for free associative algebras.
The next step shall be to implement computation of Gröbner bases. minimal generating sets and minimal projective resolutions for modules over path algebras, with a non-commutative F5 algorithm.
Depends on #16453
Depends on #17526
CC: @nthiery @nathanncohen @egunawan
Component: algebra
Keywords: path algebra elements, days64.5, days65
Author: Simon King
Branch/Commit:
5b4ed6e
Reviewer: Frédéric Chapoton
Issue created by migration from https://trac.sagemath.org/ticket/17435
The text was updated successfully, but these errors were encountered: