Opened 6 years ago

Closed 4 years ago

Last modified 4 years ago

#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 gmoose05)

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)

trac_10538-quiver-cs.patch (63.7 KB) - added by stumpc5 4 years ago.
trac_10538_qmu_save.patch (1.4 KB) - added by etn40ff 4 years ago.

Download all attachments as: .zip

Change History (47)

comment:1 Changed 6 years ago by stumpc5

  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 6 years ago by stumpc5

  • Dependencies set to 10527

comment:3 Changed 6 years ago by chapoton

  • Dependencies changed from 10527 to #10527

comment:4 Changed 6 years ago by stumpc5

  • Owner changed from sage-combinat to (none)

comment:5 Changed 6 years ago by stumpc5

  • Milestone set to sage-4.7.2

comment:6 Changed 6 years ago by stumpc5

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 follow-up: Changed 5 years ago by JStarx

  • 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 5 years ago by stumpc5

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: Changed 5 years ago by 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)?

comment:10 in reply to: ↑ 9 Changed 5 years ago by stumpc5

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 5 years ago by hthomas

I have added a review patch, but I haven't put anything in it yet.

cheers,

Hugh

comment:12 Changed 5 years ago by hthomas

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 5 years ago by stumpc5

  • Description modified (diff)
  • Summary changed from Implementation of the classes ClusterSeed and Quiver to Implementation of the class Quiver

comment:14 Changed 5 years ago by stumpc5

  • Status changed from needs_info to needs_review

comment:15 Changed 5 years ago by gmoose05

  • Reviewers set to Gregg Musiker

comment:16 follow-up: Changed 5 years ago by robertwb

With an eye towards startup time, can all these new modules be imported on demand?

comment:17 in reply to: ↑ 16 ; follow-up: Changed 5 years ago by 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?

comment:18 follow-up: Changed 5 years ago by gmoose05

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 5 years ago by gmoose05

  • 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: Changed 5 years ago by 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).

comment:21 in reply to: ↑ 20 Changed 5 years ago by robertwb

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 5 years ago by stumpc5

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 5 years ago by stumpc5

Comments on your patch:

  • In the documentation, you use
    sage: MT = Q._mutation_type
    
    Please use
    sage: 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 5 years ago by gmoose05

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
Last edited 5 years ago by stumpc5 (previous) (diff)

comment:25 Changed 5 years ago by gmoose05

Christian, the changes we discussed have now been essentially made:

  • 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 5 years ago by stumpc5

I rebased the patches according to the new lazy import in #10527.

comment:27 follow-up: Changed 5 years ago by gmoose05

  • 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 5 years ago by stumpc5

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 4 years ago by drupel

  • Cc d.rupel@… added; drupel@… removed
  • Reviewers changed from Gregg Musiker to Dylan Rupel

comment:30 Changed 4 years ago by gmoose05

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 4 years ago by drupel

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 4 years ago by stumpc5

I uploaded a new version of the patch with all missing doctests added. Let's see what the patchbot says...

comment:33 Changed 4 years ago by stumpc5

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: Changed 4 years ago by drupel

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 4 years ago by stumpc5

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 4 years ago by stumpc5

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 4 years ago by gmoose05

  • Authors changed from Christian Stump to Christian Stump, Gregg Musiker

comment:38 Changed 4 years ago by drupel

  • Status changed from needs_review to positive_review

comment:39 follow-up: Changed 4 years ago by jdemeyer

  • 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 4 years ago by stumpc5

comment:40 in reply to: ↑ 39 Changed 4 years ago by stumpc5

  • Status changed from needs_work to positive_review

Replying to jdemeyer:

Fixed!

comment:41 Changed 4 years ago by jdemeyer

  • Merged in set to sage-5.6.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:42 Changed 4 years ago by etn40ff

  • Owner changed from (none) to etn40ff

Changed 4 years ago by etn40ff

comment:43 follow-up: Changed 4 years ago by 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.

comment:44 in reply to: ↑ 43 ; follow-up: Changed 4 years ago by gmoose05

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

comment:45 in reply to: ↑ 44 Changed 4 years ago by etn40ff

Hi Gregg, I just did as you suggested: the new ticket is #14851; you are in cc. Best S.

PS: you might be interested in #14844 too

Note: See TracTickets for help on using tickets.