Opened 4 years ago

Closed 2 years ago

Last modified 17 months ago

#19339 closed enhancement (fixed)

Use digraph labels if present for ClusterAlgebra and ClusterQuiver

Reported by: aram.dermenjian Owned by:
Priority: minor Milestone: sage-8.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 tscrim)

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 assign S = 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,...,n-1]. (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,..., n-1} to these vertex labels so that it understands the "ij-th" entry of the B-matrix 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 aram.dermenjian

  • 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 aram.dermenjian

  • Keywords clusters added
  • Type changed from PLEASE CHANGE to enhancement

comment:3 Changed 4 years ago by chapoton

  • Keywords cluster added; clusters removed

comment:4 Changed 4 years ago by gmoose05

  • Cc benstrasser added

comment:5 Changed 4 years ago by aram.dermenjian

  • Branch set to u/aram.dermenjian/use_digraph_labels_if_present_for_clusteralgebra_and_clusterquiver

comment:6 Changed 4 years ago by benstrasser

  • Commit set to 44dfc308b00fffd467b9ad124583b50a79585766

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:

sage: dg = DiGraph([[5,6]])
sage: S = ClusterSeed(dg); S.quiver().digraph().vertices()
[5, 6]
sage: S.mutate(0)
sage: S.quiver().digraph().vertices()
[0, 1]

2) An enhancement allowing users to specify a list of frozen variables.


New commits:

d92f4e7Vertex relabelling based on digraphs
c9d20c2Fix var initialisation
44dfc30Allow plotting by default as well

comment:7 follow-up: Changed 4 years ago by benstrasser

  • 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 benstrasser

  • 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:

e9af9bballows user to submit a list of frozen vertices, preserves vertex labels when mutated

comment:9 Changed 4 years ago by git

  • Commit changed from e9af9bba339060a05bf003bf0957328a3fe4c89e to 210e31bc4b79d160c0d1f6ef498ef9c86488f5b4

Branch pushed to git repo; I updated commit sha1. New commits:

210e31bensures vertex labels are preserved under principal extensions

comment:10 Changed 4 years ago by git

  • Commit changed from 210e31bc4b79d160c0d1f6ef498ef9c86488f5b4 to 3548f9309ad73d63df434eb2ac70dfe5edab2ba9

Branch pushed to git repo; I updated commit sha1. New commits:

62aa969debugged code so that it passes all doc tests
3548f93debugged various doctests

comment:11 Changed 4 years ago by aram.dermenjian

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

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

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

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

  • Commit changed from 68ee5107ca882a21d3270be286cc758b744f2f9d to ee4c9653239ea74be77feb776679beacefa27595

Branch pushed to git repo; I updated commit sha1. New commits:

ee4c965Added input classification to the mutate command

comment:16 Changed 4 years ago by git

  • Commit changed from ee4c9653239ea74be77feb776679beacefa27595 to 0cadf2086f397b0546892558409527655b495741

Branch pushed to git repo; I updated commit sha1. New commits:

0cadf20Cleaned up code and added additional tests

comment:17 Changed 4 years ago by git

  • Commit changed from 0cadf2086f397b0546892558409527655b495741 to e2599cef9132a9e203f259f55c7615576b86a333

Branch pushed to git repo; I updated commit sha1. New commits:

e2599ceSped up mutation at lists of cluster variables by remembering cluster indices

comment:18 Changed 3 years ago by benstrasser

  • Cc egunawan added

comment:19 Changed 3 years ago by git

  • Commit changed from e2599cef9132a9e203f259f55c7615576b86a333 to eb2f6f91394e6b6c10e371d55e7ebac1a870ca50

Branch pushed to git repo; I updated commit sha1. New commits:

eb2f6f9updated the plot command to work with vertex labels

comment:20 Changed 3 years ago by egunawan

  • Keywords algebra quiver quiver added

comment:21 Changed 3 years ago by git

  • Commit changed from eb2f6f91394e6b6c10e371d55e7ebac1a870ca50 to 5ccae7c18f22da1984725e5d72d1ffc2b9d7adc4

Branch pushed to git repo; I updated commit sha1. New commits:

5ccae7cFixed the proliferation of warning messages with the "mutation_class" command

