Opened 8 years ago
Closed 7 years ago
#18594 closed enhancement (fixed)
Add additional mutation options for clusters
Reported by:  Aram Dermenjian  Owned by:  

Priority:  major  Milestone:  sage6.9 
Component:  combinatorics  Keywords:  SageDays64.5, clusters, mutation, seeds, quiver 
Cc:  Emily Gunawan, Salvatore Stella  Merged in:  
Authors:  Aram Dermenjian, Gregg Musiker  Reviewers:  Viviane Pons, Christian Stump 
Report Upstream:  N/A  Work issues:  
Branch:  f257d82 (Commits, GitHub, GitLab)  Commit:  f257d8224aa79c85a84ced0e69f378fafedbe546 
Dependencies:  Stopgaps: 
Description (last modified by )
Adding additional mutation options to ClusterSeed? and ClusterQuiver?. These options include:
 mutating by sources and sinks
 mutating by green and red vertices
 mutating by urban renewals
Additionally, we add a mutation analysis which goes through and returns basic properties of all mutations possible from the current state.
Update functionality to cluster seed in order to:
 Use cvectors for faster calculations of mutations
 Update c, d and g vectors to use faster algorithms
 Add mutation tracking
 Allow relabelling of vertices in graphs
 Allow searching for green vertices
Change History (43)
comment:1 Changed 8 years ago by
Authors:  → aram.dermenjian 

Description:  modified (diff) 
Keywords:  SageDays64.5 clusters mutation seeds quiver added 
comment:2 Changed 8 years ago by
Component:  PLEASE CHANGE → combinatorics 

Priority:  major → trivial 
Type:  PLEASE CHANGE → enhancement 
comment:3 Changed 8 years ago by
Branch:  → u/aram.dermenjian/add_additional_mutation_options_for_clusters 

comment:4 Changed 8 years ago by
Commit:  → 4e995e6bbbff0132c27f530911e777736e23ef18 

comment:5 Changed 8 years ago by
Description:  modified (diff) 

comment:6 Changed 8 years ago by
Status:  new → needs_review 

comment:7 Changed 8 years ago by
Authors:  aram.dermenjian → Aram Dermenjian 

Branch:  u/aram.dermenjian/add_additional_mutation_options_for_clusters → u/VivianePons/add_additional_mutation_options_for_clusters 
Commit:  4e995e6bbbff0132c27f530911e777736e23ef18 
Reviewers:  → Viviane Pons 
Status:  needs_review → needs_work 
I have looked at the documentation and fixed a few things. Here are some Todos:
 Please fill the output section of mutation_analysis
 Please document the new behavior of the mutate method of a quiver (accepts string) and add examples and tests
After this is done, I can finish reviewing the documentation. I would still need someone who knows more about quivers and cluster algebras to check the mathematical content of the methods (especially mutation_analysis in cluster_seed.py and number_of_edges in quiver.py).
comment:8 Changed 8 years ago by
Branch:  u/VivianePons/add_additional_mutation_options_for_clusters → u/VivianePons/add_additional_mutation_option_for_clusters 

Commit:  → 65cad5a8b9b8eb1ce3b38aef4c757ec2878b4d17 
comment:9 Changed 8 years ago by
Branch:  u/VivianePons/add_additional_mutation_option_for_clusters → u/aram.dermenjian/add_additional_mutation_option_for_clusters 

comment:10 Changed 8 years ago by
Commit:  65cad5a8b9b8eb1ce3b38aef4c757ec2878b4d17 → 2ec2bf56c04da255036bf7dc0cd3f2431c5e558d 

Branch pushed to git repo; I updated commit sha1. New commits:
2ec2bf5  Adding smallest c vector test

comment:11 Changed 8 years ago by
Dependencies:  → 18615 

comment:12 Changed 8 years ago by
Commit:  2ec2bf56c04da255036bf7dc0cd3f2431c5e558d → 1de24284943ff0f1153e84e3b1a558f7ac89725a 

Branch pushed to git repo; I updated commit sha1. New commits:
9a2a564  Add initial decreased mutation function

694dce2  Initial alterations to cluster seed adding new vectors and matrices

fe5ccf4  Add getters and setters

3a4cd7c  Merge branch 'cluster' into t/18615/updates_to_c_d__and_g_vectors__f_polynomials__and_mutations

81798ef  Merging with 18615

03fe348  Incorporate new cluster seed structure

1de2428  Fix bugs and add additional functionality

comment:13 Changed 8 years ago by
Commit:  1de24284943ff0f1153e84e3b1a558f7ac89725a → 4af3e418f58f734f4db4a3aed7d494077769ee4e 

Branch pushed to git repo; I updated commit sha1. New commits:
4af3e41  Add user labels

comment:14 Changed 8 years ago by
Description:  modified (diff) 

Priority:  trivial → major 
comment:15 Changed 8 years ago by
Cc:  Emily Gunawan added 

