#10538 closed enhancement (fixed)
Implementation of the class ClusterQuiver
Reported by: | stumpc5 | Owned by: | etn40ff |
---|---|---|---|
Priority: | major | Milestone: | sage-5.6 |
Component: | combinatorics | Keywords: | cluster algebra, quiver |
Cc: | d.rupel@… | Merged in: | sage-5.6.beta3 |
Authors: | Christian Stump, Gregg Musiker | Reviewers: | Dylan Rupel |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #10527 | Stopgaps: |
Description (last modified by )
This class implements quivers for skew-symmetrizable matrices.
The patch contains multiple examples.
sage: Q = ClusterQuiver(['A',3]); Q Quiver on 3 vertices of type ['A', 3]
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.
Attachments (2)
Change History (47)
comment:1 Changed 7 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 7 years ago by
- Dependencies set to 10527
comment:3 Changed 7 years ago by
- Dependencies changed from 10527 to #10527
comment:4 Changed 7 years ago by
- Owner changed from sage-combinat to (none)
comment:5 Changed 7 years ago by
- Milestone set to sage-4.7.2
comment:6 Changed 7 years ago by
comment:7 follow-up: ↓ 8 Changed 6 years ago by
- Status changed from needs_review to needs_info
Is this dependent on anything other than #10527? After applying that and this patch I get the following:
sage: S = ClusterSeed(['A',3]) ERROR: An unexpected error occurred while tokenizing input The following traceback may be corrupted or invalid The error message is: ('EOF in multi-line statement', (398, 0)) --------------------------------------------------------------------------- AttributeError Traceback (most recent call last) /Users/Starx/sage/devel/sage-main/<ipython console> in <module>() /Users/Starx/sage/local/lib/python2.6/site-packages/sage/combinat/cluster_algebra_quiver/cluster_seed.pyc in __init__(self, data, frozen) 93 elif type( data ) in [list,tuple] and ( type( data[0] ) is str or all(type( comp ) in [list,tuple] and type( comp[0] ) is str for comp in data) ): 94 mutation_type = QuiverMutationType( data ) ---> 95 quiver = mutation_type.standard_quiver() 96 self.__init__( quiver ) 97 /Users/Starx/sage/local/lib/python2.6/site-packages/sage/misc/cachefunc.pyc in __call__(self, *args, **kwds) 553 return self.cache[k] 554 except KeyError: --> 555 w = self._cachedmethod._instance_call(self._instance, *args, **kwds) 556 self.cache[k] = w 557 return w /Users/Starx/sage/local/lib/python2.6/site-packages/sage/misc/cachefunc.pyc in _instance_call(self, inst, *args, **kwds) 776 777 """ --> 778 return self._cachedfunc.f(inst, *args, **kwds) 779 780 def _get_instance_cache(self, inst): /Users/Starx/sage/local/lib/python2.6/site-packages/sage/combinat/cluster_algebra_quiver/quiver_mutation_type.pyc in standard_quiver(self) 1211 """ 1212 from quiver import Quiver -> 1213 Q = Quiver( self._digraph ) 1214 Q._mutation_type = self 1215 return Q /Users/Starx/sage/local/lib/python2.6/site-packages/sage/combinat/cluster_algebra_quiver/quiver.pyc in __init__(self, data, frozen) 161 162 M = _edge_list_to_matrix( dg.edge_iterator(), n, m ) --> 163 if not _principal_part(M).is_skew_symmetrizable( positive=True ): 164 raise ValueError, "The input digraph must be skew-symmetrizable" 165 self._digraph = dg /Users/Starx/sage/local/lib/python2.6/site-packages/sage/structure/element.so in sage.structure.element.Element.__getattr__ (sage/structure/element.c:2884)() /Users/Starx/sage/local/lib/python2.6/site-packages/sage/structure/parent.so in sage.structure.parent.getattr_from_other_class (sage/structure/parent.c:3236)() AttributeError: 'sage.matrix.matrix_integer_sparse.Matrix_integer_sparse' object has no attribute 'is_skew_symmetrizable'
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 in reply to: ↑ 7 Changed 6 years ago by
Replying to JStarx:
I think the patch was meant for 4.7.2, maybe it needs to be updated to work with the current version of sage?
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 follow-up: ↓ 10 Changed 6 years ago by
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 in reply to: ↑ 9 Changed 6 years ago by
Replying to hthomas:
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)?
Hi Hugh,
I rearranged the patches, so that the functions
_construct_mutation_classes_by_rank
_construct_exceptional_mutation_classes
_save_exceptional_data
are now in #10538. I also added a function save_exceptional_data
that saves the data to SAGE_ROOT/data/cluster_algebra/quiver
. This function is supposed to be imported by the user if needed. The data files are now not part of the patch anymore, but can only be created by the user using this function.
Best, Christian
comment:11 Changed 6 years ago by
I have added a review patch, but I haven't put anything in it yet.
cheers,
Hugh
comment:12 Changed 6 years ago by
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.
comment:13 Changed 6 years ago by
- Description modified (diff)
- Summary changed from Implementation of the classes ClusterSeed and Quiver to Implementation of the class Quiver
comment:14 Changed 6 years ago by
- Status changed from needs_info to needs_review
comment:15 Changed 6 years ago by
- Reviewers set to Gregg Musiker
comment:16 follow-up: ↓ 17 Changed 6 years ago by
With an eye towards startup time, can all these new modules be imported on demand?
comment:17 in reply to: ↑ 16 ; follow-up: ↓ 20 Changed 6 years ago by
Replying to robertwb:
With an eye towards startup time, can all these new modules be imported on demand?
I am not very familiar with possible implications. Do you suggest that we should lazily import them?
comment:18 follow-up: ↓ 22 Changed 6 years ago by
This patch fixes a number of issues and also gives the user more flexibility. Please see details below (especially 8c)
1) For the .plot() and .show() commands, the default by source code and experimentation is circular = False.
I corrected the documentation to reflect this.
2) standard_quiver was defined twice in a row, so I deleted one of the two copies.
3) The auxiliary commands
_construct_mutation_classes_by_rank, _construct_exceptional_mutation_classes, _save_exceptional_data, and save_exceptional_data
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 that comes after quivers, cluster_seeds, ... in our sequence."
4) The class 'Quiver' has been changed to 'ClusterQuiver?' (Associated examples and function calls also changed so that all tests pass) Future patches would have to be updated accordingly.
5) I edited the principal_extension command so that it adds n frozen variables (corresponding to an identity matrix in the bottom of the B-matrix) *even* if the quiver already contained frozen vertices. This is important for examples like a cluster algebra from a surface *with boundary*.
6) Renamed "principal_restriction" to "exchangeable_part". I think this name is a little more descriptive and also .tab will allow better shortcuts for both commands "principal_extension" and "exchangeable_part" this way.
7) Rewrote init command for ClusterQuiver? so that it raises an error for unexpected input (e..g ClusterQuiver?('whatever') or ClusterQuiver?('12345') now raises an error instead of constructing an object as it did before).
8a) In QuiverMutationType?.py:
Separated out the cases
['F', 4, [1,2] ] and ['F', 4, [2,1] ] ['G', 4, [1,3] ] as well as ['G', 4, [3,1] ]
in the class QuiverMutationType_Irreducible.
I also added the coercions ['F',4,[2,1]] -> ['F',4,[1,2]] and ['G',2,[3,1]] -> ['G',2,[1,3]] up front in the Mutation Type Casting list to make this clearer.
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 _mutation_type_error command in trac_10527-quiver_mutation_type-gm-additional.patch, it was still incorrect as I forgot to consider the syntax for elliptic G.
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 trac_10538-quiver-gm.patch fixes this.
8c) SIGNIFICANT CHANGE:
I have changed the behavior of ClusterQuiver?(['C',2]), ClusterQuiver?(['GR',[3,7]]), ClusterQuiver?(['F',4,[2,1]]), etc.
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.
9) Very subtle bug in mutate command fixed:
If one writes Q.mutate(0,0), Q.mutate(0,1), or Q.mutate(0,2), etc instead of Q.mutate([0,0]), Q.mutate([0,1]), or Q.mutate([0,2]), there could be weird behavior as Sage interprets the second argument as the parameter 'inplace'. Warnings are now printed if the user possibly makes this mistake.
comment:19 Changed 6 years ago by
- Description modified (diff)
- Summary changed from Implementation of the class Quiver to Implementation of the class ClusterQuiver
comment:20 in reply to: ↑ 17 ; follow-up: ↓ 21 Changed 6 years ago by
Replying to stumpc5:
Replying to robertwb:
With an eye towards startup time, can all these new modules be imported on demand?
I am not very familiar with possible implications. Do you suggest that we should lazily import them?
Yes, exactly (whether using the lazy_import module or another mechanism).
comment:21 in reply to: ↑ 20 Changed 6 years ago by
Replying to robertwb:
Replying to stumpc5:
Replying to robertwb:
With an eye towards startup time, can all these new modules be imported on demand?
I am not very familiar with possible implications. Do you suggest that we should lazily import them?
Yes, exactly (whether using the lazy_import module or another mechanism).
Which you seem to have done; thanks.
comment:22 in reply to: ↑ 18 Changed 6 years ago by
Replying to gmoose05:
This patch fixes a number of issues and also gives the user more flexibility.
Thanks for working on that! I will look at it in a bit, a first quick remark is that you introduced trailing whitespaces: the patchbot says:
+ trac_10538-quiver-gm.patch:190 + # otherwise, an error is raised $
+ trac_10538-quiver-gm.patch:676 + # type ['TR',1] needs to be treated on itself (as there is no edge) $
comment:23 Changed 6 years ago by
Comments on your patch:
- In the documentation, you use
sage: MT = Q._mutation_type
Please usesage: MT = Q.mutation_type()
instead since _ methods and attributes should not be used there, if possible.
- "Returns the restriction to the principal part (i.e. exchangeable part) of self. I.e., the subquiver obtained by deleting the frozen vertices of self." contains two times i.e. . Could you change that?
if type(inplace) is Integer:
Instead of printing a warning, I would suggest to only allow boolean arguments here, and raise a ValueError otherwise.
elif rank == 4 and (twist == [1,2] or twist == [2,1])
Am I seeing it right that you now introduced two QuiverMutationTypes for the two possible twists? I would certainly not like that since both are mutation equivalent. [ I just think about the same kind of behaviour in type D and affine A with an oriented circle - it doesn't seem that you actually treated this case, did you? ] I still prefer the standard quiver method to have an optional argument, but we can maybe discuss that again later today or tomorrow...
Beside that, all changes seem good to me.
Best, Christian
comment:24 Changed 6 years ago by
I just re-uploaded my patch. Christian, thanks for catching those bugs.
- The trailing whitespace should be gone now. I don't think I introduced any new space, but will double-check after the patchbot is finished re-running.
- I agree with you about .mutation_type() instead of ._mutation_type but we have not defined .mutation_type() yet.
Once we add in the newer patches, we should remember to include editing these doc-tests.
- The wording in exchangeable_part has been changed.
- Yes, that it is better - the warnings in mutate have been turned into Value Errors
- Regarding twist [1,2] and [2,1]. This is a little more subtle. So
sage: QuiverMutationType(['F',4,[1,2]])
and
sage: QuiverMutationType(['F',4,[2,1]])
both give you:
['F', 4, [1, 2]]
In fact,
sage: QuiverMutationType(['F',4,[2,1]]) == QuiverMutationType(['F',4,[1,2]]) True
We have as before:
sage: Q = ClusterQuiver(['F',4,[1,2]]); Q Quiver on 6 vertices of type ['F', 4, [1, 2]] sage: Q2 = ClusterQuiver(['F',4,[1,2]]); Q2 Quiver on 6 vertices of type ['F', 4, [1, 2]] sage: Q3 = ClusterQuiver(['F',4,[2,1]]); Q3 Quiver on 6 vertices of type ['F', 4, [1, 2]] sage: Q1._mutation_type == Q3._mutation_type True sage: Q1 == Q2 True
The difference now is:
sage: Q1 == Q3 False
comment:25 Changed 6 years ago by
Christian, the changes we discussed have now been essentially made:
- The ClusterQuiver? constructor passes along all inputs which are of the form QuiverMutationType? to .standard_quiver()
- However, does case-by-case handling of (C,2), (F,4, [2,1]), (G,2, (3,1)), 'GR' type, and 'TR' type.
Note that I employed a strategy slightly different than we discussed: I use QuiverMutationType_Irreducible._digraph to get the desired directed graph rather than repeating these lines here.
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.
- I also now allow an input of
sage: QuiverMutationType(['A',[n,0],1]) ['D', n]
with special cases for small n.
However
sage: ClusterQuiver(['A',[n,0],1])
yields the directed cycle quiver of type D_n rather than the standard Dynkin D_n.
- ClusterQuiver? now also has the command .mutation_type() which yields the _mutation_type, but without trying to compute it when it is not already known.
- I found out where in our code we are missing doctests. It is really only for the graphical commands: .plot(), .show() in QuiverMutationType?.py and .show(), .interact(), and .save_image() in Quiver.py.
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 Changed 6 years ago by
I rebased the patches according to the new lazy import in #10527.
comment:27 follow-up: ↓ 28 Changed 6 years ago by
- Cc drupel@… added
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 in reply to: ↑ 27 Changed 6 years ago by
Replying to gmoose05:
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.
Done.
comment:29 Changed 6 years ago by
- Cc d.rupel@… added; drupel@… removed
- Reviewers changed from Gregg Musiker to Dylan Rupel
comment:30 Changed 5 years ago by
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 Changed 5 years ago by
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 Changed 5 years ago by
I uploaded a new version of the patch with all missing doctests added. Let's see what the patchbot says...
comment:33 Changed 5 years ago by
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 follow-up: ↓ 35 Changed 5 years ago by
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 in reply to: ↑ 34 Changed 5 years ago by
Replying to drupel:
I did some reorganizing of the examples, is the placement of my colons correct?
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...
I made a small change in the mutate() method so that it remembers the QuiverMutationType? when inplace=False.
good call, thanks!
comment:36 Changed 5 years ago by
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
comment:37 Changed 5 years ago by
comment:38 Changed 5 years ago by
- Status changed from needs_review to positive_review
comment:39 follow-up: ↓ 40 Changed 5 years ago by
- Status changed from positive_review to needs_work
sage -t --long -force_lib devel/sage/sage/combinat/cluster_algebra_quiver/quiver.py ********************************************************************** File "/release/merger/sage-5.6.beta2/devel/sage-main/sage/combinat/cluster_algebra_quiver/quiver.py", line 501: sage: Q.interact() # long time Expected nothing Got: 'The interactive mode only runs in the Sage notebook.' **********************************************************************
Changed 5 years ago by
comment:40 in reply to: ↑ 39 Changed 5 years ago by
- Status changed from needs_work to positive_review
Replying to jdemeyer:
Fixed!
comment:41 Changed 5 years ago by
- Merged in set to sage-5.6.beta3
- Resolution set to fixed
- Status changed from positive_review to closed
comment:42 Changed 5 years ago by
- Owner changed from (none) to etn40ff
Changed 5 years ago by
comment:43 follow-up: ↓ 44 Changed 5 years ago by
There is a minor issue in qmu_save: it does not work whenever the quiver has 0<m!=n frozen vertices. Please include the attached patch.
comment:44 in reply to: ↑ 43 ; follow-up: ↓ 45 Changed 5 years ago by
Replying to etn40ff:
There is a minor issue in qmu_save: it does not work whenever the quiver has 0<m!=n frozen vertices. Please include the attached patch.
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
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...