Opened 3 years ago
Closed 3 years ago
#27430 closed enhancement (fixed)
more choices of algorithms for Coxeter Smith form of posets
Reported by:  chapoton  Owned by:  

Priority:  minor  Milestone:  sage8.7 
Component:  combinatorics  Keywords:  
Cc:  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Travis Scrimshaw, Martin Rubey 
Report Upstream:  N/A  Work issues:  
Branch:  aa07710 (Commits, GitHub, GitLab)  Commit:  aa07710ce924921675135496cf7faa1f8f62992e 
Dependencies:  Stopgaps: 
Description
This allows to compare the speed of all those.
Change History (19)
comment:1 Changed 3 years ago by
 Branch set to u/chapoton/27430
 Commit set to 96b815023ed635532ec72d02e49c99afb76fa42f
 Status changed from new to needs_review
comment:2 Changed 3 years ago by
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.
comment:3 Changed 3 years ago by
 Commit changed from 96b815023ed635532ec72d02e49c99afb76fa42f to 5dcd3cd304b94e70dc936f3a9cc22e8c2d37ee3a
Branch pushed to git repo; I updated commit sha1. New commits:
5dcd3cd  trac 27430 some details

comment:4 Changed 3 years ago by
thx, done
comment:5 Changed 3 years ago by
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 nonleaking statement. Roughly, it will be
(fricas([x]*n).diagonalMatrix()  fricas(c0)).smith().diagonal().sage()
comment:6 Changed 3 years ago by
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?
comment:7 Changed 3 years ago by
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.
comment:8 followup: ↓ 9 Changed 3 years ago by
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?
comment:9 in reply to: ↑ 8 Changed 3 years ago by
Replying to chapoton:
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.
comment:10 Changed 3 years ago by
Ok. Then please try to provide a working better alternative. There is no urgency.
comment:11 Changed 3 years ago by
I tried the obvious modification for Fricas, as you suggested, and it failed.
comment:12 Changed 3 years ago by
Could you post the code? I can look at it tonight (probably after 19:00)
comment:13 Changed 3 years ago by
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.
comment:14 Changed 3 years ago by
 Branch changed from u/chapoton/27430 to u/mantepse/27430
comment:15 Changed 3 years ago by
 Commit changed from 5dcd3cd304b94e70dc936f3a9cc22e8c2d37ee3a to aa07710ce924921675135496cf7faa1f8f62992e
comment:16 Changed 3 years ago by
Dear Frederic !
sorry, I didn't know that you wouldn't see it. Yes, this should work with FriCAS.
comment:17 Changed 3 years ago by
ok, the fricas code works and looks good to me. Travis, do you agree that this is ready to go?
comment:18 Changed 3 years ago by
 Reviewers set to Travis Scrimshaw, Martin Rubey
 Status changed from needs_review to positive_review
Yep, LGTM. Thanks.
comment:19 Changed 3 years ago by
 Branch changed from u/mantepse/27430 to aa07710ce924921675135496cf7faa1f8f62992e
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
more algorithm for coxeter_smith_form in posets