Opened 5 years ago
Closed 4 years ago
#21254 closed enhancement (fixed)
Implement cluster algebras using the ParentElement framework
Reported by:  etn40ff  Owned by:  etn40ff 

Priority:  major  Milestone:  sage7.6 
Component:  algebra  Keywords:  cluster algebras, SageDays64.5, days79 
Cc:  gmoose05, drupel, stumpc5, mdpressland@…, tscrim, egunawan  Merged in:  
Authors:  Dylan Rupel, Salvatore Stella  Reviewers:  Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  389cdc9 (Commits, GitHub, GitLab)  Commit:  389cdc925fb4ed8b438c2bece4f078cefe248d4a 
Dependencies:  #22160  Stopgaps: 
Description (last modified by )
This module uses structural results from "Cluster algebras IV" to implement cluster algebras as a subobject of LaurentPolynomialRing_generic
.
It provides a new class ClusterAlgebra
, and two new auxiliary classes ClusterAlgebraSeed
and ClusterAlgebraElement
.
Change History (80)
comment:1 Changed 5 years ago by
 Branch set to public/ticket/21254
 Cc gmoose05 drupel stumpc5 added
 Commit set to b76ba976cff6be70a31fd3ab187006e15cd176ea
 Dependencies set to #19538
 Description modified (diff)
 Owner changed from (none) to etn40ff
comment:2 Changed 5 years ago by
 Commit changed from b76ba976cff6be70a31fd3ab187006e15cd176ea to e2810612d1c37b7975b49bd85c6f1d51bff9316a
comment:3 Changed 5 years ago by
 Commit changed from e2810612d1c37b7975b49bd85c6f1d51bff9316a to 2c6475623f43384b2975fbe098b7eab9496ea81a
Branch pushed to git repo; I updated commit sha1. New commits:
2c64756  Spaaaaaace (again)

comment:4 Changed 5 years ago by
 Commit changed from 2c6475623f43384b2975fbe098b7eab9496ea81a to e2723026b10881db8f0eeac2f5263b347615d677
Branch pushed to git repo; I updated commit sha1. New commits:
e272302  Catch only StopIteration

comment:5 Changed 5 years ago by
 Commit changed from e2723026b10881db8f0eeac2f5263b347615d677 to 673926d50370a03bccd826546c79974b3b2d651a
Branch pushed to git repo; I updated commit sha1. New commits:
673926d  Added hash function to ClusterAlgebraSeed

comment:6 Changed 5 years ago by
 Commit changed from 673926d50370a03bccd826546c79974b3b2d651a to 31a51a198df956038e08386fa14eee5e38fe15a5
Branch pushed to git repo; I updated commit sha1. New commits:
31a51a1  Figured out how to continue seeds after a KeyboardInterrupt

comment:7 Changed 5 years ago by
 Commit changed from 31a51a198df956038e08386fa14eee5e38fe15a5 to 13625a1ce644b0e249aa9c8604dbd188cbee6d5a
Branch pushed to git repo; I updated commit sha1. New commits:
13625a1  Rafactored all fuctions based on seeds()

comment:8 Changed 5 years ago by
I have just launched my own patchbot on the ticket.
There are some missing empty lines, for example here
+ EXAMPLES:: + sage: A = ClusterAlgebra(['B',2],principal_coefficients=True)
comment:9 Changed 5 years ago by
 Commit changed from 13625a1ce644b0e249aa9c8604dbd188cbee6d5a to 2e4ab20a6e707a39a870b83fa2ca7ffa8967b72b
comment:10 Changed 5 years ago by
 Commit changed from 2e4ab20a6e707a39a870b83fa2ca7ffa8967b72b to 3dd81b4be147b52f74379967e961ea7c8652aca8
Branch pushed to git repo; I updated commit sha1. New commits:
3dd81b4  Fixed missing empty lines

