Opened 9 months ago

Closed 9 months ago

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

thx, done

comment:5 Changed 9 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 9 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 9 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 9 months ago by chapoton (previous) (diff)

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

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

comment:11 Changed 9 months ago by chapoton

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

comment:12 Changed 9 months ago by mantepse

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

comment:13 Changed 9 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 9 months ago by mantepse

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

comment:15 Changed 9 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 9 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 9 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 9 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 9 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.