more choices of algorithms for Coxeter Smith form of posets

[fchapoton]

This allows to compare the speed of all those.

Component: combinatorics

Author: Frédéric Chapoton

Branch/Commit: aa07710

Reviewer: Travis Scrimshaw, Martin Rubey

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

[fchapoton]

Branch: u/chapoton/27430

[fchapoton]

Commit: 96b8150

[fchapoton]

New commits:

96b8150

more algorithm for coxeter_smith_form in posets

[tscrim]

comment:2

I think it would be better to have the comments about the (anecdotal) relative speed of the algorithms in the docstring rather than the code. That way it is easier for the (common) user to see it. Otherwise LGTM.

[sagetrac-git]

Changed commit from 96b8150 to 5dcd3cd

[sagetrac-git]

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

5dcd3cd

trac 27430 some details

[fchapoton]

comment:4

thx, done

[mantepse]

comment:5

I cannot try the ticket right now, but I'm afraid that the FriCAS code will leak variables. Could you try

sage: _ = P.coxeter_smith_form(algorithm='fricas')
sage: fricas.eval("Z")
sage: fricas.eval("x")

If it does, I can prepare a non-leaking statement. Roughly, it will be

(fricas([x]*n).diagonalMatrix() - fricas(c0)).smith().diagonal().sage()

[mantepse]

comment:6

A more fundamental question: why not provide the algorithm keyword for matrices in general?

I admit, that I do not know how the matrix design currently works, but possibly sage.matrix.matrix2.smith_form is the right place?

[fchapoton]

comment:7

Well. Indeed. That would go into "matrices with entries in QQ[x]' namely "matrix_polynomial_dense.pyx". Maybe for another ticket, unless you volunteer to do it ?

Now compiling Fricas to check the leak.

[fchapoton]

comment:8

Answer:

sage: p=posets.PentagonPoset()
sage: sage: _ = p.coxeter_smith_form(algorithm='fricas')
....: sage: fricas.eval("Z")
....: sage: fricas.eval("x")
....: 
'\n   Integer\n'
'\n   x\n'

Does this mean that there is a leak?

[mantepse]

comment:9

Replying to @fchapoton:

Answer:

sage: p=posets.PentagonPoset()
sage: sage: _ = p.coxeter_smith_form(algorithm='fricas')
....: sage: fricas.eval("Z")
....: sage: fricas.eval("x")
....: 
'\n   Integer\n'
'\n   x\n'

Does this mean that there is a leak?

Yes, after p.coxeter_smith_form(algorithm='fricas'), the name Z has value Integer in FriCAS. By contrast, x still returns x, which is good.

So, for example, executing after that

sage: var("Z")
sage: fricas(Z^2+1)

will raise a strange error.

[fchapoton]

comment:10

Ok. Then please try to provide a working better alternative. There is no urgency.

[fchapoton]

comment:11

I tried the obvious modification for Fricas, as you suggested, and it failed.

[mantepse]

comment:12

Could you post the code? I can look at it tonight (probably after 19:00)

[fchapoton]

comment:13

I have made a branch public/ticket/27430 with the tentative. I would say that the matrix type is not recognized correctly, so that the method smith does not apply.

[mantepse]

Changed branch from u/chapoton/27430 to u/mantepse/27430

[fchapoton]

Changed commit from 5dcd3cd to aa07710

[fchapoton]

comment:15

Is this a working branch or what ?

Nota bene: If you do not make a comment, I do not see the change of branch.

---

New commits:

6a548d9	tentative code for friacs algorithm in posets
aa07710	tweak fricas code so variables don't leak

[mantepse]

comment:16

Dear Frederic !

sorry, I didn't know that you wouldn't see it. Yes, this should work with FriCAS.

[fchapoton]

comment:17

ok, the fricas code works and looks good to me. Travis, do you agree that this is ready to go?

[tscrim]

Reviewer: Travis Scrimshaw, Martin Rubey

[tscrim]

comment:18

Yep, LGTM. Thanks.

[vbraun]

Changed branch from u/mantepse/27430 to aa07710