comment:11 Changed 5 years ago by
There are some missing empty lines, for example here
I just fixed that; thank you for catching it. Is there any automatic way to check that the code adhere to all the conventions? I only know of autopep8 but this does not test docstrings. (By the way we have plenty of long lines so we do not really adhere to pep8 either.)
On a different topic: there has been a discussion on sagedevel on the best way
to implement the exploring iterator. The discussion migrated to #21312 where the
possibility to use RecursivelyEnumeratedSet
has been explored. It turns out that
this can be done gaining a significant slowdown. I ended up writing the
current solution. A migration to RecursivelyEnumeratedSet
might be considered
for a followup ticket. I am just writing this here for the record.
comment:12 Changed 5 years ago by
 Commit changed from 3dd81b4be147b52f74379967e961ea7c8652aca8 to 8f0480a99a0c817c7e165f814fb44a3c6bb0c7e5
Branch pushed to git repo; I updated commit sha1. New commits:
8f0480a  Missing digit in doctest

comment:13 Changed 5 years ago by
 Commit changed from 8f0480a99a0c817c7e165f814fb44a3c6bb0c7e5 to 74006327ce8fd25264fe73bdd6f40a799cc7b60c
comment:14 Changed 5 years ago by
 Status changed from new to needs_review
Seems ready to go up for review
comment:15 Changed 5 years ago by
 Commit changed from 74006327ce8fd25264fe73bdd6f40a799cc7b60c to 14939e7bf90d76c80babc4faabbce3b92b9b6eb4
Branch pushed to git repo; I updated commit sha1. New commits:
14939e7  Fixed typos

comment:16 Changed 5 years ago by
 Commit changed from 14939e7bf90d76c80babc4faabbce3b92b9b6eb4 to f933020d91a9961a5dd02d0ee0fd7da27aae10e3
Branch pushed to git repo; I updated commit sha1. New commits:
f933020  100% coverage reached

comment:17 Changed 5 years ago by
 Commit changed from f933020d91a9961a5dd02d0ee0fd7da27aae10e3 to 5d270cad0da423ff37a4e3698cac475e8cdbe1be
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
97f22da  Rafactored all fuctions based on seeds()

18a09db  Better interrupt handling

dcf6d1f  Fixed missing :

34a11d4  Fixed missing empty lines

f3fb9cc  Missing digit in doctest

0702e07  Improved documentation of _div_

0277f86  Fixed typos

6298bb3  Changed introduction to avoit plugin failure

8217ed1  98.6% coverage reached. No idea on how to doctest _mutation_parse

5d270ca  100% coverage reached

comment:18 Changed 5 years ago by
 Commit changed from 5d270cad0da423ff37a4e3698cac475e8cdbe1be to 0ad8b4761409e5d296e8a68766d847f56544f39b
comment:19 Changed 5 years ago by
 typo: variariable
 please do not use xrange, but six.moves.range (py3)
 WARNING: should be .. WARNING::
 please do not put a dot at the end of the text of raise *Error("some text")
 pep8 cleanup suggestions:
1 E225 missing whitespace around operator 74 E226 missing whitespace around arithmetic operator 9 E228 missing whitespace around modulo operator 33 E231 missing whitespace after ',' 9 E302 expected 2 blank lines, found 1 3 W601 .has_key() is deprecated, use 'in'
ok, enough for today
EDIT another typo : usethe
EDIT: itareator
comment:20 Changed 5 years ago by
 Cc mdpressland@… added
comment:21 Changed 5 years ago by
 Cc tscrim egunawan added
comment:22 Changed 5 years ago by
 Commit changed from 0ad8b4761409e5d296e8a68766d847f56544f39b to 32383b152148e21a222fa8b94373170328527e6f
comment:23 Changed 5 years ago by
 Commit changed from 32383b152148e21a222fa8b94373170328527e6f to da62aeb841996b9497af0b3f06f69225f86726aa
