#19339 closed enhancement (fixed)
Use digraph labels if present for ClusterAlgebra and ClusterQuiver
Reported by:  aram.dermenjian  Owned by:  

Priority:  minor  Milestone:  sage8.1 
Component:  algebra  Keywords:  cluster, cluster algebra, quiver, cluster quiver, IMA coding sprints 
Cc:  gmoose05, benstrasser, egunawan, aram.dermenjian  Merged in:  
Authors:  Ben Strasser  Reviewers:  Aram Dermenjian, Emily Gunawan, Jacob P. Matherne, Travis Scrimshaw, Gregg Musiker 
Report Upstream:  N/A  Work issues:  
Branch:  5d8224c (Commits)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
Information from Gregg Musiker
I recently realized that if one starts with a digraph D
(with vertices labeled however) and then writes
S = ClusterSeed(D)
, then the code will effectively call
Q = ClusterQuiver(D)
and then assignS = ClusterSeed(Q)
.
In the same spirit as the last ticket, I think it makes sense to add other vertex names to ClusterQuiver
and allow these to be passed on to ClusterSeed
. Since we only changed the behavior of cluster_seed.py
in the last ticket, I believe the behavior of quiver.py
was unaffected.
In particular, if one writes ClusterSeed(D)
or ClusterQuiver(D)
, then I believe Sage should then use the vertices of digraph as labels rather than defaulting to [0,...,n1]
. (Although we will have to think somewhat about backwards compatibility.)
In particular, I'm thinking of two changes.
1) In ClusterQuiver.__init__()
, if a digraph is the input, then the vertices of ClusterQuiver
agree with the labels of the vertices of the input digraph. (Although for internal data, sage would also have a dictionary from {0,..., n1}
to these vertex labels so that it understands the "ijth" entry of the Bmatrix for instance.
2) In ClusterSeed.__init__()
, if a ClusterQuiver
is the input, then it uses those vertices as labels for the cluster variables as if the user had given user_labels
.
The only exception would be if the user puts in such a ClusterQuiver
and also user_labels
as a separate input, then the new user_labels
should override the old ones from the ClusterQuiver
.
Change History (70)
comment:1 Changed 4 years ago by
 Cc gmoose05 added
 Component changed from PLEASE CHANGE to algebra
 Description modified (diff)
 Priority changed from major to minor
comment:2 Changed 4 years ago by
 Keywords clusters added
 Type changed from PLEASE CHANGE to enhancement
comment:3 Changed 4 years ago by
 Keywords cluster added; clusters removed
comment:4 Changed 4 years ago by
 Cc benstrasser added
comment:5 Changed 4 years ago by
 Branch set to u/aram.dermenjian/use_digraph_labels_if_present_for_clusteralgebra_and_clusterquiver
comment:6 Changed 4 years ago by
 Commit set to 44dfc308b00fffd467b9ad124583b50a79585766
comment:7 followup: ↓ 8 Changed 4 years ago by
 Branch changed from u/aram.dermenjian/use_digraph_labels_if_present_for_clusteralgebra_and_clusterquiver to u/benstrasser/use_digraph_labels_if_present_for_clusteralgebra_and_clusterquiver
comment:8 in reply to: ↑ 7 Changed 4 years ago by
 Commit changed from 44dfc308b00fffd467b9ad124583b50a79585766 to e9af9bba339060a05bf003bf0957328a3fe4c89e
Replying to benstrasser:
I changed ClusterQuiver? and ClusterSeed? to maintain lists of mutable and immutable vertices throughout, and ensured that they pass these lists around when necessary. Thus, the user should be able to submit lists of frozen vertices and mutate them freely. Let me know what you think. If these seem like good changes, we can clean up the documentation and finalize.
New commits:
e9af9bb  allows user to submit a list of frozen vertices, preserves vertex labels when mutated

comment:9 Changed 4 years ago by
 Commit changed from e9af9bba339060a05bf003bf0957328a3fe4c89e to 210e31bc4b79d160c0d1f6ef498ef9c86488f5b4
Branch pushed to git repo; I updated commit sha1. New commits:
210e31b  ensures vertex labels are preserved under principal extensions