comment:16 Changed 8 years ago by
Commit:  4af3e418f58f734f4db4a3aed7d494077769ee4e → 3e0724dcd9570082808d41b692caa6981f935335 

Branch pushed to git repo; I updated commit sha1. New commits:
3e0724d  Adding Florians suggestions for speeding up green and red vertice grabbing

comment:17 Changed 8 years ago by
Commit:  3e0724dcd9570082808d41b692caa6981f935335 → da8d3865149e82473e6c57a74db95a476c252143 

Branch pushed to git repo; I updated commit sha1. New commits:
da8d386  Adding documentation and tests

comment:18 Changed 8 years ago by
Commit:  da8d3865149e82473e6c57a74db95a476c252143 → 69c8497fed8bf86f739ad2bdbfab2e693d47c9ef 

Branch pushed to git repo; I updated commit sha1. New commits:
69c8497  Various corrections, bug fixes, and documentation additions

comment:19 Changed 8 years ago by
Status:  needs_work → needs_review 

comment:20 Changed 8 years ago by
Dependencies:  18615 

Hi,
here are a few first comments quickly glancing through the file, no proper review:
 please update the author field here appropriately (using full names)
 update #18615 to duplicate / needs review.
 update the authors according to http://doc.sagemath.org/html/en/developer/coding_basics.html#headingsofsagelibrarycodefiles
 update the doc strings according to http://doc.sagemath.org/html/en/developer/coding_basics.html#documentationstrings
_sanitize_init_vars
is not documented. In general, all methods, even internals, needs proper docstrings and doctests. For that, please runsage coverage <file>
to see the coverage of methods there.
Cheers, Christian
comment:21 Changed 8 years ago by
Commit:  69c8497fed8bf86f739ad2bdbfab2e693d47c9ef → fd588d4d2e8d45b0335ed196dc8b496397d42656 

Branch pushed to git repo; I updated commit sha1. New commits:
fd588d4  Fixing documentation problems

comment:22 Changed 8 years ago by
All the fields have been updated as you wanted. For the author field, I didn't know when you and Gregg did the original version and so I didn't put a date for that and instead wrote initial version. If you want, I can go ahead and add it if you provide the date? (Or Gregg provides the date)
comment:23 Changed 8 years ago by
Reviewers:  Viviane Pons → Viviane Pons, Christian Stump 

Some more comments:
 Looking at the diff's
git diff develop src/sage/combinat/cluster_algebra_quiver/quiver.py
I see that you introduce tons of trailing whitespaces (both in
quiver.py
and incluster_seed.py
)
quiver.py
has changed without acknowledgements in the author fields.
 all tests pass, great!
 100% coverage, great!
 How do you ensure that
first_sink
andfirst_source
actually find the *first*, and what is the first (since the vertices are not necessarily labelled by something in a total order)? Do you mean to usea_sink
anda_source
?
I don't have the time these days to actually play with the code, but I hope (and expect) that you provided well explained and good doctests! (That at least seems to be the case in the few examples I looked at.) So from my side, this is ready to go.
On the other hand, I would like to have Salvatore and/or Dylan to also look at it again, since they will be modifying the mutation methods afterwards...
Cheers, Christian
comment:24 Changed 8 years ago by
Okay, Salvatore and Dylan had their chance. Once you treated my suggestions above, I will give it a positive review.
comment:25 Changed 8 years ago by
@Christian I just got to this after dealing with some other stuff. Are you in a hurry to give a positive review or do I still have some time? It will take me few days to go over all the changes being proposed S.
comment:26 Changed 8 years ago by
Cc:  Salvatore Stella added 

comment:27 Changed 8 years ago by
I am not in a hurry  it's only that Gregg and me are at the same conference, and Gregg would like it to be done soon...
comment:28 Changed 8 years ago by
I cant promise I'll be quick but I'll try my best. Shoud the two of you feel I am taking too long just drop me an email.
comment:29 Changed 8 years ago by
Hi Salvatore, take your time since I'm busy with the conference this week. But if possible I would like this ticket to be closed next week or so, so that others from the Sage Days will feel more free to post their tickets (since there might be lots of rebases otherwise).
Also several people said they'd be able to contribute more to sage once clusters can have any labels.
comment:30 Changed 8 years ago by
Additionally, I was wondering what the status of your ticket with Dylan for adding gvectors to cluster seed. We tried to design this ticket to play nicely with yours based on our discussions and code sharing at sage days.
Cheers, Gregg
comment:32 Changed 8 years ago by
Branch:  u/aram.dermenjian/add_additional_mutation_option_for_clusters → public/ticket/18594 

Commit:  fd588d4d2e8d45b0335ed196dc8b496397d42656 → fd9ed73d12e5a4dfc2a690da55ad8e5cd6c889ac 
comment:33 Changed 8 years ago by
Milestone:  sage6.8 → sage6.9 