Branch pushed to git repo; I updated commit sha1. New commits:
da62aeb  Pep8 fixes

comment:24 Changed 5 years ago by
thanks.
some more remarks:
 pep8 is for code, not for documentation. It was not necessary to add a space after all commas in the doc. You can use "pep8 ignore=E501 file.py" to check if there remains errors (and where).
 you should rather just do "from six.moves import range" and searchandreplace xrange by range. We are on the way to remove all xrange from sage (except in cython files)
 I am a bit puzzled by your strange manipulations of the doc (using
__doc__
) This is ugly code, imho.
 and I have the same bad feeling about your way to add methods only in special cases.. But maybe this is because I have never seen anything like that before
 a general remark on the doc is that after a single colon, you should not indent by 4, because the single colon is not defining a block.
 maybe not for now, but for the user who wants index starting at one, one may like to implement something like "index_set" (that exists for root systems, I think)
comment:25 Changed 5 years ago by
 Dependencies #19538 deleted
comment:26 Changed 5 years ago by
More comments:
 probably .rk should be .rank (we prefer explicit names)
+ (a1, a2) = d_vector
could be+ a1, a2 = d_vector
(and same thing in other places)
 remains at least one "usethe" and "itareator"
 in
+ g_mon = prod([self.ambient().gen(i)**g_vector[i] for i in xrange(self.rk())])
you do not need the square bracket inside the prod.
 typo "g_vetor"
comment:27 followup: ↓ 29 Changed 5 years ago by
After Frederic already started with a structural review, I finally take some time to play with the code.
Here is the list of points I want to mention concerning the documentation:
 The documentation start is too technical for my taste, containing many implementation details. I suggest to start with a brief description (or links) to what cluster algebras are + (even more importantly) followed by the later provided good examples. Only after, there should be a description of the algorithms used (I personally would even prefer to be pointed to the exact theorem) + the details of the implementation you give.
 some example sentences start with capital letters, some with small letters
 I agree with Frederic's "I have [...] bad feeling about your way to add methods only in special cases". Often, this is solved raising a
NotImplementedError
. On the other hand, I hate those errors and would often prefer if I rather not have a given method...
comment:28 Changed 5 years ago by
Concerning the usage:
 If I want to use
A.F_polynomials()
, I have to first explore and then get the Fpolys so far. I suggest to adddef F_polynomials()
as an iterator.
 Maybe I misunderstand the objects, but shouldn't there be
A.d_vectors()
(andA.c_vectors()
in the case of principal coefficients)?
Beside that, I did play with it for a while and it really seemed to behave nicely and intuitively!
comment:29 in reply to: ↑ 27 ; followup: ↓ 31 Changed 5 years ago by
Replying to stumpc5:
After Frederic already started with a structural review, I finally take some time to play with the code.
Here is the list of points I want to mention concerning the documentation:
 The documentation start is too technical for my taste, containing many implementation details. I suggest to start with a brief description (or links) to what cluster algebras are + (even more importantly) followed by the later provided good examples. Only after, there should be a description of the algorithms used (I personally would even prefer to be pointed to the exact theorem) + the details of the implementation you give.
I am okay with having the modulelevel documentation being somewhat technical. However, I strongly think the main bulk of the documentation should be at the classlevel for ClusterAlgebra
(with the __init__
only being a short 1line and TESTS::
with a (few) TestSuite(C).run()
calls). This way the user can get it by ClusterAlgebra?
.
 I agree with Frederic's "I have [...] bad feeling about your way to add methods only in special cases". Often, this is solved raising a
