Compute mutation type of a ClusterSeed or ClusterQuiver

[sagetrac-gmoose05]

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

Apply to the sage library:

• attachment: trac_13425-mutation_type-gm.patch
• attachment: trac_13425-review-cs.patch
• attachment: trac_13425-review_two-cs.patch
• attachment: trac_13425-small-doc-fix.patch
• attachment: trac_13425-review_four-cs.patch
• attachment: trac_13425-review_five-cs.patch
• attachment: trac_13425-sixth-review-gm.patch

Depends on #13424

Component: combinatorics

Keywords: cluster algebra, quiver, days45

Author: Gregg Musiker, Christian Stump

Reviewer: Christian Stump, Gregg Musiker

Merged: sage-5.9.beta2

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

[sagetrac-gmoose05]

Dependencies: 13424

[sagetrac-gmoose05]

Changed dependencies from 13424 to #13424

[sagetrac-gmoose05]

Changed author from Gregg Musiker and Christian Stump to Gregg Musiker, Christian Stump

[stumpc5]

comment:4

The tests become much too slow with this patch:

with only #13424 applied:

sage -t  "devel/sage-combinat/sage/combinat/cluster_algebra_quiver/cluster_seed.py"
	 [3.3 s]
sage -t  "devel/sage-combinat/sage/combinat/cluster_algebra_quiver/quiver.py"
	 [2.2 s]

and with this patch applied as well:

sage -t  "devel/sage-combinat/sage/combinat/cluster_algebra_quiver/cluster_seed.py"
	 [33.5 s]
sage -t  "devel/sage-combinat/sage/combinat/cluster_algebra_quiver/quiver.py"
	 [32.8 s]

We should avoid that.

[Etn40ff]

Changed keywords from cluster algebra, quiver to cluster algebra, quiver, days45

[stumpc5]

comment:7

Hi Gregg!

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

[sagetrac-gmoose05]

comment:8

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 mutation_type.py:

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.

Gregg

[sagetrac-gmoose05]

comment:9

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 cluster_seed.py 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.

Gregg

[stumpc5]

comment:10

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

[stumpc5]

comment:11

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

[stumpc5]

comment:12

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.

[stumpc5]

Attachment: trac_13425-mutation_type-gm.patch.gz

[stumpc5]

Reviewer: Christian Stump, Gregg Musikier

[stumpc5]

Attachment: trac_13425-review-cs.patch.gz

[stumpc5]

comment:15

There is the green light, finally! Gregg, if you are happy with my latest changes and you can verify that I didn't do any mistakes while folding the patches, please give it a positive review!

[jdemeyer]

comment:17

There are various problem with the documentation:

dochtml.log:[combinat ] /padic/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/combinat/cluster_algebra_quiver/quiver.py:docstring of sage.combinat.cluster_algebra_quiver.quiver.ClusterQuiver.is_finite:5: WARNING: Literal block expected; none found.
dochtml.log:[combinat ] /padic/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/combinat/cluster_algebra_quiver/quiver.py:docstring of sage.combinat.cluster_algebra_quiver.quiver.ClusterQuiver:41: WARNING: Bullet list ends without a blank line; unexpected unindent.
dochtml.log:[combinat ] /padic/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/combinat/cluster_algebra_quiver/quiver_mutation_type.py:docstring of sage.combinat.cluster_algebra_quiver.quiver_mutation_type.save_quiver_data:1: WARNING: Inline literal start-string without end-string.

Also, I wondering whether you are using assert correctly. assert should only be used to check for bugs, not for bad user input. In other words, if there is any way to produce an AssertionError by supplying bad input to a public function, that is by definition a bug. Use ValueError or TypeError (or other exceptions) for bad user input.

[stumpc5]

comment:19

Replying to @jdemeyer:

Hi Gregg, I will upload a fix within the next minutes -- though I don't really find the second doc warning in the docstring.

[stumpc5]

Attachment: trac_13425-review_two-cs.patch.gz

[jdemeyer]

comment:32

Replying to @stumpc5:

What should we thus do to get the output in some generic order?

I have no idea, ask sage-devel.

[jdemeyer]

comment:33

Replying to @stumpc5:

We also had this problem on our machines.

If you knew this problem existed, this ticket should never have gotten positive review...

[nthiery]

comment:34

Replying to @jdemeyer:

Replying to @stumpc5:

What should we thus do to get the output in some generic order?

I have no idea, ask sage-devel.

If you can afford a sort somewhere, a typical trick is to do:

sage: sorted(..., key=str)

And then it will sort according to the string representation which is platform independent.