comment:10 Changed 4 years ago by
 Commit changed from 210e31bc4b79d160c0d1f6ef498ef9c86488f5b4 to 3548f9309ad73d63df434eb2ac70dfe5edab2ba9
comment:11 Changed 4 years ago by
 Branch changed from u/benstrasser/use_digraph_labels_if_present_for_clusteralgebra_and_clusterquiver to u/aram.dermenjian/use_digraph_labels_if_present_for_clusteralgebra_and_clusterquiver
comment:12 Changed 3 years ago by
 Branch changed from u/aram.dermenjian/use_digraph_labels_if_present_for_clusteralgebra_and_clusterquiver to u/benstrasser/use_digraph_labels_if_present_for_clusteralgebra_and_clusterquiver
comment:13 Changed 3 years ago by
 Branch changed from u/benstrasser/use_digraph_labels_if_present_for_clusteralgebra_and_clusterquiver to u/gmoose05/use_digraph_labels_if_present_for_clusteralgebra_and_clusterquiver
 Commit changed from 3548f9309ad73d63df434eb2ac70dfe5edab2ba9 to 68ee5107ca882a21d3270be286cc758b744f2f9d
Merged to sage.7.1.beta5
comment:14 Changed 3 years ago by
 Branch changed from u/gmoose05/use_digraph_labels_if_present_for_clusteralgebra_and_clusterquiver to u/benstrasser/use_digraph_labels_if_present_for_clusteralgebra_and_clusterquiver
comment:15 Changed 3 years ago by
 Commit changed from 68ee5107ca882a21d3270be286cc758b744f2f9d to ee4c9653239ea74be77feb776679beacefa27595
Branch pushed to git repo; I updated commit sha1. New commits:
ee4c965  Added input classification to the mutate command

comment:16 Changed 3 years ago by
 Commit changed from ee4c9653239ea74be77feb776679beacefa27595 to 0cadf2086f397b0546892558409527655b495741
Branch pushed to git repo; I updated commit sha1. New commits:
0cadf20  Cleaned up code and added additional tests

comment:17 Changed 3 years ago by
 Commit changed from 0cadf2086f397b0546892558409527655b495741 to e2599cef9132a9e203f259f55c7615576b86a333
Branch pushed to git repo; I updated commit sha1. New commits:
e2599ce  Sped up mutation at lists of cluster variables by remembering cluster indices

comment:18 Changed 3 years ago by
 Cc egunawan added
comment:19 Changed 3 years ago by
 Commit changed from e2599cef9132a9e203f259f55c7615576b86a333 to eb2f6f91394e6b6c10e371d55e7ebac1a870ca50
Branch pushed to git repo; I updated commit sha1. New commits:
eb2f6f9  updated the plot command to work with vertex labels

comment:20 Changed 3 years ago by
 Keywords algebra quiver quiver added
comment:21 Changed 3 years ago by
 Commit changed from eb2f6f91394e6b6c10e371d55e7ebac1a870ca50 to 5ccae7c18f22da1984725e5d72d1ffc2b9d7adc4
Branch pushed to git repo; I updated commit sha1. New commits:
5ccae7c  Fixed the proliferation of warning messages with the "mutation_class" command

comment:22 Changed 3 years ago by
 Status changed from new to needs_review
comment:23 Changed 3 years ago by
 Status changed from needs_review to needs_work
Hi Ben, These are my suggestions for now:
 In the doc for
ClusterQuiver
taking digraph as input, could you add another linesage: for Q.nlist()
so the user will read thatClusterQuiver
will remember labels from the input digraph.
 The following bug does not exist before this ticket was pulled:
sage: Q=ClusterQuiver(DiGraph([[0,5]])) sage: Q.b_matrix() [ 0 1] [1 0] sage: Q.mutate(0) IndexError: index out of range
 Also, I think we should either explain the following behavior in the doc, or change the code so that the behavior for