NotImplementedError
. On the other hand, I hate those errors and would often prefer if I rather not have a given method...
I would recommend using @abstract_method
(if optional to implement, you can do @abstract_method(optional=True)
). While it doesn't remove the method if it is not implemented, it does prevent the user from looking at its doc (and the TestSuite?` will fail if such a method is found).
comment:30 Changed 5 years ago by
Also the ALGORITHM:
should not be indented.
comment:31 in reply to: ↑ 29 Changed 5 years ago by
Replying to tscrim:
I am okay with having the modulelevel documentation being somewhat technical.
The problem then is that looking at the html documentation might scare users to further look.
comment:32 Changed 5 years ago by
 Commit changed from da62aeb841996b9497af0b3f06f69225f86726aa to 8c290be8819933ab781afcea3f445ee4f7108417
Branch pushed to git repo; I updated commit sha1. New commits:
888e004  Removed xrange

600b407  Spelling

291fc6e  PEP8 E265

25cc544  PEP8 E226

211124f  Fixed indentations within documentation

edf28dd  Changed rk() to rank()

677021e  Streamlined tuple assignments

71ee4b5  Removed useless []

fabb498  typo

8c290be  Reorganization + implementation of F_polynomials

comment:33 followup: ↓ 44 Changed 5 years ago by
Thanks everybody for the in depth comments and sorry for taking so long to reply.
I just updated the code implementing most of your suggestions. Here are some extra remarks:
 xrange has been removed
 several indentations has been fixed in the documentation
 implementing index_set is probably a good idea but I might prefer to get first a positive review on this. I already know how this should be implemented and it will not take much.
 as far as the manipulation of
__doc__
I am posting a request for comments on sagedevel to see what is the general consensus on how to best implement this.
 same holds for methods not always defined
 brackets inside
prod()
anddict()
has been removed
F_polynomials
is now implemented as an iterator
 I am not implementing cvectors at this stage because, as the code is structured right now, we do not keep track of them. A future patch will address the issue of storing and producing exchange relations; cvectors belong to that
