simon-king-jena commented 9 years ago

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 5a5e35d to 35eb6c2

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

35eb6c2 Remove arguments from __cinit__

simon-king-jena commented 9 years ago

comment:37

Apparently it was needed to remove optional arguments from __cinit__. Dunno if the behaviour of __cinit__ or PY_NEW has changed recently.

simon-king-jena commented 9 years ago

Changed work issues from Fix segfaults to none

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

c89be23 Fix multiplication path*monomial*path

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 35eb6c2 to c89be23

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from c89be23 to f30e7f7

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

`3e94fef`	`Merge with 6.7.beta4, cope with the change of _cmp_c_impl -> _cmp_`
`93f5310`	`Address reviewer's comments`
`cbde0c3`	`Elaborate on the meaning of gcd for paths`
`e8d802e`	`Merge branch 't/17435/cythonise_path_algebra_elements_6.7.beta' into t/17435/cythonise_path_algebra_elements_6.7.beta4`
`f30e7f7`	Change _cmp_c_impl -> `_cmp_` for path algebra elements

simon-king-jena commented 9 years ago

comment:41

I had to resolve a merge conflict at #16453, and I had to cope with the recent change from _cmp_c_impl to _cmp_ for elements. All tests in sage.quivers pass.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from f30e7f7 to ecf87e1

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

`9168ed3`	`Some Cython tweaks for quiver paths`
`3f0adee`	`Use cached_method on PathSemigroup.reverese, for efficiency`
`028c85d`	`Store edge tuple as attribute of path semigroups; length zero path evaluate to false; fix some typos`
`2dd3206`	`Fix path slicing with step -1, and add test`
`ecf87e1`	`Merge branch 't/16453/cythonize_quiver_paths_6.7.beta4' into t/17435/cythonise_path_algebra_elements_6.7.beta4`

simon-king-jena commented 9 years ago

comment:43

There was a conflict, hence, I think I had to push a merge commit...

simon-king-jena commented 9 years ago

Work Issues: Resolve yet another merge conflict

simon-king-jena commented 9 years ago

comment:44

ANOTHER merge conflict???? It really sucks.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

330baeb Merge branch 'public/17435/cythonise_path_algebra_elements' of git://trac.sagemath.org/sage into t/17435/cythonise_path_algebra_elements_6.8.beta2

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from ecf87e1 to 330baeb

simon-king-jena commented 9 years ago

comment:46

Conflict resolved, tests pass. Needs review.

simon-king-jena commented 9 years ago

Changed work issues from Resolve yet another merge conflict to none

simon-king-jena commented 9 years ago

Changed keywords from path algebra elements to path algebra elements SageDays64.5

simon-king-jena commented 9 years ago

comment:48

Remark: The failing test on some patchbot is in pexpect, hence, unrelated to this ticket.

simon-king-jena commented 9 years ago

Changed keywords from path algebra elements SageDays64.5 to path algebra elements, days64.5, days65

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 330baeb to 21da25a

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

21da25a Add a method to determine the complement of a subpath

simon-king-jena commented 9 years ago

comment:52

The previous commit adds a method for paths, that should be useful in Gröbner basis computations and thus somehow belongs to a ticket on path algebras.

1adc0eef-8957-46d9-975b-2dd71dfbd9ba commented 9 years ago

Work Issues: Yet another merge conflict

simon-king-jena commented 9 years ago

comment:54

Are you kidding?? There really is another merge conflict? Unbelievable!

simon-king-jena commented 9 years ago

comment:55

The conflict appears to be with #16064.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 21da25a to 0e09405

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

0e09405 Resolve merge conflict with trac #16064

simon-king-jena commented 9 years ago

Changed work issues from Yet another merge conflict to none

simon-king-jena commented 9 years ago

comment:57

Should be good now.

simon-king-jena commented 9 years ago

comment:58

The failing test shown by the patchbot is in pexpect and clearly unrelated with this ticket. Can somebody try to review it, before it starts bitrotting again?

simon-king-jena commented 9 years ago

comment:59

Replying to @simon-king-jena:

Can somebody try to review it, before it starts bitrotting again?

I am afraid it happened again. This time, Py_NEW won't work any longer, since a cythoned class is now not a type any longer (if I understand correctly).

In addition to that, I'd like to change a couple of things that are related with my upcoming application (F5 algorithm for path algebras), namely a framework to use Schreyer orderings. They may be useful when computing Syzygies (as experience in the commutative case shows). Let I_i be the i-th generator of a two-sided free module, and a*I_i*b be a monomial (paths a, b). A Schreyer ordering is given by choosing, for each i, a path s_i, and then comparison of a*I_i*b and c*I_j*d involves a comparison of the paths a*s_i*b and c*s_j*d.

I believe such a change belongs here (even though the applications do not appear here).

simon-king-jena commented 9 years ago

Work Issues: Remove Py_NEW, prepare Schreyer orderings

simon-king-jena commented 9 years ago

comment:60

PS: I see that I here use a "cutoff" for monomials (longer monomials are set to zero). I won't use that in my applications and thus remove that, too.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 0e09405 to 10c8167

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