mutation
is more consistent. I think either is fine. The following works wellsage: S=ClusterSeed(DiGraph([['a','b']])) sage: S.mutate('a')
but the following throws an error
sage: S=ClusterSeed(DiGraph([[5,'b']])) sage: S.mutate(5) ValueError: The quiver can only be mutated at a vertex or at a sequence of vertices
 If possible, could you get the recent version of Sage (if not 7.3beta 6, then at least 7.2 master branch), and then add your code on top of this recent Sage? Currently your branch is marked as red because it cannot automatically merge.
comment:24 Changed 3 years ago by
 Cc aram.dermenjian added
 Reviewers set to Aram Dermenjian, Emily Gunawan
comment:25 Changed 3 years ago by
 Branch changed from u/benstrasser/use_digraph_labels_if_present_for_clusteralgebra_and_clusterquiver to u/benstrasser/19339
 Commit changed from 5ccae7c18f22da1984725e5d72d1ffc2b9d7adc4 to 1aabf461525ff0bcd21b180126b139fb3d3659b7
comment:26 Changed 3 years ago by
Some quick comments:
 I believe you want
user_labels
to be a list, so once we switch to Python3, the current code won't work. So I recommend (and is also faster):user_labels = [ZZ(x) if type(x) == int else x for x in user_labels]
 Instead of
elif type(user_labels[i]) in [list,tuple]:
, you should useelif isinstance(user_labels[i], (list, tuple)):
(unless you want to explicitly forbid any subclasses oflist
ortuple
).  Python uses different conventions than, e.g., C++/Java. So variables
isVertices
should beis_vertices
.  You have an erroneous character:
For the compendium on the cluster algebra and quiver package see ::  +f http://arxiv.org/abs/1102.4844.
comment:27 Changed 3 years ago by
doc does not build:
+[dochtml] [combinat ] /home/kevin/sagepatchbot/local/lib/python2.7/sitepackages/sage/combinat/cluster_algebra_quiver/cluster_seed.py:docstring of sage.combinat.cluster_algebra_quiver.cluster_seed.ClusterSeed.mutate:29: ERROR: Unexpected indentation. +[dochtml] [combinat ] /home/kevin/sagepatchbot/local/lib/python2.7/sitepackages/sage/combinat/cluster_algebra_quiver/quiver.py:docstring of sage.combinat.cluster_algebra_quiver.quiver:21: ERROR: Unexpected indentation.
comment:28 Changed 3 years ago by
 Branch changed from u/benstrasser/19339 to u/gmoose05/19339
 Commit 1aabf461525ff0bcd21b180126b139fb3d3659b7 deleted
comment:29 Changed 3 years ago by
 Branch changed from u/gmoose05/19339 to u/benstrasser/19339
 Commit set to 1aabf461525ff0bcd21b180126b139fb3d3659b7
Last 10 new commits:
a1a2be3  Added functionality so that the user can specify the type of input to the ``mutate`` command

68ee510  merged to sage7.1beta5

7b91bb2  Ensures that the cluster is unchanged when mutating at a sequence of cluster variables

ee4c965  Added input classification to the mutate command

0cadf20  Cleaned up code and added additional tests

e2599ce  Sped up mutation at lists of cluster variables by remembering cluster indices

eb2f6f9  updated the plot command to work with vertex labels

5ccae7c  Fixed the proliferation of warning messages with the "mutation_class" command

23fd599  merged with 7.3.beta7

1aabf46  slightly edited documentation

comment:30 Changed 3 years ago by
 Branch changed from u/benstrasser/19339 to u/gmoose05/19339
 Commit 1aabf461525ff0bcd21b180126b139fb3d3659b7 deleted
comment:31 Changed 3 years ago by
 Commit set to c8d4927545d6ab893499b8379c2537e3a6de90c0
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
68ee510  merged to sage7.1beta5

7b91bb2  Ensures that the cluster is unchanged when mutating at a sequence of cluster variables

ee4c965  Added input classification to the mutate command

0cadf20  Cleaned up code and added additional tests

e2599ce  Sped up mutation at lists of cluster variables by remembering cluster indices

eb2f6f9  updated the plot command to work with vertex labels

5ccae7c  Fixed the proliferation of warning messages with the "mutation_class" command

23fd599  merged with 7.3.beta7

1aabf46  slightly edited documentation