d_vectors
: I did not implement it yet but it is easily doable. This raises a question concerningcluster_variables
andF_polynomials
. These, asd_vectors
would be, are simple syntactic sugar around amap
. For examplecluster_variables
is the same assage: map(A.cluster_variable, A.g_vectors())
is it worth implementing such methods or should we just get rid of them? Pros: less code to maintain! Cons: users might be scared?
Now to the more serious issue: I tried implementing Travis's suggestion about
documentation and I ran into some issues with TestSuite
that I can't wrap my
head around. It looks like instances of neither ClusterAlgebraElement
nor
ClusterAlgebraSees
can't be dumped with dumps
.
sage: A = ClusterAlgebra(['B',4]) sage: x = A.initial_cluster_variable(0) sage: S = A.current_seed() sage: from six.moves import cPickle sage: cPickle.dumps(x, protocol=2)  PicklingError Traceback (most recent call last) <ipythoninput778bafcb24ef1> in <module>() > 1 cPickle.dumps(x, protocol=Integer(2)) PicklingError: Can't pickle <type 'function'>: attribute lookup __builtin__.function failed sage: cPickle.dumps(S, protocol=2)  PicklingError Traceback (most recent call last) <ipythoninput877bd4c9405ba> in <module>() > 1 cPickle.dumps(S, protocol=Integer(2)) PicklingError: Can't pickle <type 'function'>: attribute lookup __builtin__.function failed
Where am I being stupid?
comment:34 Changed 5 years ago by
You don't have a pickling protocol implemented for the parent. Actually, I don't see why you aren't using UniqueRepresentation
. Also, instead of manually setting the methods, you should use a subclass and dynamically set the Element
of ClusterAlgebra
.
comment:35 followup: ↓ 36 Changed 5 years ago by
Thanks, I will move ClusterAlgebra
to UniqueRepresentation
and test if there are no undesired side effects. I am not sure myself why we did not do it earlier; we might have had reasons but I can't remember them right now.
Which methods are you referring to? Isn't enough to set Element = ClusterAlgebraElement
inside ClusterAlgebra
?
comment:36 in reply to: ↑ 35 Changed 5 years ago by
Replying to etn40ff:
Which methods are you referring to? Isn't enough to set
Element = ClusterAlgebraElement
insideClusterAlgebra
?
You can do it in the __init__
as long as you do it before you make the call to Parent.__init__
.
comment:37 Changed 5 years ago by
Ok now I remember why we ended up not using UniqueRepresentation
: in many (read "in every one I can think of") cases we want to be able to invoke ClusterAlgebra
on non hashable arguments.
comment:38 followup: ↓ 40 Changed 5 years ago by
Sorry I am particularly slow with understanding tonight.
Are you saying that instead of using MethodType
in the cases when I want them
I should define a subclass containing also the extra methods and dynamically
pick the right one in init?
I am not sure this will work in both cases: I can see how it will help for the
extra methods in ClusterAlgebraElement
but how do you make it work for the
extra methods in ClusterAlgebra
?
The only way I see is to introduce a factory but I would really prefer to
avoid doing that.
comment:39 Changed 5 years ago by
What would be an example? For matrices, we can make an immutable copy or some other equivalent (hashable) data. Same for quivers. (At least I would think ClusterAlgebraSeed
would fulfill this role.)
comment:40 in reply to: ↑ 38 Changed 5 years ago by
Replying to etn40ff:
Are you saying that instead of using
MethodType
in the cases when I want them I should define a subclass containing also the extra methods and dynamically pick the right one in init? I am not sure this will work in both cases: I can see how it will help for the extra methods inClusterAlgebraElement
but how do you make it work for the extra methods inClusterAlgebra
? The only way I see is to introduce a factory but I would really prefer to avoid doing that.
This approach is not very readable (subsequently harder to debug) and strongly breaks encapsulation. So I am strongly against it. Also, I believe you should be using UniqueRepresentation
, where the __classcall(_private)__
can be used as a factory. Although you can just add the ClasscallMetaclass
to get the same behavior to return the subclass.
comment:41 Changed 5 years ago by
OK I might have found a way to make everyone happy. What about something like this?
class MyClass(Parent): def __init__(self): # do something here pass if somecondition: def method_not_always_defines(self): # do something else here pass def method_always_defined(self): # do nothing pass
I do not know why I did not think of something of this sort before. The only thing I haven't figured out yet is whether it is possible for somecondition to be dependent on the argument with which MyClass
is called.
comment:42 Changed 5 years ago by
When I said non hashable I meant that they are not hashable on the nose but can be made hashable. Basically one would like to feed in anything that can be already interpreted by ClusterQuiver
that take also non hashable inputs.
comment:43 followup: ↓ 45 Changed 5 years ago by
Well the data you really want is ClusterQuiver
or ClusterSeed
, so you really should use UniqueRepresentation
as it solves a number of problems.
comment:41 is better, but I'm still 1. Use subclasses.
comment:44 in reply to: ↑ 33 Changed 5 years ago by
Replying to etn40ff:
d_vectors
: I did not implement it yet but it is easily doable. This raises a question concerningcluster_variables
andF_polynomials
. These, asd_vectors
would be, are simple syntactic sugar around amap
. For examplecluster_variables
is the same assage: map(A.cluster_variable, A.g_vectors())is it worth implementing such methods or should we just get rid of them? Pros: less code to maintain! Cons: users might be scared?
I prefer to look at the usage from, well, the user's perspective. And using
sage: map(A.cluster_variable, A.g_vectors())
implies that I have understood already how the implementation works (or, at least, that I have read through the documentation). I'd therefore prefer to have explicit methods providing all sorts of data attached to a cluster seed (independent from how you handle the seed internally).
Just my 2 cents, it's your code so it's your decision what to provide how...
comment:45 in reply to: ↑ 43 Changed 5 years ago by
Replying to tscrim:
Well the data you really want is
ClusterQuiver
orClusterSeed
, so you really should useUniqueRepresentation
as it solves a number of problems.comment:41 is better, but I'm still 1. Use subclasses.
I will give it a try with UniqueRepresentation
and report back when I am done.
I am unfortunately quite short on time for sage lately so this may take a while.
As for subclasses I am quite against the idea because I will, hopefully soon, add more methods defined only in certain cases and I would have to make a lot of classes just to mix and match all the possibilities. I posted a reply on sagedevel on this and I might just define the methods in any case and have them raise and error when they are meaningless. Unless of course someone suggests a better idea.
comment:46 followup: ↓ 47 Changed 5 years ago by
With the current code, you should use subclasses. In the future, for the more corner case situations, I would then raise a TypeError
when the particular method does not apply. I should have some time next week during the Sage Days to attack things if you'd want me to.
comment:47 in reply to: ↑ 46 Changed 4 years ago by
Replying to tscrim:
I should have some time next week during the Sage Days to attack things if you'd want me to.
I played a little with UniqueRepresentation
and there are some issues arising from mutate_initial
(and mutate
).
If your offer still holds I would be happy to talk any time this week except for Thursday.
comment:48 Changed 4 years ago by
 Commit changed from 8c290be8819933ab781afcea3f445ee4f7108417 to fbbb24fc3c5776bab32dfa8f678f1249506b8b1b