`45ac933`	`Merge branch 'develop' into t/17435/cythonise_path_algebra_elements`
`10c8167`	`Prepare Schreyer orderings for later applications`

simon-king-jena commented 9 years ago

comment:62

Since Py_NEW only is a problem with the latest beta, I merged with develop. Then, I removed the "cutoff" thingy that has turned out (in my private work branch) to lead to nothing, and instead I provided some framework for Schreyer orderings (although they aren't used here). And then I improved clarity of some comments.

Note that basic arithmetic is still faster than with Letterplace (which could be because of a memory leak in Letterplace that meanwhile is fixed upstream):

sage: L.<x,y,z> = FreeAlgebra(GF(25,'t'), implementation='letterplace')
sage: %timeit p = x^4+x*y*x*z+2*z^2*x*y
The slowest run took 12.06 times longer than the fastest. This could mean that an intermediate result is being cached 
1000 loops, best of 3: 394 µs per loop
sage: %timeit p = x^4+x*y*x*z+2*z^2*x*y
1000 loops, best of 3: 395 µs per loop
sage: p = x^4+x*y*x*z+2*z^2*x*y
sage: %timeit q = p^7
The slowest run took 4.78 times longer than the fastest. This could mean that an intermediate result is being cached 
100 loops, best of 3: 2.27 ms per loop
sage: %timeit q = p^7
100 loops, best of 3: 2.39 ms per loop

versus

sage: P = DiGraph({1:{1:['x','y','z']}}).path_semigroup().algebra(GF(25,'t'))
sage: P.inject_variables()
Defining e_1, x, y, z
sage: %timeit p = x^4+x*y*x*z+2*z^2*x*y
The slowest run took 24.51 times longer than the fastest. This could mean that an intermediate result is being cached 
10000 loops, best of 3: 20.7 µs per loop
sage: %timeit p = x^4+x*y*x*z+2*z^2*x*y
The slowest run took 4.05 times longer than the fastest. This could mean that an intermediate result is being cached 
10000 loops, best of 3: 21.2 µs per loop
sage: p = x^4+x*y*x*z+2*z^2*x*y
sage: %timeit q = p^7
1000 loops, best of 3: 1.69 ms per loop

Tests pass and it still needs review!

simon-king-jena commented 9 years ago

Changed work issues from Remove Py_NEW, prepare Schreyer orderings to none

fchapoton commented 9 years ago

comment:63

Is the change to module_list.py really needed ?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

e2a27cc Revert unnecessary change to module_list.py

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 10c8167 to e2a27cc

simon-king-jena commented 9 years ago

comment:65

Replying to @fchapoton:

Is the change to module_list.py really needed ?

You are right, it isn't.

fchapoton commented 9 years ago

comment:66

Hello,

I am not able to do a serious technical review (cython..), but I am happy enough with what I understand, and the bot gives a green light. So I am ready to give a positive review, if you are satisfied with this rather superficial check.

One minor point: I would prefer to have a bigger example for the latex method, where \cdot appears.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from e2a27cc to 63f08e1

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

63f08e1 Add a stronger test to the `_latex_` method

simon-king-jena commented 9 years ago

comment:68

Replying to @fchapoton:

One minor point: I would prefer to have a bigger example for the latex method, where \cdot appears.

Good idea. I think the additional test is better, and one can also check manually that the product is correctly computed.

Perhaps the release manager can decide if your non-technical review is enough?

fchapoton commented 9 years ago

comment:69

Some more comments, review in progress:

1) Why is there no "except -1" at the end of this line:

+    cpdef tuple complement(self, QuiverPath subpath)
     cpdef bint has_subpath(self, QuiverPath subpath) except -1

of "src/sage/quivers/paths.pxd" ?

2) According to the new code in src/sage/quivers/algebra.py, it is now possible, if A is a path algebra, to call A(a dictionary here). Is this tested ?

simon-king-jena commented 9 years ago

comment:70

Replying to @fchapoton:

Some more comments, review in progress:

1) Why is there no "except -1" at the end of this line:
+    cpdef tuple complement(self, QuiverPath subpath)
     cpdef bint has_subpath(self, QuiverPath subpath) except -1
of "src/sage/quivers/paths.pxd" ?

Which line do you mean? cpdef tuple complement...? Here, the return type is a python object (tuple), which means that you can not set (and also do not need to set) an except value.

2) According to the new code in src/sage/quivers/algebra.py, it is now possible, if A is a path algebra, to call A(a dictionary here). Is this tested ?

I'll have a look. If it is possible, then it should be tested. Right now, I try to speed-up a couple of things.

fchapoton commented 9 years ago

comment:71

Ok.

3) Please remove the final dot in the title (first line) of algebra_elements.pyx

and 2) is indeed tested here:

sage: P(p.monomial_coefficients()) == p

sagemath / sage

Cythonise path algebra elements #17435

16453 provides a Cython version of quiver paths. The purpose of this ticket is to introduce a Cython implementation of path algebra elements.