c8d4927  Udpated how parameter frozen is used and updated most inputs of n and m to nlist and mlist

comment:32 Changed 3 years ago by
 Commit changed from c8d4927545d6ab893499b8379c2537e3a6de90c0 to 3f6ef57db95acc579f010f5a66768c9eb8768ca1
Branch pushed to git repo; I updated commit sha1. New commits:
3f6ef57  Fixed set_frozen command and added doctests

comment:33 Changed 3 years ago by
 Branch changed from u/gmoose05/19339 to u/benstrasser/19339
 Commit changed from 3f6ef57db95acc579f010f5a66768c9eb8768ca1 to 62ee79a91b5c26da7f8d8f2ad6c2f18dbbc4b5d2
comment:34 Changed 3 years ago by
 Branch changed from u/benstrasser/19339 to u/gmoose05/19339
 Commit changed from 62ee79a91b5c26da7f8d8f2ad6c2f18dbbc4b5d2 to 3f6ef57db95acc579f010f5a66768c9eb8768ca1
comment:35 Changed 3 years ago by
 Commit changed from 3f6ef57db95acc579f010f5a66768c9eb8768ca1 to 899d82e7f6d9e54767ef3acc867bdb8bb2fc2883
Branch pushed to git repo; I updated commit sha1. New commits:
9a446c8  Made small changes based on comments by tscrim

62ee79a  Added a method remove_frozen_edges to remove edges between frozen vertices in self.digraph

899d82e  Fixes mutation_class() and b_matrix_class() proedures for quivers from generally labeled digraphs

comment:36 Changed 3 years ago by
Anyone know why the branch name (in the above description) isn't green?
comment:37 Changed 3 years ago by
Typically it means a conflict with the latest beta.
comment:38 Changed 3 years ago by
 Branch changed from u/gmoose05/19339 to u/aram.dermenjian/19339
comment:39 Changed 3 years ago by
 Commit changed from 899d82e7f6d9e54767ef3acc867bdb8bb2fc2883 to f9b3fe44ccbce06f9214e746e032ee06f0371e56
 Milestone changed from sage6.9 to sage7.4
comment:40 Changed 2 years ago by
 Branch changed from u/aram.dermenjian/19339 to u/benstrasser/19339
 Commit changed from f9b3fe44ccbce06f9214e746e032ee06f0371e56 to 78e248dec2ae70b9450891b77e9ee88a65e550fc
New commits:
78e248d  Rebased to the latest beta

comment:42 Changed 2 years ago by
Do not use xrange in .py files (see patchbot plugin report).
If you need an iterator range, use from from six.moves import range
comment:43 Changed 2 years ago by
 Commit changed from 78e248dec2ae70b9450891b77e9ee88a65e550fc to 46679b9732d2472e59f7617cc71830330fdf7a19
Branch pushed to git repo; I updated commit sha1. New commits:
46679b9  removed xrange from cluster_seed

comment:44 Changed 2 years ago by
 Branch changed from u/benstrasser/19339 to u/aram.dermenjian/19339
comment:45 Changed 2 years ago by
 Commit changed from 46679b9732d2472e59f7617cc71830330fdf7a19 to aa5ea25c480fc53618bccbf14a31d0b2ed3a80c7
 Status changed from needs_work to needs_review
comment:46 Changed 2 years ago by
 Keywords IMA coding sprints added
comment:47 Changed 2 years ago by
 Status changed from needs_review to needs_work
comment:48 Changed 2 years ago by
 Branch changed from u/aram.dermenjian/19339 to u/benstrasser/19339
 Commit changed from aa5ea25c480fc53618bccbf14a31d0b2ed3a80c7 to ee5cea619cac22213649c5d1096f91be36bb8c20
New commits:
ee5cea6  allows user_labels with universal_extansion and negative user_labels

comment:49 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:50 Changed 2 years ago by
 Status changed from needs_review to needs_work
comment:51 Changed 2 years ago by
 Commit changed from ee5cea619cac22213649c5d1096f91be36bb8c20 to c9de843a888ffd5f1528bf337ce2752782980dbd
Branch pushed to git repo; I updated commit sha1. New commits:
c9de843  Ensured vertex labels are preserved even when not mutating in place