Branch pushed to git repo; I updated commit sha1. New commits:
56c21e0  Merge branch 'public/ticket/21254' of trac.sagemath.org:sage into public/ticket/21254

0cf61b7  Initial pass of documentation fixes and added TestSuite (to be fixed).

fbbb24f  Creating element class for cluster algebras with prinicpal coefficients.

comment:49 followup: ↓ 50 Changed 4 years ago by
I've used subclasses for the elements to begin with and done my pass over the documentation. I'm doing the subclasses for the parents next, which should be pushed in minute. Correction: That will likely depend on how the ClusterAlgebra
class changes with UniqueRepresentation
, which I can't really do until I understand the difference below.
I don't understand why we need the ClusterAlgebraSeed
when it seems like all of that functionality is (essentially) done by ClusterSeed
.
I would appreciate some time to Skype at some point today or tomorrow (I'm 1 hour ahead from Paris time I believe) to go over things. We can set that up over email.
comment:50 in reply to: ↑ 49 ; followup: ↓ 51 Changed 4 years ago by
Thanks for your work.
Replying to tscrim:
I would appreciate some time to Skype at some point today or tomorrow (I'm 1 hour ahead from Paris time I believe) to go over things. We can set that up over email.
Today would be great; I am currently queuing in some office but I should be available any time from 13 onwards (Paris time). My Skype username is etn40ff
Hold on for ClusterAlgebraSeed
I'll explain on the phone.
comment:51 in reply to: ↑ 50 ; followup: ↓ 52 Changed 4 years ago by
Replying to etn40ff:
hold on for
ClusterAlgebraSeed
I'll explain on the phone.
please post the transcript of the call here ;)
comment:52 in reply to: ↑ 51 ; followup: ↓ 53 Changed 4 years ago by
comment:53 in reply to: ↑ 52 ; followup: ↓ 54 Changed 4 years ago by
comment:54 in reply to: ↑ 53 Changed 4 years ago by
Replying to stumpc5:
Replying to etn40ff:
Replying to stumpc5:
Replying to etn40ff:
hold on for
ClusterAlgebraSeed
I'll explain on the phone.please post the transcript of the call here ;)
Might be hard, do you want to join?
i was just kidding, sorry.
I was not: constructive input is always welcome :)
I'll post a summary afterward
comment:55 Changed 4 years ago by
 Keywords days79 added
 Milestone changed from sage7.4 to sage7.5
Here is the short summary of our discussion:
 We will replace
initial_mutate
by something calledchange_initial_seed
(the name has not really been decided) to create a new cluster algebra using another seeds
as initial data and copy the old data into the new cluster algebra usings
(and its mutation sequence).
 We will remove the decorator and implement the
UniqueRepresentation
behavior forClusterAlgebra
.
 We will have the greedy element method as a method for all cluster algebras but raise a
NotImplementedError
for when it is not rank 2.
comment:56 Changed 4 years ago by
 Commit changed from fbbb24fc3c5776bab32dfa8f678f1249506b8b1b to 97d3890a154c39f0d4ccd0f1baa05bf82562918f
Branch pushed to git repo; I updated commit sha1. New commits:
97d3890  First attempt at using UniqueRepresentation, mutate_initial should now be broken

comment:57 Changed 4 years ago by
I started working on UniqueRepresentation
and I think the prototype is ok. I did not touch mutate_initial
yet.
One extremely weird issue: if you run
sage b sage t long src/sage/algebras/cluster_algebra.py sage t long src/sage/algebras/cluster_algebra.py
the first test will pass while the second will fail. It looks like the cache is not cleared in between tests. Ideas?
comment:58 Changed 4 years ago by
I cannot reproduce that behavior: I get 3 failures on every run.
comment:59 Changed 4 years ago by
 Commit changed from 97d3890a154c39f0d4ccd0f1baa05bf82562918f to 1bb0f903519c4771ed8663b5d0c051b9aa1a422e
Branch pushed to git repo; I updated commit sha1. New commits:
f3171f6  Second pass at using UniqueRepresentatio; still need to remove useless methods like __eq__

84b4942  Removed useless methods and got the code to fail with UniqeRepresentation

3356c33  Implemented new mutate_initial; this is now consistent with the use of UniqueRepresentation

1bb0f90  Removed old mutate_initial; removed mutate_wrapper; refactored mutate

comment:60 Changed 4 years ago by
Ok UniqueRepresentation
is now part of the code. Any comment?
One thing that bothers me is that we still can create different algebras that should be the same because the parsing of cluster_variable_names
and symilar is done in __init__
rather than in __classcall__
. Would there be drawbacks in moving these?
comment:61 Changed 4 years ago by
 Commit changed from 1bb0f903519c4771ed8663b5d0c051b9aa1a422e to 446619fc0ac79fc0e240df68a09255f3f328b341
Branch pushed to git repo; I updated commit sha1. New commits:
446619f  Polish + removed MethodType

comment:62 Changed 4 years ago by
 Commit changed from 446619fc0ac79fc0e240df68a09255f3f328b341 to 1d5bfc793a8564ee3fcea999e35e3262a159b51e
Branch pushed to git repo; I updated commit sha1. New commits:
872a4d0  Update to version 7.5.rc1. Merge branch 'public/ticket/21254' of git://trac.sagemath.org/sage into public/ticket/21254

5341cbd  21254: add titles and authors to references LLZ2014 and NZ2012, remove duplicate FZ2007

1d5bfc7  21254: add cluster algebras to /algebras/catalog.py

comment:63 Changed 4 years ago by
I did not double check my reference to [NZ2012]. I linked it to: https://arxiv.org/abs/1101.3736
comment:64 Changed 4 years ago by
 Dependencies set to #22160
Hi Emily,
I think the fix you made for the references was not the correct one: if I am not mistaken the goal is to go towards a unified reference system.
I created a new ticket to fix the duplication in ClusterSeed
#22160 (which needs a quick review) and I will shortly update a new version of this branch to reflect the changes.
Once this is done this ticket will depend on #22160.
comment:65 Changed 4 years ago by
 Commit changed from 1d5bfc793a8564ee3fcea999e35e3262a159b51e to a80a18450a2292348d76af7dbf18c04b752fd8f2
Branch pushed to git repo; I updated commit sha1. New commits:
a80a184  Revert 5341cbd219c650801030dd9df3dd49b34e3c6952

comment:66 Changed 4 years ago by
 Commit changed from a80a18450a2292348d76af7dbf18c04b752fd8f2 to f4417101137d2f6f65b9ec5b552d7b068b444d2a
