Sage: Ticket #13425: Compute mutation type of a ClusterSeed or ClusterQuiver
https://trac.sagemath.org/ticket/13425
<p>
This ticket implements mutation type checks for cluster seeds and quivers.
</p>
<p>
Apply to the sage library:
</p>
<ul><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/13425/trac_13425-mutation_type-gm.patch" title="Attachment 'trac_13425-mutation_type-gm.patch' in Ticket #13425">trac_13425-mutation_type-gm.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/13425/trac_13425-mutation_type-gm.patch" title="Download"></a>
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/13425/trac_13425-review-cs.patch" title="Attachment 'trac_13425-review-cs.patch' in Ticket #13425">trac_13425-review-cs.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/13425/trac_13425-review-cs.patch" title="Download"></a>
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/13425/trac_13425-review_two-cs.patch" title="Attachment 'trac_13425-review_two-cs.patch' in Ticket #13425">trac_13425-review_two-cs.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/13425/trac_13425-review_two-cs.patch" title="Download"></a>
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/13425/trac_13425-small-doc-fix.patch" title="Attachment 'trac_13425-small-doc-fix.patch' in Ticket #13425">trac_13425-small-doc-fix.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/13425/trac_13425-small-doc-fix.patch" title="Download"></a>
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/13425/trac_13425-review_four-cs.patch" title="Attachment 'trac_13425-review_four-cs.patch' in Ticket #13425">trac_13425-review_four-cs.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/13425/trac_13425-review_four-cs.patch" title="Download"></a>
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/13425/trac_13425-review_five-cs.patch" title="Attachment 'trac_13425-review_five-cs.patch' in Ticket #13425">trac_13425-review_five-cs.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/13425/trac_13425-review_five-cs.patch" title="Download"></a>
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/13425/trac_13425-sixth-review-gm.patch" title="Attachment 'trac_13425-sixth-review-gm.patch' in Ticket #13425">trac_13425-sixth-review-gm.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/13425/trac_13425-sixth-review-gm.patch" title="Download"></a>
</li></ul>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/13425
Trac 1.1.6gmoose05Mon, 03 Sep 2012 04:01:11 GMTdependencies set
https://trac.sagemath.org/ticket/13425#comment:1
https://trac.sagemath.org/ticket/13425#comment:1
<ul>
<li><strong>dependencies</strong>
set to <em>13424</em>
</li>
</ul>
Ticketgmoose05Mon, 03 Sep 2012 04:02:23 GMTdependencies changed
https://trac.sagemath.org/ticket/13425#comment:2
https://trac.sagemath.org/ticket/13425#comment:2
<ul>
<li><strong>dependencies</strong>
changed from <em>13424</em> to <em>#13424</em>
</li>
</ul>
Ticketgmoose05Mon, 03 Sep 2012 04:04:01 GMTauthor changed
https://trac.sagemath.org/ticket/13425#comment:3
https://trac.sagemath.org/ticket/13425#comment:3
<ul>
<li><strong>author</strong>
changed from <em>Gregg Musiker and Christian Stump</em> to <em>Gregg Musiker, Christian Stump</em>
</li>
</ul>
Ticketstumpc5Fri, 07 Sep 2012 15:55:15 GMT
https://trac.sagemath.org/ticket/13425#comment:4
https://trac.sagemath.org/ticket/13425#comment:4
<p>
The tests become much too slow with this patch:
</p>
<p>
with only <a class="closed ticket" href="https://trac.sagemath.org/ticket/13424" title="enhancement: Compute Mutation Class for Cluster Algebra Seed or Cluster Quiver (closed: fixed)">#13424</a> applied:
</p>
<pre class="wiki">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]
</pre><p>
and with this patch applied as well:
</p>
<pre class="wiki">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]
</pre><p>
We should avoid that.
</p>
Ticketstumpc5Thu, 14 Feb 2013 16:54:44 GMTstatus changed
https://trac.sagemath.org/ticket/13425#comment:5
https://trac.sagemath.org/ticket/13425#comment:5
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
Ticketetn40ffThu, 14 Feb 2013 22:42:10 GMTkeywords changed
https://trac.sagemath.org/ticket/13425#comment:6
https://trac.sagemath.org/ticket/13425#comment:6
<ul>
<li><strong>keywords</strong>
<em>days45</em> added
</li>
</ul>
Ticketstumpc5Wed, 20 Feb 2013 08:00:49 GMT
https://trac.sagemath.org/ticket/13425#comment:7
https://trac.sagemath.org/ticket/13425#comment:7
<p>
Hi Gregg!
</p>
<p>
I know you are again working on this patch: it needs to be (almost trivially) rebased agains the newest version of <a class="closed ticket" href="https://trac.sagemath.org/ticket/13424" title="enhancement: Compute Mutation Class for Cluster Algebra Seed or Cluster Quiver (closed: fixed)">#13424</a>.
</p>
Ticketgmoose05Thu, 28 Feb 2013 05:04:58 GMT
https://trac.sagemath.org/ticket/13425#comment:8
https://trac.sagemath.org/ticket/13425#comment:8
<p>
Just uploaded a review patch (although I'm still working through it and this is just partial work).
</p>
<p>
The bulk of the changes are from mutation_type.py:
</p>
<p>
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.
</p>
<p>
I also changed auxiliary command names so that they begin with an underscore.
</p>
<p>
Christian, there were a few places where I had questions for you,
these are indicated by the word "CHECK" in caps.
</p>
<p>
More to come later as I look through the other .py files in this patch also.
</p>
<p>
Gregg
</p>
Ticketgmoose05Sun, 10 Mar 2013 04:31:18 GMT
https://trac.sagemath.org/ticket/13425#comment:9
https://trac.sagemath.org/ticket/13425#comment:9
<p>
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.
</p>
<p>
There are a few outstanding comments or questions marked with CHECK.
</p>
<p>
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.
</p>
<p>
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.
</p>
<p>
Gregg
</p>
Ticketstumpc5Sun, 10 Mar 2013 14:08:32 GMT
https://trac.sagemath.org/ticket/13425#comment:10
https://trac.sagemath.org/ticket/13425#comment:10
<p>
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.
</p>
<p>
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).
</p>
<p>
Cheers, Christian
</p>
Ticketstumpc5Fri, 15 Mar 2013 17:37:23 GMT
https://trac.sagemath.org/ticket/13425#comment:11
https://trac.sagemath.org/ticket/13425#comment:11
<p>
Hi Gregg, I uploaded a new patch.
</p>
<p>
The main changes are:
</p>
<ul><li>the mutation type check is now done in the order:
<ul><li>check if it is mutation infinite
</li><li>look in existing files
</li><li>use the big algorithm to determine the type
</li><li>generate the files and look in these newly generated files
</li></ul></li></ul><blockquote>
<p>
(I just realize that this results in two times looking in the data if it already exists. I should change that).
</p>
</blockquote>
<ul><li>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.
</li></ul><ul><li>I rewrote the code to construct the classical and exceptional data. Please have a look and say what you think.
</li></ul><p>
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 <code>ClusterQuiver([['A',3],['B',2],['T',(4,4,4)]])</code>, you will get an error since the third part is "unknown infinite" but the program is trying to build a reducible type from it.
</p>
<p>
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...
</p>
<p>
Cheers, Christian
</p>
Ticketstumpc5Fri, 15 Mar 2013 17:39:14 GMT
https://trac.sagemath.org/ticket/13425#comment:12
https://trac.sagemath.org/ticket/13425#comment:12
<p>
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.
</p>
Ticketstumpc5Tue, 19 Mar 2013 09:31:38 GMTattachment set
https://trac.sagemath.org/ticket/13425
https://trac.sagemath.org/ticket/13425
<ul>
<li><strong>attachment</strong>
set to <em>trac_13425-mutation_type-gm.patch</em>
</li>
</ul>
Ticketstumpc5Tue, 19 Mar 2013 10:14:41 GMTdescription changed; reviewer set
https://trac.sagemath.org/ticket/13425#comment:13
https://trac.sagemath.org/ticket/13425#comment:13
<ul>
<li><strong>reviewer</strong>
set to <em>Christian Stump, Gregg Musikier</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/13425?action=diff&version=13">diff</a>)
</li>
</ul>
Ticketstumpc5Tue, 19 Mar 2013 18:07:38 GMTdescription changed
https://trac.sagemath.org/ticket/13425#comment:14
https://trac.sagemath.org/ticket/13425#comment:14
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13425?action=diff&version=14">diff</a>)
</li>
</ul>
Ticketstumpc5Tue, 19 Mar 2013 18:09:38 GMTattachment set
https://trac.sagemath.org/ticket/13425
https://trac.sagemath.org/ticket/13425
<ul>
<li><strong>attachment</strong>
set to <em>trac_13425-review-cs.patch</em>
</li>
</ul>
Ticketstumpc5Tue, 19 Mar 2013 18:53:24 GMT
https://trac.sagemath.org/ticket/13425#comment:15
https://trac.sagemath.org/ticket/13425#comment:15
<p>
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!
</p>
Ticketgmoose05Tue, 19 Mar 2013 20:16:37 GMTstatus changed
https://trac.sagemath.org/ticket/13425#comment:16
https://trac.sagemath.org/ticket/13425#comment:16
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
TicketjdemeyerWed, 20 Mar 2013 17:03:52 GMT
https://trac.sagemath.org/ticket/13425#comment:17
https://trac.sagemath.org/ticket/13425#comment:17
<p>
There are various problem with the documentation:
</p>
<pre class="wiki">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.
</pre><p>
Also, I wondering whether you are using <code>assert</code> correctly. <code>assert</code> should only be used to check for bugs, not for bad user input. In other words, if there is any way to produce an <code>AssertionError</code> by supplying bad input to a public function, that is by definition a bug. Use <code>ValueError</code> or <code>TypeError</code> (or other exceptions) for bad user input.
</p>
TicketjdemeyerWed, 20 Mar 2013 17:05:19 GMTstatus changed
https://trac.sagemath.org/ticket/13425#comment:18
https://trac.sagemath.org/ticket/13425#comment:18
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
Ticketstumpc5Wed, 20 Mar 2013 17:58:15 GMT
https://trac.sagemath.org/ticket/13425#comment:19
https://trac.sagemath.org/ticket/13425#comment:19
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:18" title="Comment 18">jdemeyer</a>:
</p>
<p>
Hi Gregg, I will upload a fix within the next minutes -- though I don't really find the second doc warning in the docstring.
</p>
Ticketstumpc5Wed, 20 Mar 2013 18:43:31 GMTattachment set
https://trac.sagemath.org/ticket/13425
https://trac.sagemath.org/ticket/13425
<ul>
<li><strong>attachment</strong>
set to <em>trac_13425-review_two-cs.patch</em>
</li>
</ul>
Ticketgmoose05Thu, 21 Mar 2013 14:36:00 GMTattachment set
https://trac.sagemath.org/ticket/13425
https://trac.sagemath.org/ticket/13425
<ul>
<li><strong>attachment</strong>
set to <em>trac_13425-small-doc-fix.patch</em>
</li>
</ul>
Ticketgmoose05Thu, 21 Mar 2013 14:37:13 GMT
https://trac.sagemath.org/ticket/13425#comment:20
https://trac.sagemath.org/ticket/13425#comment:20
<p>
Hi Christian,
</p>
<p>
Small one small typo in the cluster seed documentation that is fixed by this patch, but no warnings or other errors.
</p>
<p>
If the documentation builds correctly for you, I think it is ready to go.
</p>
<p>
Gregg
</p>
Ticketstumpc5Thu, 21 Mar 2013 20:58:35 GMTstatus changed
https://trac.sagemath.org/ticket/13425#comment:21
https://trac.sagemath.org/ticket/13425#comment:21
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
</ul>
<blockquote class="citation">
<p>
If the documentation builds correctly for you, I think it is ready to go.
</p>
</blockquote>
<p>
It does, so I press the positive review button again.
</p>
TicketjdemeyerFri, 22 Mar 2013 16:10:42 GMTstatus changed
https://trac.sagemath.org/ticket/13425#comment:22
https://trac.sagemath.org/ticket/13425#comment:22
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Doctests should write to temporary files, not to files within <code>SAGE_LOCAL</code>. Probably, the function <code>save_quiver_data()</code> should take a <code>filename</code> parameter which should be <code>tmp_filename()</code> when used in doctests.
</p>
<p>
Also: please dont do this, as it's a race condition:
</p>
<pre class="wiki"> if not os.path.exists(types_path):
os.makedirs(types_path)
</pre><p>
There is a function <code>sage_makedirs()</code> which does this in a safe way.
</p>
Ticketstumpc5Fri, 22 Mar 2013 16:22:45 GMT
https://trac.sagemath.org/ticket/13425#comment:23
https://trac.sagemath.org/ticket/13425#comment:23
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:22" title="Comment 22">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Doctests should write to temporary files, not to files within <code>SAGE_LOCAL</code>. Probably, the function <code>save_quiver_data()</code> should take a <code>filename</code> parameter which should be <code>tmp_filename()</code> when used in doctests.
</p>
</blockquote>
<p>
Thanks for your comments!
</p>
<p>
In the current design of the code, there is no use of storing the "quiver data" into a file of your choice, but rather into a particular data file! Information in this file will then later be used in other methods which need the same data over and over again. I do not see any use of this function in a way the user can decide the file to store the data.
</p>
<p>
Do you anyway think, it would be better to have an optional argument to specify the filename?
</p>
<p>
Thanks, Christian
</p>
TicketjdemeyerFri, 22 Mar 2013 16:37:26 GMT
https://trac.sagemath.org/ticket/13425#comment:24
https://trac.sagemath.org/ticket/13425#comment:24
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:23" title="Comment 23">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
In the current design of the code, there is no use of storing the "quiver data" into a file of your choice, but rather into a particular data file! Information in this file will then later be used in other methods which need the same data over and over again. I do not see any use of this function in a way the user can decide the file to store the data.
</p>
</blockquote>
<p>
OK, store it somewhere in <code>DOT_SAGE</code> then.
</p>
Ticketstumpc5Fri, 22 Mar 2013 17:15:19 GMTstatus changed
https://trac.sagemath.org/ticket/13425#comment:25
https://trac.sagemath.org/ticket/13425#comment:25
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:22" title="Comment 22">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
There is a function <code>sage_makedirs()</code> which does this in a safe way.
</p>
</blockquote>
<p>
Fixed!
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:24" title="Comment 24">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
OK, store it somewhere in <code>DOT_SAGE</code> then.
</p>
</blockquote>
<p>
Fixed!
</p>
<p>
@Gregg: please have a close look at my change to recheck that I didn't introduce any bugs. We now
</p>
<ul><li>store the data in <code>DOT_SAGE</code> (which is usually <code>~/.sage</code>) in the function <code>_save_data_dig6</code>,
</li><li>check for the data in <code>load_data</code> first in the user folder for user saves and then in the folder coming from the package (if installed).
</li></ul><p>
Cheers, Christian
</p>
Ticketgmoose05Sat, 23 Mar 2013 14:30:51 GMT
https://trac.sagemath.org/ticket/13425#comment:26
https://trac.sagemath.org/ticket/13425#comment:26
<p>
Hi Christian,
</p>
<p>
It seems to work for me except for two small issues.
</p>
<p>
1) It seems that if I run save_quiver_data( ... ), and then load_data( ... ), I don't see the data I just saved. If I close and restart sage, this seems okay, or maybe there's just a time-lag but let me know if you see similar behavior.
</p>
<p>
2) Let's say that a user runs save_quiver_data(2, up_to=False,'types'=Classical).
This saves data to the ./sage folder.
</p>
<p>
If the user now runs the load_data(2) command, it looks in the ./sage folder and sees and prints data for ['A', (1,1),1], ['A', 2], ['B', 2], and ['BC', 1, 1].
</p>
<p>
If I then close sage and run ../../sage -i cluster_seed-1.0.spkg (technically with the -f option since I'm reinstalling after having deleted the folder for testing purposes), then, I should also have the exceptional data now too, but when I run load_data(2), it still only loads the classical data from the ./sage folder.
</p>
<p>
Do you get similar behavior? I think it would be slightly better if load_data( n ) loaded the union of the dig6 data from the two sources. Otherwise, if one saved the classical data (for e.g. for rank 9 or 10) then unless one also saved classicalexceptional, one would no longer have the exceptional data.
</p>
<p>
However, this is still better than the version before where save_quiver_data and the spkg were writing to the same filenames so the same problem would have been appearing there.
</p>
<p>
Best, Gregg
</p>
<p>
</p>
Ticketstumpc5Sat, 23 Mar 2013 18:16:06 GMTattachment set
https://trac.sagemath.org/ticket/13425
https://trac.sagemath.org/ticket/13425
<ul>
<li><strong>attachment</strong>
set to <em>trac_13425-review_four-cs.patch</em>
</li>
</ul>
Ticketstumpc5Sat, 23 Mar 2013 18:19:25 GMT
https://trac.sagemath.org/ticket/13425#comment:27
https://trac.sagemath.org/ticket/13425#comment:27
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:26" title="Comment 26">gmoose05</a>:
</p>
<p>
Hi Gregg,
</p>
<blockquote class="citation">
<p>
1) It seems that if I run save_quiver_data( ... ), and then load_data( ... ), I don't see the data I just saved. If I close and restart sage, this seems okay, or maybe there's just a time-lag but let me know if you see similar behavior.
</p>
</blockquote>
<p>
I think this was a cached_function thing, so I now delete this cache when saving the data (see the last lines of the updated function).
</p>
<p>
Please recheck that this is the issue you were refering to, and you could maybe also provide a doctest for it.
</p>
<blockquote class="citation">
<p>
2) Let's say that a user runs save_quiver_data(2, up_to=False,'types'=Classical).
This saves data to the ./sage folder.
</p>
</blockquote>
<p>
Fixed as well, we now update the data_dict from both sources. Also: please recheck and provide a doctest, if possible.
</p>
<p>
Cheers, Christian
</p>
Ticketgmoose05Sat, 23 Mar 2013 20:48:23 GMT
https://trac.sagemath.org/ticket/13425#comment:28
https://trac.sagemath.org/ticket/13425#comment:28
<p>
Hi Christian,
</p>
<p>
Thanks, that fixed the two issues that I had.
</p>
<blockquote>
<p>
I think the behavior is relatively self-explanatory and I am worried that extra dooctests with save and load commands where the spkg may or may not be installed would be more difficult to write than they are worth. So I am happy to go ahead and give a positive review at this point.
</p>
</blockquote>
<p>
Cheers, Gregg
</p>
Ticketgmoose05Sat, 23 Mar 2013 20:48:55 GMTstatus changed
https://trac.sagemath.org/ticket/13425#comment:29
https://trac.sagemath.org/ticket/13425#comment:29
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
TicketjdemeyerMon, 25 Mar 2013 06:05:44 GMTstatus changed
https://trac.sagemath.org/ticket/13425#comment:30
https://trac.sagemath.org/ticket/13425#comment:30
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
On the buildbot (<a class="ext-link" href="http://build.sagemath.org/sage/builders/AIMS%20snapperkob%20%28Ubuntu%2012.04%20x86_64%29/builds/118/steps/shell_8/logs/stdio"><span class="icon"></span>snapperkob</a> but several other systems too), I see
</p>
<pre class="wiki">sage -t --long devel/sage/sage/combinat/cluster_algebra_quiver/mutation_type.py
**********************************************************************
File "devel/sage/sage/combinat/cluster_algebra_quiver/mutation_type.py", line 1259, in sage.combinat.cluster_algebra_quiver.mutation_type._mutation_type_test
Failed example:
_mutation_type_test(2) # long time
Expected:
True ('A', 2)
True ('A', (1, 1), 1)
True ('B', 2)
True ('BC', 1, 1)
True ('G', 2)
Got:
True ('A', (1, 1), 1)
True ('A', 2)
True ('B', 2)
True ('BC', 1, 1)
True ('G', 2)
**********************************************************************
File "devel/sage/sage/combinat/cluster_algebra_quiver/mutation_type.py", line 1266, in sage.combinat.cluster_algebra_quiver.mutation_type._mutation_type_test
Failed example:
_mutation_type_test(3) # long time
Expected:
True ('A', 3)
True ('A', (2, 1), 1)
True ('B', 3)
True ('BB', 2, 1)
True ('BC', 2, 1)
True ('C', 3)
True ('CC', 2, 1)
True ('G', 2, -1)
True ('G', 2, 1)
Got:
True ('A', (2, 1), 1)
True ('A', 3)
True ('B', 3)
True ('BB', 2, 1)
True ('BC', 2, 1)
True ('C', 3)
True ('CC', 2, 1)
True ('G', 2, -1)
True ('G', 2, 1)
**********************************************************************
File "devel/sage/sage/combinat/cluster_algebra_quiver/mutation_type.py", line 1277, in sage.combinat.cluster_algebra_quiver.mutation_type._mutation_type_test
Failed example:
_mutation_type_test(4) # long time
Expected:
True ('A', 4)
True ('A', (2, 2), 1)
True ('A', (3, 1), 1)
True ('B', 4)
True ('BB', 3, 1)
True ('BC', 3, 1)
True ('BD', 3, 1)
True ('C', 4)
True ('CC', 3, 1)
True ('CD', 3, 1)
True ('D', 4)
True ('F', 4)
True ('G', 2, (1, 1))
True ('G', 2, (3, 3))
True ('G', 2, (1, 3))
Got:
True ('A', (2, 2), 1)
True ('A', (3, 1), 1)
True ('A', 4)
True ('B', 4)
True ('BB', 3, 1)
True ('BC', 3, 1)
True ('BD', 3, 1)
True ('C', 4)
True ('CC', 3, 1)
True ('CD', 3, 1)
True ('D', 4)
True ('F', 4)
True ('G', 2, (1, 1))
True ('G', 2, (3, 3))
True ('G', 2, (1, 3))
**********************************************************************
File "devel/sage/sage/combinat/cluster_algebra_quiver/mutation_type.py", line 1294, in sage.combinat.cluster_algebra_quiver.mutation_type._mutation_type_test
Failed example:
_mutation_type_test(5) # long time
Expected:
True ('A', 5)
True ('A', (3, 2), 1)
True ('A', (4, 1), 1)
True ('B', 5)
True ('BB', 4, 1)
True ('BC', 4, 1)
True ('BD', 4, 1)
True ('C', 5)
True ('CC', 4, 1)
True ('CD', 4, 1)
False ('D', 4, 1)
True ('D', 5)
True ('F', 4, 1)
True ('F', 4, -1)
Got:
True ('A', (3, 2), 1)
True ('A', (4, 1), 1)
True ('A', 5)
True ('B', 5)
True ('BB', 4, 1)
True ('BC', 4, 1)
True ('BD', 4, 1)
True ('C', 5)
True ('CC', 4, 1)
True ('CD', 4, 1)
False ('D', 4, 1)
True ('D', 5)
True ('F', 4, 1)
True ('F', 4, -1)
**********************************************************************
File "devel/sage/sage/combinat/cluster_algebra_quiver/mutation_type.py", line 1422, in sage.combinat.cluster_algebra_quiver.mutation_type._random_multi_tests
Failed example:
_random_multi_tests(2,100) # long time
Expected:
testing ('A', 2)
testing ('A', (1, 1), 1)
testing ('B', 2)
testing ('BC', 1, 1)
Got:
testing ('A', (1, 1), 1)
testing ('A', 2)
testing ('B', 2)
testing ('BC', 1, 1)
**********************************************************************
File "devel/sage/sage/combinat/cluster_algebra_quiver/mutation_type.py", line 1428, in sage.combinat.cluster_algebra_quiver.mutation_type._random_multi_tests
Failed example:
_random_multi_tests(3,100) # long time
Expected:
testing ('A', 3)
testing ('A', (2, 1), 1)
testing ('B', 3)
testing ('BB', 2, 1)
testing ('BC', 2, 1)
testing ('C', 3)
testing ('CC', 2, 1)
Got:
testing ('A', (2, 1), 1)
testing ('A', 3)
testing ('B', 3)
testing ('BB', 2, 1)
testing ('BC', 2, 1)
testing ('C', 3)
testing ('CC', 2, 1)
**********************************************************************
File "devel/sage/sage/combinat/cluster_algebra_quiver/mutation_type.py", line 1437, in sage.combinat.cluster_algebra_quiver.mutation_type._random_multi_tests
Failed example:
_random_multi_tests(4,100) # long time
Expected:
testing ('A', 4)
testing ('A', (2, 2), 1)
testing ('A', (3, 1), 1)
testing ('B', 4)
testing ('BB', 3, 1)
testing ('BC', 3, 1)
testing ('BD', 3, 1)
testing ('C', 4)
testing ('CC', 3, 1)
testing ('CD', 3, 1)
testing ('D', 4)
Got:
testing ('A', (2, 2), 1)
testing ('A', (3, 1), 1)
testing ('A', 4)
testing ('B', 4)
testing ('BB', 3, 1)
testing ('BC', 3, 1)
testing ('BD', 3, 1)
testing ('C', 4)
testing ('CC', 3, 1)
testing ('CD', 3, 1)
testing ('D', 4)
**********************************************************************
</pre>
Ticketstumpc5Mon, 25 Mar 2013 06:15:51 GMTstatus changed
https://trac.sagemath.org/ticket/13425#comment:31
https://trac.sagemath.org/ticket/13425#comment:31
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_info</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:30" title="Comment 30">jdemeyer</a>:
</p>
<pre class="wiki">> Expected:
> True ('A', 2)
> True ('A', (1, 1), 1)
> Got:
> True ('A', (1, 1), 1)
> True ('A', 2)
</pre><p>
We also had this problem on our machines. It is caused by the (expected, weird, buggy ???) behaviour that some machines give
</p>
<pre class="wiki">sage: cmp(2,(1,1))
1
</pre><p>
and others do
</p>
<pre class="wiki">sage: cmp(2,(1,1))
-1
</pre><p>
What should we thus do to get the output in some generic order?
</p>
TicketjdemeyerMon, 25 Mar 2013 06:24:39 GMT
https://trac.sagemath.org/ticket/13425#comment:32
https://trac.sagemath.org/ticket/13425#comment:32
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:31" title="Comment 31">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
What should we thus do to get the output in some generic order?
</p>
</blockquote>
<p>
I have no idea, ask sage-devel.
</p>
TicketjdemeyerMon, 25 Mar 2013 06:25:59 GMT
https://trac.sagemath.org/ticket/13425#comment:33
https://trac.sagemath.org/ticket/13425#comment:33
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:31" title="Comment 31">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
We also had this problem on our machines.
</p>
</blockquote>
<p>
If you knew this problem existed, this ticket should never have gotten positive review...
</p>
TicketnthieryMon, 25 Mar 2013 06:33:26 GMT
https://trac.sagemath.org/ticket/13425#comment:34
https://trac.sagemath.org/ticket/13425#comment:34
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:32" title="Comment 32">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:31" title="Comment 31">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
What should we thus do to get the output in some generic order?
</p>
</blockquote>
<p>
I have no idea, ask sage-devel.
</p>
</blockquote>
<p>
If you can afford a sort somewhere, a typical trick is to do:
</p>
<blockquote>
<p>
sage: sorted(..., key=str)
</p>
</blockquote>
<p>
And then it will sort according to the string representation which is platform independent.
</p>
<p>
I usually only use that for sorting the results in the doctests. But in your situation that could be ok too.
</p>
<p>
Cheers,
</p>
Ticketstumpc5Mon, 25 Mar 2013 06:43:10 GMT
https://trac.sagemath.org/ticket/13425#comment:35
https://trac.sagemath.org/ticket/13425#comment:35
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:34" title="Comment 34">nthiery</a>:
</p>
<blockquote class="citation">
<p>
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.
</p>
</blockquote>
<p>
Good idea, thanks -- the problem is not at all in a time critical method, so no problem to do your trick!
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:32" title="Comment 32">jdemeyer</a>:
If you knew this problem existed, this ticket should never have gotten positive review...
</p>
</blockquote>
<p>
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...
</p>
Ticketstumpc5Mon, 25 Mar 2013 07:50:26 GMTstatus changed
https://trac.sagemath.org/ticket/13425#comment:36
https://trac.sagemath.org/ticket/13425#comment:36
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:35" title="Comment 35">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:34" title="Comment 34">nthiery</a>:
Good idea, thanks -- the problem is not at all in a time critical method, so no problem to do your trick!
</p>
</blockquote>
<p>
Done! Nicolas, would you look at this last change and give it a positive review? Cheers!
</p>
Ticketstumpc5Mon, 25 Mar 2013 07:50:45 GMTreviewer changed
https://trac.sagemath.org/ticket/13425#comment:37
https://trac.sagemath.org/ticket/13425#comment:37
<ul>
<li><strong>reviewer</strong>
changed from <em>Christian Stump, Gregg Musikier</em> to <em>Christian Stump, Gregg Musiker</em>
</li>
</ul>
Ticketstumpc5Mon, 25 Mar 2013 14:45:54 GMTattachment set
https://trac.sagemath.org/ticket/13425
https://trac.sagemath.org/ticket/13425
<ul>
<li><strong>attachment</strong>
set to <em>trac_13425-review_five-cs.patch</em>
</li>
</ul>
TicketnthieryMon, 25 Mar 2013 15:10:07 GMT
https://trac.sagemath.org/ticket/13425#comment:38
https://trac.sagemath.org/ticket/13425#comment:38
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:36" title="Comment 36">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:35" title="Comment 35">stumpc5</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:34" title="Comment 34">nthiery</a>:
Good idea, thanks -- the problem is not at all in a time critical method, so no problem to do your trick!
</p>
</blockquote>
<p>
Done! Nicolas, would you look at this last change and give it a positive review? Cheers!
</p>
</blockquote>
<p>
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.
</p>
Ticketgmoose05Tue, 26 Mar 2013 03:58:42 GMT
https://trac.sagemath.org/ticket/13425#comment:39
https://trac.sagemath.org/ticket/13425#comment:39
<p>
The key=str fix in quiver_mutation_type.py seems to have already solved the issues in save_quiver_data(), etc.
</p>
<p>
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.
</p>
<p>
Reordering the examples accordingly slightly, this now passes on my machine (the one that was having trouble before). All tests passed!
</p>
<p>
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.
</p>
<p>
Is there any way to get the patchbot to run again???
</p>
<p>
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.
</p>
<p>
Gregg
</p>
Ticketgmoose05Tue, 26 Mar 2013 03:59:53 GMTattachment set
https://trac.sagemath.org/ticket/13425
https://trac.sagemath.org/ticket/13425
<ul>
<li><strong>attachment</strong>
set to <em>trac_13425-sixth-review-gm.patch</em>
</li>
</ul>
Ticketgmoose05Tue, 26 Mar 2013 13:00:46 GMT
https://trac.sagemath.org/ticket/13425#comment:40
https://trac.sagemath.org/ticket/13425#comment:40
<p>
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).
</p>
<p>
So with this latest patch, it is now ready to go.
</p>
<p>
Gregg
</p>
Ticketstumpc5Tue, 26 Mar 2013 13:10:02 GMTstatus changed
https://trac.sagemath.org/ticket/13425#comment:41
https://trac.sagemath.org/ticket/13425#comment:41
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
TicketfbisseyThu, 28 Mar 2013 01:38:17 GMT
https://trac.sagemath.org/ticket/13425#comment:42
https://trac.sagemath.org/ticket/13425#comment:42
<p>
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.
</p>
Ticketgmoose05Thu, 28 Mar 2013 05:03:24 GMTdescription changed
https://trac.sagemath.org/ticket/13425#comment:43
https://trac.sagemath.org/ticket/13425#comment:43
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13425?action=diff&version=43">diff</a>)
</li>
</ul>
Ticketgmoose05Thu, 28 Mar 2013 05:04:22 GMT
https://trac.sagemath.org/ticket/13425#comment:44
https://trac.sagemath.org/ticket/13425#comment:44
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:42" title="Comment 42">fbissey</a>:
</p>
<blockquote class="citation">
<p>
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.
</p>
</blockquote>
<p>
Just modified the description accordingly. All seven patches should be applied in order.
</p>
<p>
Gregg
</p>
TicketfbisseyThu, 28 Mar 2013 09:17:34 GMTdescription changed
https://trac.sagemath.org/ticket/13425#comment:45
https://trac.sagemath.org/ticket/13425#comment:45
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/13425?action=diff&version=45">diff</a>)
</li>
</ul>
<p>
Thanks for the clarification. I have put it in the desired format.
</p>
TicketjdemeyerThu, 28 Mar 2013 17:58:12 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/13425#comment:46
https://trac.sagemath.org/ticket/13425#comment:46
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-5.9.beta2</em>
</li>
</ul>
TicketleifMon, 01 Apr 2013 15:18:57 GMT
https://trac.sagemath.org/ticket/13425#comment:47
https://trac.sagemath.org/ticket/13425#comment:47
<p>
Doctesting <code>devel/sage/sage/combinat/cluster_algebra_quiver/quiver.py</code> with <code>--long</code> takes by far too long, compared to other files.
</p>
TicketleifMon, 01 Apr 2013 15:34:46 GMT
https://trac.sagemath.org/ticket/13425#comment:48
https://trac.sagemath.org/ticket/13425#comment:48
<p>
Doctesting <code>sage/combinat/cluster_algebra_quiver/mutation_type.py</code> with <code>--long</code> also takes pretty long.
</p>
TicketleifTue, 02 Apr 2013 05:08:43 GMT
https://trac.sagemath.org/ticket/13425#comment:49
https://trac.sagemath.org/ticket/13425#comment:49
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:47" title="Comment 47">leif</a>:
</p>
<blockquote class="citation">
<p>
Doctesting <code>devel/sage/sage/combinat/cluster_algebra_quiver/quiver.py</code> with <code>--long</code> takes by far too long, compared to other files.
</p>
</blockquote>
<p>
This one is totally weird. When running <code>make testlong</code> on Sage 5.9.beta2, this test initially timed out, so I afterwards doctested it twice with <code>sage -t --long ...</code>. In both cases, it took about 25 minutes (+/-, I don't recall exactly).
</p>
<p>
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.
</p>
<p>
Seems like the new doctesting framework is pretty flaky.
</p>
<p>
(Still, <code>mutation_type.py</code>'s long doctests take quite a while in both installations, i.e., nearly 900 seconds.)
</p>
Ticketstumpc5Tue, 02 Apr 2013 08:26:39 GMT
https://trac.sagemath.org/ticket/13425#comment:50
https://trac.sagemath.org/ticket/13425#comment:50
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:49" title="Comment 49">leif</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/13425#comment:47" title="Comment 47">leif</a>:
(Still, <code>mutation_type.py</code>'s long doctests take quite a while in both installations, i.e., nearly 900 seconds.)
</p>
</blockquote>
<p>
Thanks for your report - do you think we should provide a patch (in another ticket) to shorten the long doctests?
</p>
Ticket