comment:34 Changed 7 years ago by
Commit:  fd9ed73d12e5a4dfc2a690da55ad8e5cd6c889ac → 124befaba5674c924a20db27890fda3b0e332a82 

comment:35 Changed 7 years ago by
I just uploaded a new version:
1) the boolean flags that were at the end of init are now instead handled by the 'use' commands that were already coded.
This required other changes to the init command and examples accordingly.
2) Based on Salvatore Stella's feedback, the argument of set_c_matrix() has a new name and the command mutations() has been rewritten to give an error message instead of nothing when mutations aren't being tracked.
Let me know if there are other big changes that should be made before a positive review.
Gregg
comment:36 Changed 7 years ago by
Commit:  124befaba5674c924a20db27890fda3b0e332a82 → 39f1543c0c8d4e3dc9e20187804e17a46deac4b0 

comment:37 Changed 7 years ago by
Commit:  39f1543c0c8d4e3dc9e20187804e17a46deac4b0 → 4d90a7fd78836d74b0b671b8a53317b877eafa72 

Branch pushed to git repo; I updated commit sha1. New commits:
4d90a7f  doc cleanup

comment:38 Changed 7 years ago by
If everyone agrees that the discussed problems with the stuff implemented here can and will be fixed within finite time, I will give this ticket a positive review in order to enable the SageDays participants to submit further work.
comment:39 Changed 7 years ago by
I actually would also be in favour of adding a list of the raised issues concerning this tickets in order to have it written down for future references.
comment:40 Changed 7 years ago by
Commit:  4d90a7fd78836d74b0b671b8a53317b877eafa72 → f257d8224aa79c85a84ced0e69f378fafedbe546 

Branch pushed to git repo; I updated commit sha1. New commits:
f257d82  changed import numpy commands

comment:41 Changed 7 years ago by
For posterity, here is a list of concerns that were raised by Salvatore Stella and Christian Stump regarding the cluster_seed.py file.
These should be taken care of in a future ticket:
 There is a lot of duplicated information in the data structures for ClusterSeed?. For example, the mutable part of the B matrix is stored at least in _M, _B, _BC, and as part of the data structure of _quiver. Similarly cvectors are stored both in _BC and in _C.
NOTE: This is currently done to ensure backwards compatibility. In a future ticket warnings will be setup and then the old data structures will be phased out.
 In the initialization code there is a jungle of if statements. For example the lines in the init command where self._G is defined could instead be
try:
self._G = data.g_matrix()
except:
pass
while data.g_matrix() should take care of failing gracefully when the gmatrix can't be computed.
 Did you test how much slower and how much more spacedemanding it is, for example, to keep track of cmatrices and gmatrices vs not keeping track of them? If the difference is not noticeable (and we think it will not be) it may be just better to always keep track of them, simplify the code and spare the user few boolean flags.
NOTE: To be able to test whether or not this would speed up computations or free up space, this ticket implemented boolean flags as internal data that allow the user to keep track of less data as one mutates. After this is successfully merged into sage, we will keep track of whether this improves users' flexibility and computational speed. If it does not, we can revert to a version with less control and less overhead where all data is kept track of as we mutate.
 Similarly the various use_* functions (and few other bits and pieces) should never allow for inconsistent data. On the one hand it would not be mathematically correct, on the other it saves a lot of coding time and execution time because one can assume that the data structure is consistent and avoid all the checks that are repeatedly done right now.
NOTE: This is currently done to ensure backwards compatibility with commands like reset_cluster and set_cluster. In these commands, the user can set cluster variables directly but in the new improved data structure, sage keeps track of gvectors and Fpolynomials instead. This is computationally better because Sage works faster with polynomials than rational functions. But at the moment, it would be difficult for the user to set Fpolynomials directly or to have sage parse a userchosen cluster into Fpolynomials and gvectors.
NOTE: A future ticket by Salvatore Stella and Dylan Rupel will work directly with gvectors and Fpolynomials and would allow this parsing to occur.
 Is _sanitize_init_vars the same as (or similar to) sage.structure.parent.normalize_names ? If so it might be better to remove redundant code.
NOTE: I checked this, and I do not believe they are since _sanitize_init_vars would also allow more complicated names like allowing [1,2,5] as a variable name which would be common in cluster algebra theory where Plucker coordinates are cluster variables and have such names.
 cluster_index should take as input a cluster variable not a string and it should actually raise an error if the input can't be cast to the right type automatically.
Gregg
comment:42 Changed 7 years ago by
Authors:  Aram Dermenjian → Aram Dermenjian, Gregg Musiker 

Status:  needs_review → positive_review 
We gonna move forward!
comment:43 Changed 7 years ago by
Branch:  public/ticket/18594 → f257d8224aa79c85a84ced0e69f378fafedbe546 

Resolution:  → fixed 
Status:  positive_review → closed 
Branch pushed to git repo; I updated commit sha1. New commits:
Adding urban renewals to the list of mutation options
Adding a couple more options
Adding mutation analysis