Opened 7 years ago

Last modified 7 years ago

#13425 closed enhancement

Compute mutation type of a ClusterSeed or ClusterQuiver — at Version 13

Reported by: gmoose05 Owned by: sage-combinat
Priority: major Milestone: sage-5.9
Component: combinatorics Keywords: cluster algebra, quiver, days45
Cc: Merged in:
Authors: Gregg Musiker, Christian Stump Reviewers: Christian Stump, Gregg Musikier
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13424 Stopgaps:

Description (last modified by stumpc5)

This ticket implements mutation type checks for cluster seeds and quivers.

Exceptional mutation classes are precomputed and contained in the following package:

This package is not necessary for the features to work but speed things quite a bit.

Change History (14)

comment:1 Changed 7 years ago by gmoose05

  • Dependencies set to 13424

comment:2 Changed 7 years ago by gmoose05

  • Dependencies changed from 13424 to #13424

comment:3 Changed 7 years ago by gmoose05

  • Authors changed from Gregg Musiker and Christian Stump to Gregg Musiker, Christian Stump

comment:4 Changed 7 years ago by stumpc5

The tests become much too slow with this patch:

with only #13424 applied:

sage -t  "devel/sage-combinat/sage/combinat/cluster_algebra_quiver/"
	 [3.3 s]
sage -t  "devel/sage-combinat/sage/combinat/cluster_algebra_quiver/"
	 [2.2 s]

and with this patch applied as well:

sage -t  "devel/sage-combinat/sage/combinat/cluster_algebra_quiver/"
	 [33.5 s]
sage -t  "devel/sage-combinat/sage/combinat/cluster_algebra_quiver/"
	 [32.8 s]

We should avoid that.

comment:5 Changed 7 years ago by stumpc5

  • Status changed from new to needs_review

comment:6 Changed 7 years ago by etn40ff

  • Keywords days45 added

comment:7 Changed 7 years ago by stumpc5

Hi Gregg!

I know you are again working on this patch: it needs to be (almost trivially) rebased agains the newest version of #13424.

comment:8 Changed 7 years ago by gmoose05

Just uploaded a review patch (although I'm still working through it and this is just partial work).

The bulk of the changes are from

I documented and added examples for all methods until the testing commands near the end, and _connected_mutation_type which I still need to read and understand.

I also changed auxiliary command names so that they begin with an underscore.

Christian, there were a few places where I had questions for you, these are indicated by the word "CHECK" in caps.

More to come later as I look through the other .py files in this patch also.


comment:9 Changed 7 years ago by gmoose05

Christian, I just posted a new review patch. I think the ticket is not too far away from being ready. I documented as much of the code as I could with comments, and added explanations and examples for almost all methods.

There are a few outstanding comments or questions marked with CHECK.

Also, I see that in we add mutation_type() commands every time the user asks for a Cluster Variable or related command. However, this greatly slows down those methods with the only advantage being the almost_positive_roots command. So I figured we should discuss this over email or skype at some point.

We also discussed at Sage Days about trying to optimize by having the program test for infinite_mutation_type by randomly mutating a few times if more than 6 vertices before trying to test for a recognizable mutation_type. I haven't made such changes.


comment:10 Changed 7 years ago by stumpc5

Hi Gregg -- thanks a lot for the hard work you are putting into my unreadable code! I am about to upload a review patch. Most importantly, I have made the random tests work again and did quite some test to ensure the code does what it is supposed to do.

I solved some of the CHECK's, I think it is better to discuss the others on the phone. Before giving the code a positive review, I think we should have a strong cpu make some tests and random tests for rank 8-12 since I think I never did that (though I also think that the problems should occur already earlier).

Cheers, Christian

comment:11 Changed 7 years ago by stumpc5

Hi Gregg, I uploaded a new patch.

The main changes are:

  • the mutation type check is now done in the order:
    • check if it is mutation infinite
    • look in existing files
    • use the big algorithm to determine the type
    • generate the files and look in these newly generated files

(I just realize that this results in two times looking in the data if it already exists. I should change that).

  • I moved the is_mutation_finite into mutation_type and take a matrix as input. I did this because this is quicker than computing the quiver and then the matrix again.
  • I rewrote the code to construct the classical and exceptional data. Please have a look and say what you think.

The old mutation type check was actually very buggy. We still have a problem to solve: if you want to test the mutation type of ClusterQuiver([['A',3],['B',2],['T',(4,4,4)]]), you will get an error since the third part is "unknown infinite" but the program is trying to build a reducible type from it.

I would also appreciate if you could try to add some more tests to the mutation type and the is_finite_type tests and check if I didn't introduce any new bugs...

Cheers, Christian

comment:12 Changed 7 years ago by stumpc5

Concerning the spkg: the method is now written in a way that the algorithm tries to be fast both if the data files are present or not. If this ticket is ready before we get the spkg ready I would this vote for getting it in 5.9 rather than wait for the spkg.

Changed 7 years ago by stumpc5

comment:13 Changed 7 years ago by stumpc5

  • Description modified (diff)
  • Reviewers set to Christian Stump, Gregg Musikier
Note: See TracTickets for help on using tickets.