comment:22 Changed 3 years ago by benstrasser

  • Status changed from new to needs_review

comment:23 Changed 3 years ago by egunawan

  • Authors set to aram.dermenjian
  • 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 line sage: for Q.nlist() so the user will read that ClusterQuiver 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 well
    sage: 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 egunawan

  • Authors changed from aram.dermenjian to Ben Strasser
  • Cc aram.dermenjian added
  • Reviewers set to Aram Dermenjian, Emily Gunawan

comment:25 Changed 3 years ago by benstrasser

  • Branch changed from u/benstrasser/use_digraph_labels_if_present_for_clusteralgebra_and_clusterquiver to u/benstrasser/19339
  • Commit changed from 5ccae7c18f22da1984725e5d72d1ffc2b9d7adc4 to 1aabf461525ff0bcd21b180126b139fb3d3659b7

New commits:

23fd599merged with 7.3.beta7
1aabf46slightly edited documentation

comment:26 Changed 3 years ago by tscrim

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 use elif isinstance(user_labels[i], (list, tuple)): (unless you want to explicitly forbid any subclasses of list or tuple).
  • Python uses different conventions than, e.g., C++/Java. So variables isVertices should be is_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 chapoton

doc does not build:

+[dochtml] [combinat ] /home/kevin/sage-patchbot/local/lib/python2.7/site-packages/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/sage-patchbot/local/lib/python2.7/site-packages/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 gmoose05

  • Branch changed from u/benstrasser/19339 to u/gmoose05/19339
  • Commit 1aabf461525ff0bcd21b180126b139fb3d3659b7 deleted

comment:29 Changed 3 years ago by gmoose05

  • Branch changed from u/gmoose05/19339 to u/benstrasser/19339
  • Commit set to 1aabf461525ff0bcd21b180126b139fb3d3659b7

Last 10 new commits:

a1a2be3Added functionality so that the user can specify the type of input to the ``mutate`` command
68ee510merged to sage7.1beta5
7b91bb2Ensures that the cluster is unchanged when mutating at a sequence of cluster variables
ee4c965Added input classification to the mutate command
0cadf20Cleaned up code and added additional tests
e2599ceSped up mutation at lists of cluster variables by remembering cluster indices
eb2f6f9updated the plot command to work with vertex labels
5ccae7cFixed the proliferation of warning messages with the "mutation_class" command
23fd599merged with 7.3.beta7
1aabf46slightly edited documentation

comment:30 Changed 3 years ago by gmoose05

  • Branch changed from u/benstrasser/19339 to u/gmoose05/19339
  • Commit 1aabf461525ff0bcd21b180126b139fb3d3659b7 deleted

comment:31 Changed 3 years ago by git

  • Commit set to c8d4927545d6ab893499b8379c2537e3a6de90c0

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

68ee510merged to sage7.1beta5
7b91bb2Ensures that the cluster is unchanged when mutating at a sequence of cluster variables
ee4c965Added input classification to the mutate command
0cadf20Cleaned up code and added additional tests
e2599ceSped up mutation at lists of cluster variables by remembering cluster indices
eb2f6f9updated the plot command to work with vertex labels
5ccae7cFixed the proliferation of warning messages with the "mutation_class" command
23fd599merged with 7.3.beta7
1aabf46slightly edited documentation
c8d4927Udpated how parameter frozen is used and updated most inputs of n and m to nlist and mlist

comment:32 Changed 3 years ago by git

  • Commit changed from c8d4927545d6ab893499b8379c2537e3a6de90c0 to 3f6ef57db95acc579f010f5a66768c9eb8768ca1

Branch pushed to git repo; I updated commit sha1. New commits:

3f6ef57Fixed set_frozen command and added doctests

comment:33 Changed 3 years ago by benstrasser

  • Branch changed from u/gmoose05/19339 to u/benstrasser/19339
  • Commit changed from 3f6ef57db95acc579f010f5a66768c9eb8768ca1 to 62ee79a91b5c26da7f8d8f2ad6c2f18dbbc4b5d2

New commits:

9a446c8Made small changes based on comments by tscrim
62ee79aAdded a method remove_frozen_edges to remove edges between frozen vertices in self.digraph

