Add additional mutation options for clusters

thecaligarmo commented 9 years ago

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

CC: @egunawan @Etn40ff

Component: combinatorics

Keywords: SageDays64.5, clusters, mutation, seeds, quiver

Author: Aram Dermenjian, Gregg Musiker

Branch/Commit: f257d82

Reviewer: Viviane Pons, Christian Stump

Issue created by migration from https://trac.sagemath.org/ticket/18594

thecaligarmo commented 9 years ago

Description changed:

--- 
+++ 
@@ -1 +1,3 @@
-
+Adding additional mutation options to ClusterSeed and ClusterQuiver. These options include:
+- mutating by sources and sinks
+- mutating by green and red vertices

thecaligarmo commented 9 years ago

Author: aram.dermenjian

thecaligarmo commented 9 years ago

Changed keywords from none to SageDays64.5, clusters, mutation, seeds, quiver

thecaligarmo commented 9 years ago

Branch: u/aram.dermenjian/add_additional_mutation_options_for_clusters

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Commit: 4e995e6

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

`1c8464f`	`Adding urban renewals to the list of mutation options`
`f8b9515`	`Adding a couple more options`
`4e995e6`	`Adding mutation analysis`

thecaligarmo commented 9 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,6 @@
 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.

VivianePons commented 9 years ago

Changed branch from u/aram.dermenjian/add_additional_mutation_options_for_clusters to u/VivianePons/add_additional_mutation_options_for_clusters

VivianePons commented 9 years ago

Reviewer: Viviane Pons

VivianePons commented 9 years ago

Changed author from aram.dermenjian to Aram Dermenjian

VivianePons commented 9 years ago

comment:7

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

VivianePons commented 9 years ago

Changed commit from 4e995e6 to none

VivianePons commented 9 years ago

Commit: 65cad5a

VivianePons commented 9 years ago

Changed branch from u/VivianePons/add_additional_mutation_options_for_clusters to u/VivianePons/add_additional_mutation_option_for_clusters

VivianePons commented 9 years ago

New commits:

`9cf2696`	`Add ability to mutate by source, sink, and green and red vertices`
`1c8464f`	`Adding urban renewals to the list of mutation options`
`f8b9515`	`Adding a couple more options`
`4e995e6`	`Adding mutation analysis`
`65cad5a`	`Fix some documentation and formatting`

thecaligarmo commented 9 years ago

Changed branch from u/VivianePons/add_additional_mutation_option_for_clusters to u/aram.dermenjian/add_additional_mutation_option_for_clusters

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 65cad5a to 2ec2bf5

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

2ec2bf5 Adding smallest c vector test

thecaligarmo commented 9 years ago

Dependencies: 18615

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 2ec2bf5 to 1de2428

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

4af3e41 Add user labels

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 1de2428 to 4af3e41

thecaligarmo commented 9 years ago

Description changed:

--- 
+++ 
@@ -4,3 +4,11 @@
 - 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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 4af3e41 to 3e0724d

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

3e0724d Adding Florians suggestions for speeding up green and red vertice grabbing

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

da8d386 Adding documentation and tests

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 3e0724d to da8d386

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

69c8497 Various corrections, bug fixes, and documentation additions

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from da8d386 to 69c8497

stumpc5 commented 9 years ago

Changed dependencies from 18615 to none

stumpc5 commented 9 years ago

comment:20

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#headings-of-sage-library-code-files
update the doc strings according to http://doc.sagemath.org/html/en/developer/coding_basics.html#documentation-strings
_sanitize_init_vars is not documented. In general, all methods, even internals, needs proper docstrings and doctests. For that, please run sage --coverage <file> to see the coverage of methods there.

Cheers, Christian

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 69c8497 to fd588d4

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

fd588d4 Fixing documentation problems

thecaligarmo commented 9 years ago

comment:22

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)

stumpc5 commented 9 years ago

Changed reviewer from Viviane Pons to Viviane Pons, Christian Stump

stumpc5 commented 9 years ago

comment:23

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

stumpc5 commented 9 years ago

comment:24

Okay, Salvatore and Dylan had their chance. Once you treated my suggestions above, I will give it a positive review.

Etn40ff commented 9 years ago

comment:25

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

stumpc5 commented 9 years ago

comment:27

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

Etn40ff commented 9 years ago

comment:28

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.

a9a57cdf-9183-46c4-a1f6-41c1b5271dce commented 9 years ago

comment:29

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.

a9a57cdf-9183-46c4-a1f6-41c1b5271dce commented 9 years ago

comment:30

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

a9a57cdf-9183-46c4-a1f6-41c1b5271dce commented 9 years ago

comment:31

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

fchapoton commented 9 years ago

Changed branch from u/aram.dermenjian/add_additional_mutation_option_for_clusters to public/ticket/18594

fchapoton commented 9 years ago

comment:32

i have corrected some problems in the doc

New commits:

`52c552a`	`Merge branch 'u/aram.dermenjian/add_additional_mutation_option_for_clusters' into 6.9.b0`
`fd9ed73`	`trac #18594 some care for the doc`

fchapoton commented 9 years ago

Changed commit from fd588d4 to fd9ed73

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from fd9ed73 to 124befa

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

`5e0b4c6`	`use commands rather than in init`
`61141d0`	`use commands rather than in init`
`124befa`	`also changed argument of set_c_matrix and changed outputs of mutations() command`

a9a57cdf-9183-46c4-a1f6-41c1b5271dce commented 9 years ago

comment:35

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

sagemath / sage

Add additional mutation options for clusters #18594