-
-
Notifications
You must be signed in to change notification settings - Fork 481
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implementation of the class ClusterQuiver #10538
Comments
This comment has been minimized.
This comment has been minimized.
Dependencies: 10527 |
Changed dependencies from 10527 to #10527 |
comment:6
The failed doctest are related to the mulitple process doctesting. I get the same on my computer if I use multiple processes, but all tests pass with only a single process. I guess that has to do with reading the exceptional types from an external file, which cannot be accessed simultaneously from different processes. I add a sentence to the documentation... |
comment:7
Is this dependent on anything other than #10527? After applying that and this patch I get the following:
I think the patch was meant for 4.7.2, maybe it needs to be updated to work with the current version of sage? |
comment:8
Replying to @sagetrac-JStarx:
maybe -- I will upload a new patch as soon as someone is willing to review it. We have a notebook server running for all people that want to play with the combinatorial quivers and cluster algebras (which are quite a few!) -- you can log in and play with it at sage.lacim.uqam.ca. |
comment:9
Are the data files for the different mutation types really supposed to be part of the patch? It seems to me that it's a fairly large quantity of data, so maybe it would be better to set things up so that an interested user can reconstruct it (which wouldn't be too hard, right?). Or maybe I have the wrong idea about how receptive Sage is to including data like this. Are there guidelines for this anywhere? If not, maybe you could ask for feedback on sage-devel (or if you'd rather, I could)? |
comment:10
Replying to @hughrthomas:
Hi Hugh, I rearranged the patches, so that the functions
are now in #10538. I also added a function Best, Christian |
comment:11
I have added a review patch, but I haven't put anything in it yet. cheers, Hugh |
comment:12
I think that what I have now posted in the review patch for #10538 (on the combinat server) makes the patch compatible with the changes I have introduced in reviewing #10527. (It also pointed out some mistakes in my "corrections" to #10527...) In particular, all doctests are passing for me. If you find anything that is not working, please let me know; I will try to get it so everything is running properly for Sage Days 40. |
This comment has been minimized.
This comment has been minimized.
Reviewer: Gregg Musiker |
comment:16
With an eye towards startup time, can all these new modules be imported on demand? |
comment:17
Replying to @robertwb:
I am not very familiar with possible implications. Do you suggest that we should lazily import them? |
comment:18
This patch fixes a number of issues and also gives the user more flexibility. Please see details below (especially 8c)
I corrected the documentation to reflect this.
_construct_mutation_classes_by_rank, do not work yet because they require the .mutation_class() command for quivers which were moved into a later patch. I therefore deleted them from this patch. We should make sure to remember to include these back into the "mutation_class" patch
8a) In QuiverMutationType.py: Separated out the cases ['F', 4, [1,2] ] and ['F', 4, [2,1] ] in the class QuiverMutationType_Irreducible. I also added the coercions ['F',4,[2,1]] -> ['F',4,[1,2]] and Similarly, the values ['GR', [2,n]] and ['TR',1], ['TR',2] are now allowed as inputs to QuiverMuationtType_Irreducible 8b) During this process, I realized that even with my slight edit to the While we could make this one line edit to my old patch, it might be just as easy to green-light 10527 as is and then my recent 8c) SIGNIFICANT CHANGE: I have changed the behavior of ClusterQuiver(['C',2]), These are highlighted in the new doctests I added. The point is that this gives the savvy user an easier time to build common quivers, even though they are mutation-equivalent to the standard quiver of a quiver mutation type we already build. Try it out, I think you'll like how it now works. Note for Christian: I tried first using the method we recently discussed over IM, but in the end this seemed cleaner, and I didn't have to change the standard_quiver command in the end.
If one writes Q.mutate(0,0), Q.mutate(0,1), or Q.mutate(0,2), etc instead of |
This comment has been minimized.
This comment has been minimized.
comment:20
Replying to @stumpc5:
Yes, exactly (whether using the lazy_import module or another mechanism). |
comment:25
Christian, the changes we discussed have now been essentially made:
Note that I employed a strategy slightly different than we discussed: I use QuiverMutationType_Irreducible._digraph to get If you want to move some of these lines from QuiverMutationType_Irreducible to here, we can do that, but I wanted to make sure it worked first before trying to pull those lines over. Also, I'm perfectly happy leaving the lines in QuiverMutationType_Irreducible since it is not a globally defined class, but am open to further discussion.
with special cases for small n. However
yields the directed cycle quiver of type D_n rather than the standard Dynkin D_n.
I added a dummy example for .plot() in QuiverMutationType but didn't update the others. Hopefully adding this one function changed the coverage percentage so that we increase it rather than decrease it (I think that's what made the patchbot unhappy). We also have bad coverage in the mutation_class.py file, but state that at the top of the sheet, and I will worry about this another day. |
comment:26
I rebased the patches according to the new lazy import in #10527. |
comment:27
Recent changes to #10527 led to apply errors when my patch trac_10538-quiver-gm.patch is applied after the new #10527 patches. I just uploaded a new version of my patch to eliminate these errors. Christian, please delete my old (39.1 KB) patch and keep my (35.4 KB) patch in its place. I no longer had permissions to replace my old patch. For better or worse, the patchbot is down at the moment so we won't get any apply failures for the time being anyway. By the way, Dylan, this patch should be ready for reviewing shortly. Once you have a trac account, I can change the reviewer field accordingly. |
comment:28
Replying to @sagetrac-gmoose05:
Done. |
Changed reviewer from Gregg Musiker to Dylan Rupel |
comment:30
Just added an updated version of my patch. This includes all of the earlier changes and added a single method to the ClusterQuiver class, qmu_save(), that allows one to save a ClusterQuiver to a .qmu file so that it can uploaded directly into Bernhard Keller's Java Applet for Quiver Mutation. |
comment:31
I made some small changes to the init() method. In particular there was some undesirable behavior when 'data' was a list of edges or an empty digraph. I added examples and tests which illustrate the changes. When 'data' is a digraph it will now warn the user if loops are present rather than referring to the loops as two-cycles. I also added an 'inplace' option (not default) to the principal_extension() method. |
comment:32
I uploaded a new version of the patch with all missing doctests added. Let's see what the patchbot says... |
comment:33
Okay, I had another look and made some last minor changes in the documentation -- I think it is ready to go once we can convince the patchbot to give us a green light! |
comment:34
I did some reorganizing of the examples, is the placement of my colons correct? I made a small change in the mutate() method so that it remembers the QuiverMutationType when inplace=False. |
comment:35
Replying to @sagetrac-drupel:
You can figure out how the documetation looks like by using # sage -docbuild reference html and then look at the index.html specified at the end. I will do it myself but don't have time today to do it anymore...
good call, thanks! |
comment:36
Hi Dylan -- I folded your review into my patch. I did that because patchbot doesn't run patches by people it "doesn't know" (I don't have a proper definition here), and it is better if we see the green light before actually setting the positive review. From my side, you can go forward and do so, as soon as the patchbot is done without finding anything. Thanks for looking closely at the patch! Christian |
Changed author from Christian Stump to Christian Stump, Gregg Musiker |
comment:39
|
comment:40
Attachment: trac_10538-quiver-cs.patch.gz Replying to @jdemeyer: Fixed! |
Merged: sage-5.6.beta3 |
comment:43
Attachment: trac_10538_qmu_save.patch.gz There is a minor issue in qmu_save: it does not work whenever the quiver has 0<m!=n frozen vertices. |
comment:44
Replying to @Etn40ff:
Hi Salvatore, Thanks for finding the bug with qmu_save with frozen vertices. Since this ticket has already closed, I suggest opening up a new ticket which is simply the trac_10538_qmu_save.patch. Since it is small, I can review this quickly and then it should be able to be merged into the latest sage quickly. See Ticket #14638 for a similar example. Gregg |
This class implements quivers for skew-symmetrizable matrices.
The patch contains multiple examples.
Apply the two patches below, in order. The version posted here is the current version; the one on the combinat queue is slightly out of date.
Depends on #10527
CC: @sagetrac-drupel
Component: combinatorics
Keywords: cluster algebra, quiver
Author: Christian Stump, Gregg Musiker
Reviewer: Dylan Rupel
Merged: sage-5.6.beta3
Issue created by migration from https://trac.sagemath.org/ticket/10538
The text was updated successfully, but these errors were encountered: