#13425 closed enhancement (fixed)
Compute mutation type of a ClusterSeed or ClusterQuiver
Reported by: | gmoose05 | Owned by: | sage-combinat |
---|---|---|---|
Priority: | major | Milestone: | sage-5.9 |
Component: | combinatorics | Keywords: | cluster algebra, quiver, days45 |
Cc: | Merged in: | sage-5.9.beta2 | |
Authors: | Gregg Musiker, Christian Stump | Reviewers: | Christian Stump, Gregg Musiker |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #13424 | Stopgaps: |
Description (last modified by )
This ticket implements mutation type checks for cluster seeds and quivers.
Apply to the sage library:
Attachments (7)
Change History (57)
comment:1 Changed 7 years ago by
- Dependencies set to 13424
comment:2 Changed 7 years ago by
- Dependencies changed from 13424 to #13424
comment:3 Changed 7 years ago by
comment:4 Changed 7 years ago by
comment:5 Changed 7 years ago by
- Status changed from new to needs_review
comment:6 Changed 7 years ago by
- Keywords days45 added
comment:7 Changed 6 years ago by
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 6 years ago by
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
comment:9 Changed 6 years ago by
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
comment:10 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
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 6 years ago by
comment:13 Changed 6 years ago by
- Description modified (diff)
- Reviewers set to Christian Stump, Gregg Musikier
comment:14 Changed 6 years ago by
- Description modified (diff)
Changed 6 years ago by
comment:15 Changed 6 years ago by
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!
comment:16 Changed 6 years ago by
- Status changed from needs_review to positive_review
comment:17 Changed 6 years ago by
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.
comment:18 follow-up: ↓ 19 Changed 6 years ago by
- Status changed from positive_review to needs_work
comment:19 in reply to: ↑ 18 Changed 6 years ago by
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.
Changed 6 years ago by
Changed 6 years ago by
comment:20 Changed 6 years ago by
Hi Christian,
Small one small typo in the cluster seed documentation that is fixed by this patch, but no warnings or other errors.
If the documentation builds correctly for you, I think it is ready to go.
Gregg
comment:21 Changed 6 years ago by
- Status changed from needs_work to positive_review
If the documentation builds correctly for you, I think it is ready to go.
It does, so I press the positive review button again.
comment:22 follow-up: ↓ 23 Changed 6 years ago by
- Status changed from positive_review to needs_work
Doctests should write to temporary files, not to files within SAGE_LOCAL
. Probably, the function save_quiver_data()
should take a filename
parameter which should be tmp_filename()
when used in doctests.
Also: please dont do this, as it's a race condition:
if not os.path.exists(types_path): os.makedirs(types_path)
There is a function sage_makedirs()
which does this in a safe way.
comment:23 in reply to: ↑ 22 ; follow-up: ↓ 24 Changed 6 years ago by
Replying to jdemeyer:
Doctests should write to temporary files, not to files within
SAGE_LOCAL
. Probably, the functionsave_quiver_data()
should take afilename
parameter which should betmp_filename()
when used in doctests.
Thanks for your comments!
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.
Do you anyway think, it would be better to have an optional argument to specify the filename?
Thanks, Christian
comment:24 in reply to: ↑ 23 ; follow-up: ↓ 25 Changed 6 years ago by
Replying to stumpc5:
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.
OK, store it somewhere in DOT_SAGE
then.
comment:25 in reply to: ↑ 24 Changed 6 years ago by
- Status changed from needs_work to needs_review
Replying to jdemeyer:
There is a function
sage_makedirs()
which does this in a safe way.
Fixed!
Replying to jdemeyer:
OK, store it somewhere in
DOT_SAGE
then.
Fixed!
@Gregg: please have a close look at my change to recheck that I didn't introduce any bugs. We now
- store the data in
DOT_SAGE
(which is usually~/.sage
) in the function_save_data_dig6
, - check for the data in
load_data
first in the user folder for user saves and then in the folder coming from the package (if installed).
Cheers, Christian
comment:26 follow-up: ↓ 27 Changed 6 years ago by
Hi Christian,
It seems to work for me except for two small issues.
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.
2) Let's say that a user runs save_quiver_data(2, up_to=False,'types'=Classical). This saves data to the ./sage folder.
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].
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.
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.
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.
Best, Gregg
Changed 6 years ago by
comment:27 in reply to: ↑ 26 Changed 6 years ago by
Replying to gmoose05:
Hi Gregg,
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.
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).
Please recheck that this is the issue you were refering to, and you could maybe also provide a doctest for it.
2) Let's say that a user runs save_quiver_data(2, up_to=False,'types'=Classical). This saves data to the ./sage folder.
Fixed as well, we now update the data_dict from both sources. Also: please recheck and provide a doctest, if possible.
Cheers, Christian
comment:28 Changed 6 years ago by
Hi Christian,
Thanks, that fixed the two issues that I had.
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.
Cheers, Gregg
comment:29 Changed 6 years ago by
- Status changed from needs_review to positive_review
comment:30 follow-up: ↓ 31 Changed 6 years ago by
- Status changed from positive_review to needs_work
On the buildbot (snapperkob but several other systems too), I see
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) **********************************************************************
comment:31 in reply to: ↑ 30 ; follow-ups: ↓ 32 ↓ 33 Changed 6 years ago by
- Status changed from needs_work to needs_info
Replying to jdemeyer:
> Expected: > True ('A', 2) > True ('A', (1, 1), 1) > Got: > True ('A', (1, 1), 1) > True ('A', 2)
We also had this problem on our machines. It is caused by the (expected, weird, buggy ???) behaviour that some machines give
sage: cmp(2,(1,1)) 1
and others do
sage: cmp((1,1),2) -1
What should we thus do to get the output in some generic order?
comment:32 in reply to: ↑ 31 ; follow-up: ↓ 34 Changed 6 years ago by
Replying to stumpc5:
What should we thus do to get the output in some generic order?
I have no idea, ask sage-devel.
comment:33 in reply to: ↑ 31 Changed 6 years ago by
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...
comment:34 in reply to: ↑ 32 ; follow-up: ↓ 35 Changed 6 years ago by
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,
comment:35 in reply to: ↑ 34 ; follow-up: ↓ 36 Changed 6 years ago by
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...
comment:36 in reply to: ↑ 35 ; follow-up: ↓ 38 Changed 6 years ago by
- Status changed from needs_info to needs_review
comment:37 Changed 6 years ago by
- Reviewers changed from Christian Stump, Gregg Musikier to Christian Stump, Gregg Musiker
Changed 6 years ago by
comment:38 in reply to: ↑ 36 Changed 6 years ago by
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.
comment:39 Changed 6 years ago by
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
Changed 6 years ago by
comment:40 Changed 6 years ago by
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
comment:41 Changed 6 years ago by
- Status changed from needs_review to positive_review
comment:42 follow-up: ↓ 44 Changed 6 years ago by
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.
comment:43 Changed 6 years ago by
- Description modified (diff)
comment:44 in reply to: ↑ 42 Changed 6 years ago by
Replying to fbissey:
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
comment:45 Changed 6 years ago by
- Description modified (diff)
Thanks for the clarification. I have put it in the desired format.
comment:46 Changed 6 years ago by
- Merged in set to sage-5.9.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
comment:47 follow-up: ↓ 49 Changed 6 years ago by
Doctesting devel/sage/sage/combinat/cluster_algebra_quiver/quiver.py
with --long
takes by far too long, compared to other files.
comment:48 Changed 6 years ago by
Doctesting sage/combinat/cluster_algebra_quiver/mutation_type.py
with --long
also takes pretty long.
comment:49 in reply to: ↑ 47 ; follow-up: ↓ 50 Changed 6 years ago by
Replying to leif:
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.)
The tests become much too slow with this patch:
with only #13424 applied:
and with this patch applied as well:
We should avoid that.