#27430 closed enhancement (fixed)

more choices of algorithms for Coxeter Smith form of posets

Reported by: chapoton Owned by:
Priority: minor Milestone: sage-8.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) Commit: aa07710ce924921675135496cf7faa1f8f62992e
Dependencies: Stopgaps:

Description

This allows to compare the speed of all those.

Change History (19)

comment:1 Changed 21 months ago by chapoton

  • Branch set to u/chapoton/27430
  • Commit set to 96b815023ed635532ec72d02e49c99afb76fa42f
  • Status changed from new to needs_review

New commits:

96b8150more algorithm for coxeter_smith_form in posets

comment:2 Changed 21 months ago by tscrim

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 21 months ago by git

  • Commit changed from 96b815023ed635532ec72d02e49c99afb76fa42f to 5dcd3cd304b94e70dc936f3a9cc22e8c2d37ee3a

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

5dcd3cdtrac 27430 some details

comment:4 Changed 21 months ago by chapoton

thx, done

comment:5 Changed 21 months ago by mantepse

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()

comment:6 Changed 21 months ago by mantepse

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 21 months ago by chapoton

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.

Last edited 21 months ago by chapoton (previous) (diff)

comment:8 follow-up: Changed 21 months ago by 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?

comment:9 in reply to: ↑ 8 Changed 21 months ago by mantepse

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 21 months ago by chapoton

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

comment:11 Changed 21 months ago by chapoton

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

comment:12 Changed 21 months ago by mantepse

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

comment:13 Changed 21 months ago by chapoton

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 21 months ago by mantepse

  • Branch changed from u/chapoton/27430 to u/mantepse/27430

comment:15 Changed 21 months ago by chapoton

  • Commit changed from 5dcd3cd304b94e70dc936f3a9cc22e8c2d37ee3a to aa07710ce924921675135496cf7faa1f8f62992e

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:

6a548d9tentative code for friacs algorithm in posets
aa07710tweak fricas code so variables don't leak

comment:16 Changed 21 months ago by mantepse

Dear Frederic !

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

comment:17 Changed 21 months ago by chapoton

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

comment:18 Changed 21 months ago by tscrim

  • Reviewers set to Travis Scrimshaw, Martin Rubey
  • Status changed from needs_review to positive_review

Yep, LGTM. Thanks.

comment:19 Changed 21 months ago by vbraun

  • Branch changed from u/mantepse/27430 to aa07710ce924921675135496cf7faa1f8f62992e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.