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: sage-6.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:

Status badges

Description (last modified by Aram Dermenjian)

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 c-vectors 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 Aram Dermenjian

Authors: aram.dermenjian
Description: modified (diff)
Keywords: SageDays64.5 clusters mutation seeds quiver added

comment:2 Changed 8 years ago by Aram Dermenjian

Component: PLEASE CHANGEcombinatorics
Priority: majortrivial
Type: PLEASE CHANGEenhancement

comment:3 Changed 8 years ago by Aram Dermenjian

Branch: u/aram.dermenjian/add_additional_mutation_options_for_clusters

comment:4 Changed 8 years ago by git

Commit: 4e995e6bbbff0132c27f530911e777736e23ef18

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

1c8464fAdding urban renewals to the list of mutation options
f8b9515Adding a couple more options
4e995e6Adding mutation analysis

comment:5 Changed 8 years ago by Aram Dermenjian

Description: modified (diff)

comment:6 Changed 8 years ago by Aram Dermenjian

Status: newneeds_review

comment:7 Changed 8 years ago by Viviane Pons

Authors: aram.dermenjianAram Dermenjian
Branch: u/aram.dermenjian/add_additional_mutation_options_for_clustersu/VivianePons/add_additional_mutation_options_for_clusters
Commit: 4e995e6bbbff0132c27f530911e777736e23ef18
Reviewers: Viviane Pons
Status: needs_reviewneeds_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 Viviane Pons

Branch: u/VivianePons/add_additional_mutation_options_for_clustersu/VivianePons/add_additional_mutation_option_for_clusters
Commit: 65cad5a8b9b8eb1ce3b38aef4c757ec2878b4d17

New commits:

9cf2696Add ability to mutate by source, sink, and green and red vertices
1c8464fAdding urban renewals to the list of mutation options
f8b9515Adding a couple more options
4e995e6Adding mutation analysis
65cad5aFix some documentation and formatting

comment:9 Changed 8 years ago by Aram Dermenjian

Branch: u/VivianePons/add_additional_mutation_option_for_clustersu/aram.dermenjian/add_additional_mutation_option_for_clusters

comment:10 Changed 8 years ago by git

Commit: 65cad5a8b9b8eb1ce3b38aef4c757ec2878b4d172ec2bf56c04da255036bf7dc0cd3f2431c5e558d

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

2ec2bf5Adding smallest c vector test

comment:11 Changed 8 years ago by Aram Dermenjian

Dependencies: 18615

comment:12 Changed 8 years ago by git

Commit: 2ec2bf56c04da255036bf7dc0cd3f2431c5e558d1de24284943ff0f1153e84e3b1a558f7ac89725a

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

9a2a564Add initial decreased mutation function
694dce2Initial alterations to cluster seed adding new vectors and matrices
fe5ccf4Add getters and setters
3a4cd7cMerge branch 'cluster' into t/18615/updates_to_c_d__and_g_vectors__f_polynomials__and_mutations
81798efMerging with 18615
03fe348Incorporate new cluster seed structure
1de2428Fix bugs and add additional functionality

comment:13 Changed 8 years ago by git

Commit: 1de24284943ff0f1153e84e3b1a558f7ac89725a4af3e418f58f734f4db4a3aed7d494077769ee4e

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

4af3e41Add user labels

comment:14 Changed 8 years ago by Aram Dermenjian

Description: modified (diff)
Priority: trivialmajor

comment:15 Changed 8 years ago by Emily Gunawan

Cc: Emily Gunawan added

comment:16 Changed 8 years ago by git

Commit: 4af3e418f58f734f4db4a3aed7d494077769ee4e3e0724dcd9570082808d41b692caa6981f935335

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

3e0724dAdding Florians suggestions for speeding up green and red vertice grabbing

comment:17 Changed 8 years ago by git

Commit: 3e0724dcd9570082808d41b692caa6981f935335da8d3865149e82473e6c57a74db95a476c252143

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

da8d386Adding documentation and tests

comment:18 Changed 8 years ago by git