comment:52 Changed 2 years ago by
 Commit changed from c9de843a888ffd5f1528bf337ce2752782980dbd to d3f29b09ad90e7fedb71f8ad7fcace52197670b9
Branch pushed to git repo; I updated commit sha1. New commits:
d3f29b0  Added documentation to the *mutation_class* method

comment:53 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:54 Changed 2 years ago by
 Reviewers changed from Aram Dermenjian, Emily Gunawan to Aram Dermenjian, Emily Gunawan, Jacob P. Matherne
comment:55 Changed 2 years ago by
 Status changed from needs_review to positive_review
Seems to be working as intended, and the patchbot is green.
comment:56 Changed 2 years ago by
 Milestone changed from sage8.2 to sage8.1
 Reviewers changed from Aram Dermenjian, Emily Gunawan, Jacob P. Matherne to Aram Dermenjian, Emily Gunawan, Jacob P. Matherne, Travis Scrimshaw
 Status changed from positive_review to needs_work
Some comments, most of them are minor:
 There is a
warn
function (see, e.g.,sets/disjoint_union_enumerated_sets.py
). I feel like you should use that instead ofprint
statements, but I'm not 100% sure.  The
copy
is redundant herecopy( list(data._nlist) )
becauselist
already makes a (shallow) copy.  The
keys()
is unnecessary inlabelset = set(user_labels.keys())
.  Do you really only want to test
type(x) == int
in this line:I feel it would be better to douser_labels = [ZZ(x) if type(x) == int else x for x in user_labels]
if x in ZZ
.  If
user_labels
does conflict, are you sure you only want a string printed? This feels like it should be an error.  Change
type(user_labels[key]) in [list,tuple] +isinstance(user_labels[key], (list, tuple))
 You need a blank line here between text and doctests, e.g., here:
There is at least one other place where this is needed.
For a cluster seed from an arbitrarily labelled digraph:: + sage: S = ClusterSeed(DiGraph([['a','b'],['b','c']]),frozen=['b']) sage: S.cluster_class() [[a, c], [a, (b + 1)/c], [(b + 1)/a, c], [(b + 1)/a, (b + 1)/c]]
 From the warning message, I don't understand the added check here:
if frozen is not None and type(frozen) in [int,Integer]: print('The input data is a matrix, therefore the additional parameter frozen is ignored. Frozen vertices read off accordingly if the matrix is not square.')
 Why is this downgraded to only a warning?
+ warning_given = False for edge in dg.edge_iterator():  if edge[0] >= n and edge[1] >= n:  raise ValueError("The input digraph contains edges within the frozen vertices") + if edge[0] >= n and edge[1] >= n and not warning_given: + #raise ValueError("The input digraph contains edges within the frozen vertices") + print('Warning: The input digraph contained edges within the frozen vertices. Use remove_frozen_edges to remove these edges.') + warning_given = True
nlist()
andmlist()
are not very descriptive method names (however, it is fine for the internal attributes). I think it would be better to call themfree_vertices()
andfrozen_vertices()
. Never do
keep_previous_frozen == False
. Simply donot keep_previous_frozen
. Similar forkeep_previous_frozen == True
.  You are breaking PEP8 in a lot of places:
 Arguments should not have spaces around the
=
, e.g.,def foo(x=5)
and notdef foo(x = 5)
.  Same holds true for default arguments to functions.
 Binary operations should have spaces to either side:
strng = strng + "_" + j
. Although IMO you should leave this type aloneuser_labels.keys()[n:n+m]
as the[]
operator takes priority. dict
input as, e.g.,{x: x for x in range(5)}
. Commas will always have spaces after them.
 Arguments should not have spaces around the
 Break long (> 80 characters) docstrings.
 Use
Return
instead ofReturns
in the 1line doc description (at least do not make things worse in your added functions/methods).  There are a number of docstring formatting issues, e.g., you should code format things like
``self``
or variable names.  You have added a lot of unnecessary whitespace to the docstrings. For example:
It is good to be consistent with the rest of Sage (at most 1 line after doctests before the
@@ 120,6 +121,14 @@ class ClusterSeed(SageObject): sage: S = ClusterSeed(['A',4]); S.use_fpolys(False); S._use_fpolys False + + sage: S = ClusterSeed(DiGraph([['a','b'],['c','b'],['c','d'],['e','d']]),frozen = ['c']); S + A seed for a cluster algebra of rank 4 with 1 frozen variable + + sage: S = ClusterSeed(['D',4],user_labels = [1,0,1,2]);S + A seed for a cluster algebra of rank 4 of type ['D', 4] + + """ def __init__(self, data, frozen=None, is_principal=False, user_labels=None, user_labels_prefix='x'):
"""
).  Remove trailing whitespace.
comment:57 Changed 2 years ago by
 Commit changed from d3f29b09ad90e7fedb71f8ad7fcace52197670b9 to 8b05a7c9cc4106a132aef2890f394a75b8878449
comment:58 Changed 2 years ago by
 Commit changed from 8b05a7c9cc4106a132aef2890f394a75b8878449 to 4e7916e8def9fa1e480c9230d46a838db9d85fca
Branch pushed to git repo; I updated commit sha1. New commits:
4e7916e  changed dictionary input to follow PEP8

comment:59 followup: ↓ 62 Changed 2 years ago by
Thanks for the detailed comments, Travis. I've done my best to address your concerns, although its possible I missed something.
 Some of the 'print('Warning: ... 's which I added were really just clarifying default behavior, so I am not sure they should be warnings. I have removed the `Warning' from this string. I am not sure if this is the correct way to do such a thing in general, but it is at least consistent with the rest of the cluster_algebra_quiver package (see, for example, the messages about ignoring frozen vertices). Perhaps a later ticket should go through and add the
warn
command where appropriate.
 Regarding the warning vs error message for edges between frozen vertices, I had downgraded the error to a warning in order to play nicely with the new
set_frozen
command. I can't remember exactly why Gregg and I decided to add this functionality. Furthermore, this seems to go a bit beyond the stated goals of this ticket. For the moment I've decided to remove theset_frozen
andremove_frozen_edges
methods. I can add these commands (with or without the warning message) with a later ticket if someone wants it.
 I did my best to change 'returns' to 'return' on the docstrings for the methods which we added here. As you point out, this is one of many stylistic inconsistencies throughout the cluster_algebra_quiver package. Perhaps a later ticket could go through and ensure the rest of the package conforms with this. The PEP8 issues are similar  I've done my best to make the new code conform, but there appears to be existing code which doesn't quite follow these standards. If this is a serious issue, someone could try to make the entire package conform with PEP8 at a later date.
Please let me know if I've missed anything of misinterpreted any of your comments!
New commits:
c45ce6d  made stylistic changes based on comments by tscrim

8b05a7c  made various stylistic changes based on comments by tscrim

4e7916e  changed dictionary input to follow PEP8

comment:60 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:61 Changed 2 years ago by
I just changed it. Thanks for the input!
comment:62 in reply to: ↑ 59 Changed 2 years ago by
 Branch changed from u/benstrasser/19339 to u/tscrim/cluster_diagraph_labels19339
 Commit changed from 4e7916e8def9fa1e480c9230d46a838db9d85fca to d42e9dd6120ce504a5f4f28eed82c6eb7bc9ade6
Replying to benstrasser:
Thanks for the detailed comments, Travis. I've done my best to address your concerns, although its possible I missed something.
Thanks. I went through and made some additional changes and fixes as well, but there's too much to standardize to do it all on this ticket.
 Some of the 'print('Warning: ... 's which I added were really just clarifying default behavior, so I am not sure they should be warnings. I have removed the `Warning' from this string. I am not sure if this is the correct way to do such a thing in general, but it is at least consistent with the rest of the cluster_algebra_quiver package (see, for example, the messages about ignoring frozen vertices). Perhaps a later ticket should go through and add the
warn
command where appropriate.
I would say no, but since the rest of the code here does it, it is better to be consistent.
 I did my best to change 'returns' to 'return' on the docstrings for the methods which we added here. As you point out, this is one of many stylistic inconsistencies throughout the cluster_algebra_quiver package. Perhaps a later ticket could go through and ensure the rest of the package conforms with this. The PEP8 issues are similar  I've done my best to make the new code conform, but there appears to be existing code which doesn't quite follow these standards. If this is a serious issue, someone could try to make the entire package conform with PEP8 at a later date.
