Opened 3 years ago

Closed 3 years ago

Fix few bugs in ClusterAlgebra discovered while working on #26771

Reported by: Owned by: etn40ff major sage-8.9 algebra ClusterAlgebras tscrim, gmoose05, stumpc5, gh-EBanaian, drupel, gh-kelleye Salvatore Stella Elizabeth Kelley N/A 6d7ce8d 6d7ce8d1b8408c96cfe9c14af796a4899c49b5eb

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 give `p24` or, at the very least *not* `p13`. Getting `p24` 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 F-polynomials 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 g-vectors 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.

comment:1 Changed 3 years ago by etn40ff

• Branch set to u/etn40ff/28176
• Cc tscrim gmoose05 stumpc5 gh-EBanaian drupel gh-kelleye added
• Commit set to d3cd59d86bf16c8414d36cdf5d95d2c5abd7d07f
• Component changed from PLEASE CHANGE to algebra
• Status changed from new to needs_review

New commits:

 ​d3cd59d `Fix 28176`

comment:2 Changed 3 years ago by chapoton

bot is not happy:

```sage -t --long src/sage/algebras/cluster_algebra.py  # Tab character found
```

comment:3 Changed 3 years ago by etn40ff

• Description modified (diff)

Oops. Fixed, sorry Frédéric!

With the occasion I also got some dramatic speedup in `mutate_initial` by not mutating F-polynomials a quadratic number of times.

Also I changed a division into a polynomial division in `_mutated_F`; this enforces F-polynomials to live in the correct ambient and yields a decent speedup in `mutate`.

comment:4 Changed 3 years ago by git

• Commit changed from d3cd59d86bf16c8414d36cdf5d95d2c5abd7d07f to 33f4fdd891df6f513ff28406e69f64fd66e96dde

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

 ​9fc136c `Fix 28176` ​33f4fdd `Fixed tabs and significant speedup`

comment:5 follow-up: ↓ 7 Changed 3 years ago by chapoton

doc does not build

```+[dochtml] OSError: /home/chapoton/sage/local/lib/python2.7/site-packages/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 'doc-html' failed
```
Version 0, edited 3 years ago by chapoton (next)

comment:6 Changed 3 years ago by git

• 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 ; follow-up: ↓ 8 Changed 3 years ago by etn40ff

doc does not build

```+[dochtml] OSError: /home/chapoton/sage/local/lib/python2.7/site-packages/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 3 years ago by tscrim

doc does not build

```+[dochtml] OSError: /home/chapoton/sage/local/lib/python2.7/site-packages/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 3 years ago by gmoose05

• Branch changed from u/etn40ff/28176 to u/gmoose05/28176

comment:10 Changed 3 years ago by git

• 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 3 years ago by gh-kelleye

It would be helpful to have a comment explaining a little bit about how lines 1285 - 1287 work; without explanation, understanding it takes some non-trivial thought.

comment:12 Changed 3 years ago by etn40ff

• Branch changed from u/gmoose05/28176 to public/28176
• Commit changed from dccaa2c92a02a9435751c7696ca81b0dd42c2254 to 6d7ce8d1b8408c96cfe9c14af796a4899c49b5eb

Done

New commits:

 ​6d7ce8d `Comments as requested`

comment:13 Changed 3 years ago by gh-kelleye

• Reviewers set to gh-kelleye
• Status changed from needs_review to positive_review

All the changes (and additional comments, thank you!) look good to me.

comment:14 Changed 3 years ago by gh-kelleye

• Reviewers changed from gh-kelleye to Elizabeth Kelley

comment:15 Changed 3 years ago by vbraun

• Branch changed from public/28176 to 6d7ce8d1b8408c96cfe9c14af796a4899c49b5eb
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.