I usually only use that for sorting the results in the doctests. But in your situation that could be ok too.

Cheers,

[stumpc5]

comment:35

Replying to @nthiery:

If you can afford a sort somewhere, a typical trick is to do:
I usually only use that for sorting the results in the doctests. But in your situation that could be ok too.

Good idea, thanks -- the problem is not at all in a time critical method, so no problem to do your trick!

Replying to @jdemeyer:
If you knew this problem existed, this ticket should never have gotten positive review...

I find it very hard to judge if a problem like this only appears because the machine it happened on is a 32bit Mac - I will anyway post such a problem on sage-devel next time...

[stumpc5]

comment:36

Replying to @stumpc5:

Replying to @nthiery:
Good idea, thanks -- the problem is not at all in a time critical method, so no problem to do your trick!

Done! Nicolas, would you look at this last change and give it a positive review? Cheers!

[stumpc5]

Changed reviewer from Christian Stump, Gregg Musikier to Christian Stump, Gregg Musiker

[stumpc5]

Attachment: trac_13425-review_five-cs.patch.gz

[nthiery]

comment:38

Replying to @stumpc5:

Replying to @stumpc5:

Replying to @nthiery:
Good idea, thanks -- the problem is not at all in a time critical method, so no problem to do your trick!

Done! Nicolas, would you look at this last change and give it a positive review? Cheers!

I checked the patch, and it looks reasonable. You can set the ticket positive review on my behalf as soon as the light goes green and, if you can run them there, the test pass on your other machine which was causing trouble.

[sagetrac-gmoose05]

comment:39

The key=str fix in quiver_mutation_type.py seems to have already solved the issues in save_quiver_data(), etc.

I am about to upload a new patch where we use the key=str for _mutation_type_test() and _random_multi_tests() in mutation_type.py.

Reordering the examples accordingly slightly, this now passes on my machine (the one that was having trouble before). All tests passed!

Christian, assuming that it passes on your machine with the new order of examples, and if you don't see other issues, it should be ready to go.

Is there any way to get the patchbot to run again???

Do you want to fold the patches all into one again? If the patchbot and you are happy, I think it's ready for a positive review on my behalf.

Gregg

[sagetrac-gmoose05]

comment:40

Attachment: trac_13425-sixth-review-gm.patch.gz

I confirmed with Christian that with the new patch, -t -long passes on his machine, and I just successfully ran -testall -long overnight on my machine (which was the one with the slight order difference in the output during a few doctests).

So with this latest patch, it is now ready to go.

Gregg

[kiwifb]

comment:42

Jeroen will want to know and I'd like to know too. Which patch do you need to apply? You should update the summary with the list of patches that needs to be applied for this ticket. We cannot know which of the patch listed in this ticket are to be applied without digging deeper.

[sagetrac-gmoose05]

comment:44

Replying to @kiwifb:

Jeroen will want to know and I'd like to know too. Which patch do you need to apply? You should update the summary with the list of patches that needs to be applied for this ticket. We cannot know which of the patch listed in this ticket are to be applied without digging deeper.

Just modified the description accordingly. All seven patches should be applied in order.

Gregg

[kiwifb]

comment:45

Thanks for the clarification. I have put it in the desired format.

[jdemeyer]

Merged: sage-5.9.beta2

[nexttime]

comment:47

Doctesting devel/sage/sage/combinat/cluster_algebra_quiver/quiver.py with --long takes by far too long, compared to other files.

[nexttime]

comment:48

Doctesting sage/combinat/cluster_algebra_quiver/mutation_type.py with --long also takes pretty long.

[nexttime]

comment:49

Replying to @nexttime:

Doctesting devel/sage/sage/combinat/cluster_algebra_quiver/quiver.py with --long takes by far too long, compared to other files.

This one is totally weird. When running make testlong on Sage 5.9.beta2, this test initially timed out, so I afterwards doctested it twice with sage -t --long .... In both cases, it took about 25 minutes (+/-, I don't recall exactly).

I then built Sage 5.9.beta2 with FLINT 2.3, and there it took less than half a minute (same machine, same compiler flags), repeatedly, and so I reran the test in vanilla 5.9.beta2 again, and it now takes about the same time there as well.

Seems like the new doctesting framework is pretty flaky.

(Still, mutation_type.py's long doctests take quite a while in both installations, i.e., nearly 900 seconds.)

[stumpc5]

comment:50

Replying to @nexttime:

Replying to @nexttime:
(Still, mutation_type.py's long doctests take quite a while in both installations, i.e., nearly 900 seconds.)

Thanks for your report - do you think we should provide a patch (in another ticket) to shorten the long doctests?