Commit: da8d3865149e82473e6c57a74db95a476c25214369c8497fed8bf86f739ad2bdbfab2e693d47c9ef

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

69c8497Various corrections, bug fixes, and documentation additions

comment:19 Changed 8 years ago by Aram Dermenjian

Status: needs_workneeds_review

comment:20 Changed 8 years ago by Christian Stump

Dependencies: 18615

Hi,

here are a few first comments quickly glancing through the file, no proper review:

Cheers, Christian

comment:21 Changed 8 years ago by git

Commit: 69c8497fed8bf86f739ad2bdbfab2e693d47c9effd588d4d2e8d45b0335ed196dc8b496397d42656

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

fd588d4Fixing documentation problems

comment:22 Changed 8 years ago by Aram Dermenjian

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 Christian Stump

Reviewers: Viviane PonsViviane 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 in cluster_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 and first_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 use a_sink and a_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 Christian Stump

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 Salvatore Stella

@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 Salvatore Stella

Cc: Salvatore Stella added

comment:27 Changed 8 years ago by Christian Stump

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 Salvatore Stella

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 e-mail.

comment:29 Changed 8 years ago by Gregg Musiker

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 Gregg Musiker

Additionally, I was wondering what the status of your ticket with Dylan for adding g-vectors 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:31 Changed 8 years ago by Gregg Musiker

sorry not "adding g-vectors" but the functionalities you demoed

comment:32 Changed 8 years ago by Frédéric Chapoton

Branch: u/aram.dermenjian/add_additional_mutation_option_for_clusterspublic/ticket/18594
Commit: fd588d4d2e8d45b0335ed196dc8b496397d42656fd9ed73d12e5a4dfc2a690da55ad8e5cd6c889ac

i have corrected some problems in the doc


New commits:

52c552aMerge branch 'u/aram.dermenjian/add_additional_mutation_option_for_clusters' into 6.9.b0
fd9ed73trac #18594 some care for the doc

comment:33 Changed 8 years ago by Frédéric Chapoton

Milestone: sage-6.8sage-6.9

comment:34 Changed 7 years ago by git

Commit: fd9ed73d12e5a4dfc2a690da55ad8e5cd6c889ac124befaba5674c924a20db27890fda3b0e332a82

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

5e0b4c6use commands rather than in init
61141d0use commands rather than in init
124befaalso changed argument of set_c_matrix and changed outputs of mutations() command

comment:35 Changed 7 years ago by Gregg Musiker

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 git

Commit: 124befaba5674c924a20db27890fda3b0e332a8239f1543c0c8d4e3dc9e20187804e17a46deac4b0

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

9084d1dMerge branch 'public/ticket/18594' into 6.9.b3
39f1543trac #18594 fixing doc

comment:37 Changed 7 years ago by git

Commit: 39f1543c0c8d4e3dc9e20187804e17a46deac4b04d90a7fd78836d74b0b671b8a53317b877eafa72

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

4d90a7fdoc cleanup

comment:38 Changed 7 years ago by Christian Stump

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 Christian Stump

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 git

Commit: 4d90a7fd78836d74b0b671b8a53317b877eafa72f257d8224aa79c85a84ced0e69f378fafedbe546

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

f257d82changed import numpy commands

comment:41 Changed 7 years ago by Gregg Musiker

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 c-vectors are stored both in _BC and in _C.

NOTE: This is currently done to ensure backwards compatibility. In a future ticket warnings will be set-up 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 g-matrix can't be computed.

  • Did you test how much slower and how much more space-demanding it is, for example, to keep track of c-matrices and g-matrices 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 g-vectors and F-polynomials 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 F-polynomials directly or to have sage parse a user-chosen cluster into F-polynomials and g-vectors.

NOTE: A future ticket by Salvatore Stella and Dylan Rupel will work directly with g-vectors and F-polynomials 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 Christian Stump

Authors: Aram DermenjianAram Dermenjian, Gregg Musiker
Status: needs_reviewpositive_review

We gonna move forward!

comment:43 Changed 7 years ago by Volker Braun

Branch: public/ticket/18594f257d8224aa79c85a84ced0e69f378fafedbe546
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.