Opened 2 years ago
Closed 2 years ago
#28176 closed defect (fixed)
Fix few bugs in ClusterAlgebra discovered while working on #26771
Reported by:  etn40ff  Owned by:  

Priority:  major  Milestone:  sage8.9 
Component:  algebra  Keywords:  ClusterAlgebras 
Cc:  tscrim, gmoose05, stumpc5, ghEBanaian, drupel, ghkelleye  Merged in:  
Authors:  Salvatore Stella  Reviewers:  Elizabeth Kelley 
Report Upstream:  N/A  Work issues:  
Branch:  6d7ce8d (Commits, GitHub, GitLab)  Commit:  6d7ce8d1b8408c96cfe9c14af796a4899c49b5eb 
Dependencies:  Stopgaps: 
Description (last modified by )
As pointed out by the people at #26771, ClusterAlgebra
has few bugs.
 Default arguments do not play nicely with
UniqueRepresentation
sage: A1 = ClusterAlgebra(['A',2]) sage: A2 = ClusterAlgebra(['A',2], cluster_variable_prefix='x') sage: A1 is A2 False
mutate_initial
keeps the same variable names; In theory this is not a problem but may create confusion. For example suppose we are looking at the Grassmannian of planes in a 4 dimensional vector space. Then the associate cluster algebra is
sage: A = ClusterAlgebra( matrix(5,[0,1,1,1,1]), cluster_variable_names=['p13'], coefficient_names=['p12','p23','p34','p41'], scalars=QQbar); A A Cluster Algebra with cluster variable p13 and coefficients p12, p23, p34, p41 over Algebraic Field
Swapping out
p13
from the initial seed should givep24
or, at the very least *not*p13
. Gettingp24
requires some understanding of what the ring in question really is that goes beyond the scope of this class. Currently we get
sage: A.mutate_initial(0) A Cluster Algebra with cluster variable p13 and coefficients p12, p23, p34, p41 over Algebraic Field
mutate_initial
does not compute all the required Fpolynomials and it does not enforce the fact that they are not rational expressions
sage: A1 = ClusterAlgebra(['A',[2,1],1]) sage: A2 = A1.mutate_initial([0,1,0]) sage: len(A2.g_vectors_so_far()) == len(A2.F_polynomials_so_far()) False sage: all(parent(f) == A2._U for f in A2.F_polynomials_so_far()) False
 finally
mutate_initial
does not enforce the fact that initial gvectors belong to the initial cluster but rather prepend the performed mutation sequence to all the paths already known.
sage: A2.find_g_vector((0,0,1)) [0, 1, 0]
This, while technically not false, may generate confusion.
_mutated_F
is doing/
rather than//
resulting in slower computations and parent issues.
This ticket deals with all these issues.
Change History (15)
comment:1 Changed 2 years ago by
 Branch set to u/etn40ff/28176
 Cc tscrim gmoose05 stumpc5 ghEBanaian drupel ghkelleye added
 Commit set to d3cd59d86bf16c8414d36cdf5d95d2c5abd7d07f
 Component changed from PLEASE CHANGE to algebra
 Status changed from new to needs_review
comment:2 Changed 2 years ago by
bot is not happy:
sage t long src/sage/algebras/cluster_algebra.py # Tab character found
comment:3 Changed 2 years ago by
 Description modified (diff)
Oops. Fixed, sorry Frédéric!
With the occasion I also got some dramatic speedup in mutate_initial
by not
mutating Fpolynomials a quadratic number of times.
Also I changed a division into a polynomial division in _mutated_F
; this
enforces Fpolynomials to live in the correct ambient and yields a decent
speedup in mutate
.
comment:4 Changed 2 years ago by
 Commit changed from d3cd59d86bf16c8414d36cdf5d95d2c5abd7d07f to 33f4fdd891df6f513ff28406e69f64fd66e96dde
comment:5 followup: ↓ 7 Changed 2 years ago by
doc does not build
+[dochtml] OSError: /home/chapoton/sage/local/lib/python2.7/sitepackages/sage/algebras/cluster_algebra.py:docstring of sage.algebras.cluster_algebra.ClusterAlgebra.mutate_initial:4: WARNING: Content block expected for the "WARNING" directive; none found. +Makefile:2036: recipe for target 'dochtml' failed
and missing :: here
+ A faster example without recomputing Fpolynomials
comment:6 Changed 2 years ago by
 Commit changed from 33f4fdd891df6f513ff28406e69f64fd66e96dde to 0b22eb7ba0443036aef930c61c3356c5e3d8e48a
Branch pushed to git repo; I updated commit sha1. New commits:
0b22eb7  Doctest

comment:7 in reply to: ↑ 5 ; followup: ↓ 8 Changed 2 years ago by
Replying to chapoton:
doc does not build
+[dochtml] OSError: /home/chapoton/sage/local/lib/python2.7/sitepackages/sage/algebras/cluster_algebra.py:docstring of sage.algebras.cluster_algebra.ClusterAlgebra.mutate_initial:4: WARNING: Content block expected for the "WARNING" directive; none found.
I have never seen this before, is it just a missing indentation? In case it is fixed now.
comment:8 in reply to: ↑ 7 Changed 2 years ago by
Replying to etn40ff:
Replying to chapoton:
doc does not build
+[dochtml] OSError: /home/chapoton/sage/local/lib/python2.7/sitepackages/sage/algebras/cluster_algebra.py:docstring of sage.algebras.cluster_algebra.ClusterAlgebra.mutate_initial:4: WARNING: Content block expected for the "WARNING" directive; none found.I have never seen this before, is it just a missing indentation? In case it is fixed now.
Yes, that is correct.
comment:9 Changed 2 years ago by
 Branch changed from u/etn40ff/28176 to u/gmoose05/28176
comment:10 Changed 2 years ago by
 Commit changed from 0b22eb7ba0443036aef930c61c3356c5e3d8e48a to dccaa2c92a02a9435751c7696ca81b0dd42c2254
Branch pushed to git repo; I updated commit sha1. New commits:
dccaa2c  B to B0 in class_call and added a few doctests

comment:11 Changed 2 years ago by
It would be helpful to have a comment explaining a little bit about how lines 1285  1287 work; without explanation, understanding it takes some nontrivial thought.
comment:12 Changed 2 years ago by
 Branch changed from u/gmoose05/28176 to public/28176
 Commit changed from dccaa2c92a02a9435751c7696ca81b0dd42c2254 to 6d7ce8d1b8408c96cfe9c14af796a4899c49b5eb
comment:13 Changed 2 years ago by
 Reviewers set to ghkelleye
 Status changed from needs_review to positive_review
All the changes (and additional comments, thank you!) look good to me.
comment:14 Changed 2 years ago by
 Reviewers changed from ghkelleye to Elizabeth Kelley
comment:15 Changed 2 years ago by
 Branch changed from public/28176 to 6d7ce8d1b8408c96cfe9c14af796a4899c49b5eb
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Fix 28176