Branch pushed to git repo; I updated commit sha1. New commits:
f441710  Removed blank line

comment:67 Changed 4 years ago by
+Missing doctests in algebras/cluster_algebra.py: 74 / 75 = 98%
comment:68 Changed 4 years ago by
 Commit changed from f4417101137d2f6f65b9ec5b552d7b068b444d2a to dbf42e6c5f4d3ef0c0dbac97d88829b47431a544
Branch pushed to git repo; I updated commit sha1. New commits:
dbf42e6  Added missing doctest

comment:69 Changed 4 years ago by
Thank you for catching this Frédéric. I hope I added the test in the right place.
comment:70 Changed 4 years ago by
Branch does no longer apply..
This has waited long enough..
If you rebase, I will run my bot, and give a positive review if some bot gives a green light and no other issue arise.
comment:71 Changed 4 years ago by
 Commit changed from dbf42e6c5f4d3ef0c0dbac97d88829b47431a544 to 06000a4f2e9a2a2b2bf6455add97c25dd486a798
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
6c2d7a2  First attempt at using UniqueRepresentation, mutate_initial should now be broken

130de19  Second pass at using UniqueRepresentatio; still need to remove useless methods like __eq__

66e75cf  Removed useless methods and got the code to fail with UniqeRepresentation

bcac0e9  Implemented new mutate_initial; this is now consistent with the use of UniqueRepresentation

fdc90e8  Removed old mutate_initial; removed mutate_wrapper; refactored mutate

41560ee  Polish + removed MethodType

5a827a1  21254: add titles and authors to references LLZ2014 and NZ2012, remove duplicate FZ2007

3981299  21254: add cluster algebras to /algebras/catalog.py

94a5d1a  Revert 5341cbd219c650801030dd9df3dd49b34e3c6952

06000a4  Added missing doctest

comment:72 Changed 4 years ago by
Thank you Frédéric. I just rebased to the current develop and now it builds cleanly on my machine. Tests seems ok so far. I am starting my patchbot to run overnight.
comment:73 Changed 4 years ago by
 Reviewers set to Frédéric Chapoton
 Status changed from needs_review to positive_review
ok, let it be
comment:74 Changed 4 years ago by
 Milestone changed from sage7.5 to sage7.6
comment:75 Changed 4 years ago by
 Status changed from positive_review to needs_work
On 32bit:
sage t long src/sage/algebras/cluster_algebra.py 1069********************************************************************** 1070File "src/sage/algebras/cluster_algebra.py", line 712, in sage.algebras.cluster_algebra.ClusterAlgebraSeed.__hash__ 1071Failed example: 1072 hash(S) 1073Expected: 1074 6108559638409052534 1075Got: 1076 1755906422 1077********************************************************************** 10781 item had failures: 1079 1 of 4 in sage.algebras.cluster_algebra.ClusterAlgebraSeed.__hash__ 1080 [402 tests, 1 failure, 11.03 s] 108
comment:76 Changed 4 years ago by
On 64bit it works. Which is the correct way to doctest hash functions independently of the architecture?
comment:77 Changed 4 years ago by
Copy and paste Volker's output with # 32bit
and # 64bit
. Although I prefer the softest that hash(x) == hash(x)
.
comment:78 Changed 4 years ago by
 Commit changed from 06000a4f2e9a2a2b2bf6455add97c25dd486a798 to 389cdc925fb4ed8b438c2bece4f078cefe248d4a
Branch pushed to git repo; I updated commit sha1. New commits:
389cdc9  Fixed hash doctest

comment:79 Changed 4 years ago by
 Status changed from needs_work to positive_review
Done, thanks. I am setting this back to positive review because I do not have a 32bit machine to test.
comment:80 Changed 4 years ago by
 Branch changed from public/ticket/21254 to 389cdc925fb4ed8b438c2bece4f078cefe248d4a
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Added warning to functions depending on _sd_iter