comment:34 Changed 3 years ago by gmoose05

  • Branch changed from u/benstrasser/19339 to u/gmoose05/19339
  • Commit changed from 62ee79a91b5c26da7f8d8f2ad6c2f18dbbc4b5d2 to 3f6ef57db95acc579f010f5a66768c9eb8768ca1

comment:35 Changed 3 years ago by git

  • Commit changed from 3f6ef57db95acc579f010f5a66768c9eb8768ca1 to 899d82e7f6d9e54767ef3acc867bdb8bb2fc2883

Branch pushed to git repo; I updated commit sha1. New commits:

9a446c8Made small changes based on comments by tscrim
62ee79aAdded a method remove_frozen_edges to remove edges between frozen vertices in self.digraph
899d82eFixes mutation_class() and b_matrix_class() proedures for quivers from generally labeled digraphs

comment:36 Changed 3 years ago by gmoose05

Anyone know why the branch name (in the above description) isn't green?

comment:37 Changed 3 years ago by tscrim

Typically it means a conflict with the latest beta.

comment:38 Changed 3 years ago by aram.dermenjian

  • Branch changed from u/gmoose05/19339 to u/aram.dermenjian/19339

comment:39 Changed 3 years ago by aram.dermenjian

  • Commit changed from 899d82e7f6d9e54767ef3acc867bdb8bb2fc2883 to f9b3fe44ccbce06f9214e746e032ee06f0371e56
  • Milestone changed from sage-6.9 to sage-7.4

New commits:

2aef961Merge with develop
e0fde25doctest fixing
f9b3fe4Merge branch 'develop' into t/19339/19339

comment:40 Changed 2 years ago by benstrasser

  • Branch changed from u/aram.dermenjian/19339 to u/benstrasser/19339
  • Commit changed from f9b3fe44ccbce06f9214e746e032ee06f0371e56 to 78e248dec2ae70b9450891b77e9ee88a65e550fc

New commits:

78e248dRebased to the latest beta

comment:41 Changed 2 years ago by benstrasser

  • Milestone changed from sage-7.4 to sage-8.2

aram.dermenjian

comment:42 Changed 2 years ago by chapoton

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 git

  • Commit changed from 78e248dec2ae70b9450891b77e9ee88a65e550fc to 46679b9732d2472e59f7617cc71830330fdf7a19

Branch pushed to git repo; I updated commit sha1. New commits:

46679b9removed xrange from cluster_seed

comment:44 Changed 2 years ago by aram.dermenjian

  • Branch changed from u/benstrasser/19339 to u/aram.dermenjian/19339

comment:45 Changed 2 years ago by benstrasser

  • Commit changed from 46679b9732d2472e59f7617cc71830330fdf7a19 to aa5ea25c480fc53618bccbf14a31d0b2ed3a80c7
  • Status changed from needs_work to needs_review

Passes src/sage/rings/function_field and src/sage/graphs locally even though it times out on the patch bot.


New commits:

ffcfb60Allow numbers in mutations and add doc tests
553b579Remove xrange for Python3
aa5ea25merge

comment:46 Changed 2 years ago by benstrasser

  • Keywords IMA coding sprints added

comment:47 Changed 2 years ago by benstrasser

  • Status changed from needs_review to needs_work

comment:48 Changed 2 years ago by benstrasser

  • Branch changed from u/aram.dermenjian/19339 to u/benstrasser/19339
  • Commit changed from aa5ea25c480fc53618bccbf14a31d0b2ed3a80c7 to ee5cea619cac22213649c5d1096f91be36bb8c20

New commits:

ee5cea6allows user_labels with universal_extansion and negative user_labels

comment:49 Changed 2 years ago by benstrasser

  • Status changed from needs_work to needs_review

comment:50 Changed 2 years ago by benstrasser

  • Status changed from needs_review to needs_work

comment:51 Changed 2 years ago by git

  • Commit changed from ee5cea619cac22213649c5d1096f91be36bb8c20 to c9de843a888ffd5f1528bf337ce2752782980dbd

Branch pushed to git repo; I updated commit sha1. New commits:

c9de843Ensured vertex labels are preserved even when not mutating in place

comment:52 Changed 2 years ago by git

  • Commit changed from c9de843a888ffd5f1528bf337ce2752782980dbd to d3f29b09ad90e7fedb71f8ad7fcace52197670b9