Serious, not really. Yet, it makes the code a little more difficult to parse quickly, and it would help give Sage a more polished feel.
If my changes are good, then positive review.
New commits:
f093c68  Merge branch 'u/benstrasser/19339' of git://trac.sagemath.org/sage into u/tscrim/cluster_diagraph_labels19339

d42e9dd  Some additional cleanup of the doc and PEP8.

comment:63 Changed 2 years ago by
 Commit changed from d42e9dd6120ce504a5f4f28eed82c6eb7bc9ade6 to a66a6a5f15995f22af32be5bab12ae228b1daa04
Branch pushed to git repo; I updated commit sha1. New commits:
a66a6a5  Some little more cleanup and whitespace removal.

comment:64 Changed 2 years ago by
There seems to be a new error found by the patchbot now:
sage t long src/sage/combinat/cluster_algebra_quiver/cluster_seed.py
File "src/sage/combinat/cluster_algebra_quiver/cluster_seed.py", line 2356, in sage.combinat.cluster_algebra_quiver.cluster_seed.ClusterSeed?.mutate Failed example:
S = ClusterSeed?(['A', 4], user_labels=['a', 'b', 'c']);
Expected:
Traceback (most recent call last): ... ValueError?: The number of userdefined labels is not the number of exchangeable and frozen variables.
Got:
<BLANKLINE>
I think once this is fixed, that the code would be ready for a positive review.
comment:65 Changed 2 years ago by
Hmm I've run into this kind of issue before... I don't really know how to deal with it since it doesn't seem to occur outside of doctests (i.e. if you manually enter it in terminal). Has anyone seen this before? What tends to cause this?
I'll try to work it out tomorrow.
comment:66 Changed 2 years ago by
 Commit changed from a66a6a5f15995f22af32be5bab12ae228b1daa04 to 5d8224ce89f4c81b35fa252ff91917c5277fa00f
Branch pushed to git repo; I updated commit sha1. New commits:
5d8224c  Fixing doctest and docbuild.

comment:67 Changed 2 years ago by
 Description modified (diff)
It is not a blankline; the patchbot reports an actual error traceback (although the blankline is annoying to have it in the output, it is not something that is actually is checked). It is a simple fix because of changes I made to the error messages to conform to our (and Python's) convention of error messages being lowercase and without periods.
I have also fixed the docbuilding issues reported by the patchbot.
comment:68 Changed 2 years ago by
 Reviewers changed from Aram Dermenjian, Emily Gunawan, Jacob P. Matherne, Travis Scrimshaw to Aram Dermenjian, Emily Gunawan, Jacob P. Matherne, Travis Scrimshaw, Gregg Musiker
 Status changed from needs_review to positive_review
comment:69 Changed 2 years ago by
 Branch changed from u/tscrim/cluster_diagraph_labels19339 to 5d8224ce89f4c81b35fa252ff91917c5277fa00f
 Resolution set to fixed
 Status changed from positive_review to closed
comment:70 Changed 14 months ago by
 Commit 5d8224ce89f4c81b35fa252ff91917c5277fa00f deleted
Why use reduce()
here?
is_cluster_vars = reduce(lambda x, y: isinstance(y, str), seqq, 1) and seed._use_fpolys
This code is equivalent to
if len(seqq) == 0 or isinstance(seqq[1], str): is_cluster_vars = seed._use_fpolys else: is_cluster_vars = False
but is that what is intented?
Or did you perhaps mean
is_cluster_vars = all(isinstance(y, str) for y in seqq) and seed._use_fpolys
I ask because we trying to clean this up in #25571.
I am working on a review patch for this ticket. This will include:
1) A small bug fix to address the issue where digraph labels get lost if the ClusterSeed? is mutated:
2) An enhancement allowing users to specify a list of frozen variables.
New commits:
Vertex relabelling based on digraphs
Fix var initialisation
Allow plotting by default as well