Branch pushed to git repo; I updated commit sha1. New commits:

d3f29b0Added documentation to the *mutation_class* method

comment:53 Changed 2 years ago by benstrasser

  • Status changed from needs_work to needs_review

comment:54 Changed 2 years ago by jmatherne

  • Reviewers changed from Aram Dermenjian, Emily Gunawan to Aram Dermenjian, Emily Gunawan, Jacob P. Matherne

comment:55 Changed 2 years ago by jmatherne

  • 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 tscrim

  • Milestone changed from sage-8.2 to sage-8.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 of print statements, but I'm not 100% sure.
  • The copy is redundant here copy( list(data._nlist) ) because list already makes a (shallow) copy.
  • The keys() is unnecessary in labelset = set(user_labels.keys()).
  • Do you really only want to test type(x) == int in this line:
    user_labels = [ZZ(x) if type(x) == int else x for x in user_labels]
    
    I feel it would be better to do 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:
             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]]
    
    There is at least one other place where this is needed.
  • 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() and mlist() are not very descriptive method names (however, it is fine for the internal attributes). I think it would be better to call them free_vertices() and frozen_vertices().
  • Never do keep_previous_frozen == False. Simply do not keep_previous_frozen. Similar for keep_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 not def 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 alone user_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.
  • Break long (> 80 characters) docstrings.
  • Use Return instead of Returns in the 1-line 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:
    @@ -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'):
    
    It is good to be consistent with the rest of Sage (at most 1 line after doctests before the """).
  • Remove trailing whitespace.

comment:57 Changed 2 years ago by git

  • Commit changed from d3f29b09ad90e7fedb71f8ad7fcace52197670b9 to 8b05a7c9cc4106a132aef2890f394a75b8878449

Branch pushed to git repo; I updated commit sha1. New commits:

c45ce6dmade stylistic changes based on comments by tscrim
8b05a7cmade various stylistic changes based on comments by tscrim

comment:58 Changed 2 years ago by git

  • Commit changed from 8b05a7c9cc4106a132aef2890f394a75b8878449 to 4e7916e8def9fa1e480c9230d46a838db9d85fca

Branch pushed to git repo; I updated commit sha1. New commits:

4e7916echanged dictionary input to follow PEP8

comment:59 follow-up: Changed 2 years ago by benstrasser

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 the set_frozen and remove_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:

c45ce6dmade stylistic changes based on comments by tscrim
8b05a7cmade various stylistic changes based on comments by tscrim
4e7916echanged dictionary input to follow PEP8

comment:60 Changed 2 years ago by benstrasser

  • Status changed from needs_work to needs_review

comment:61 Changed 2 years ago by benstrasser

I just changed it. Thanks for the input!

comment:62 in reply to: ↑ 59 Changed 2 years ago by tscrim

  • Branch changed from u/benstrasser/19339 to u/tscrim/cluster_diagraph_labels-19339
  • 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:

f093c68Merge branch 'u/benstrasser/19339' of git://trac.sagemath.org/sage into u/tscrim/cluster_diagraph_labels-19339
d42e9ddSome additional cleanup of the doc and PEP8.

comment:63 Changed 2 years ago by git

  • Commit changed from d42e9dd6120ce504a5f4f28eed82c6eb7bc9ade6 to a66a6a5f15995f22af32be5bab12ae228b1daa04

Branch pushed to git repo; I updated commit sha1. New commits:

a66a6a5Some little more cleanup and whitespace removal.

comment:64 Changed 2 years ago by gmoose05

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 user-defined 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 benstrasser

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 git

  • Commit changed from a66a6a5f15995f22af32be5bab12ae228b1daa04 to 5d8224ce89f4c81b35fa252ff91917c5277fa00f

Branch pushed to git repo; I updated commit sha1. New commits:

5d8224cFixing doctest and docbuild.

comment:67 Changed 2 years ago by tscrim

  • 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 gmoose05

  • 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 vbraun

  • Branch changed from u/tscrim/cluster_diagraph_labels-19339 to 5d8224ce89f4c81b35fa252ff91917c5277fa00f
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:70 Changed 17 months ago by jdemeyer

  • 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.

Note: See TracTickets for help on using tickets.