Opened 5 years ago

Closed 4 years ago

#21254 closed enhancement (fixed)

Implement cluster algebras using the Parent-Element framework

Reported by: etn40ff Owned by: etn40ff
Priority: major Milestone: sage-7.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:

Status badges

Description (last modified by etn40ff)

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 etn40ff

  • 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 git

  • Commit changed from b76ba976cff6be70a31fd3ab187006e15cd176ea to e2810612d1c37b7975b49bd85c6f1d51bff9316a

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

e281061Added warning to functions depending on _sd_iter

comment:3 Changed 5 years ago by git

  • Commit changed from e2810612d1c37b7975b49bd85c6f1d51bff9316a to 2c6475623f43384b2975fbe098b7eab9496ea81a

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

2c64756Spaaaaaace (again)

comment:4 Changed 5 years ago by git

  • Commit changed from 2c6475623f43384b2975fbe098b7eab9496ea81a to e2723026b10881db8f0eeac2f5263b347615d677

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

e272302Catch only StopIteration

comment:5 Changed 5 years ago by git

  • Commit changed from e2723026b10881db8f0eeac2f5263b347615d677 to 673926d50370a03bccd826546c79974b3b2d651a

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

673926dAdded hash function to ClusterAlgebraSeed

comment:6 Changed 5 years ago by git

  • Commit changed from 673926d50370a03bccd826546c79974b3b2d651a to 31a51a198df956038e08386fa14eee5e38fe15a5

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

31a51a1Figured out how to continue seeds after a KeyboardInterrupt

comment:7 Changed 5 years ago by git

  • Commit changed from 31a51a198df956038e08386fa14eee5e38fe15a5 to 13625a1ce644b0e249aa9c8604dbd188cbee6d5a

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

13625a1Rafactored all fuctions based on seeds()

comment:8 Changed 5 years ago by chapoton

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 git

  • Commit changed from 13625a1ce644b0e249aa9c8604dbd188cbee6d5a to 2e4ab20a6e707a39a870b83fa2ca7ffa8967b72b

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

bb66742Better interrupt handling
2e4ab20Fixed missing :

comment:10 Changed 5 years ago by git

  • Commit changed from 2e4ab20a6e707a39a870b83fa2ca7ffa8967b72b to 3dd81b4be147b52f74379967e961ea7c8652aca8

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

3dd81b4Fixed missing empty lines

comment:11 Changed 5 years ago by etn40ff

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 sage-devel 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 follow-up ticket. I am just writing this here for the record.

Last edited 5 years ago by etn40ff (previous) (diff)

comment:12 Changed 5 years ago by git

  • Commit changed from 3dd81b4be147b52f74379967e961ea7c8652aca8 to 8f0480a99a0c817c7e165f814fb44a3c6bb0c7e5

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

8f0480aMissing digit in doctest

comment:13 Changed 5 years ago by git

  • Commit changed from 8f0480a99a0c817c7e165f814fb44a3c6bb0c7e5 to 74006327ce8fd25264fe73bdd6f40a799cc7b60c

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

4cba0a2Merge branch 'develop' into cluster_algebras
7400632Improved documentation of _div_

comment:14 Changed 5 years ago by etn40ff

  • Status changed from new to needs_review

Seems ready to go up for review

comment:15 Changed 5 years ago by git

  • Commit changed from 74006327ce8fd25264fe73bdd6f40a799cc7b60c to 14939e7bf90d76c80babc4faabbce3b92b9b6eb4

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

14939e7Fixed typos

comment:16 Changed 5 years ago by git

  • Commit changed from 14939e7bf90d76c80babc4faabbce3b92b9b6eb4 to f933020d91a9961a5dd02d0ee0fd7da27aae10e3

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

f933020100% coverage reached

comment:17 Changed 5 years ago by git

  • Commit changed from f933020d91a9961a5dd02d0ee0fd7da27aae10e3 to 5d270cad0da423ff37a4e3698cac475e8cdbe1be

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

97f22daRafactored all fuctions based on seeds()
18a09dbBetter interrupt handling
dcf6d1fFixed missing :
34a11d4Fixed missing empty lines
f3fb9ccMissing digit in doctest
0702e07Improved documentation of _div_
0277f86Fixed typos
6298bb3Changed introduction to avoit plugin failure
8217ed198.6% coverage reached. No idea on how to doctest _mutation_parse
5d270ca100% coverage reached

comment:18 Changed 5 years ago by git

  • Commit changed from 5d270cad0da423ff37a4e3698cac475e8cdbe1be to 0ad8b4761409e5d296e8a68766d847f56544f39b

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

67eb036Some fixes due to pylint
7bf638aUse python3 map
9847b8eAdded methods to produce iterators on g-vectors and cluster variables
0ad8b47Some fixes due to pylint

comment:19 Changed 5 years ago by chapoton

  • 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

Last edited 5 years ago by chapoton (previous) (diff)

comment:20 Changed 5 years ago by etn40ff

  • Cc mdpressland@… added

comment:21 Changed 5 years ago by tscrim

  • Cc tscrim egunawan added

comment:22 Changed 5 years ago by git

  • Commit changed from 0ad8b4761409e5d296e8a68766d847f56544f39b to 32383b152148e21a222fa8b94373170328527e6f

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

2ed6066Implemented some of the suggested changes
32383b1missing s

comment:23 Changed 5 years ago by git

  • Commit changed from 32383b152148e21a222fa8b94373170328527e6f to da62aeb841996b9497af0b3f06f69225f86726aa

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

da62aebPep8 fixes

comment:24 Changed 5 years ago by chapoton

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 search-and-replace 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)
Last edited 5 years ago by chapoton (previous) (diff)

comment:25 Changed 5 years ago by chapoton

  • Dependencies #19538 deleted

comment:26 Changed 5 years ago by chapoton

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"
Last edited 5 years ago by chapoton (previous) (diff)

comment:27 follow-up: Changed 5 years ago by 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.
  • 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...
Last edited 5 years ago by stumpc5 (previous) (diff)

comment:28 Changed 5 years ago by stumpc5

Concerning the usage:

  • If I want to use A.F_polynomials(), I have to first explore and then get the F-polys so far. I suggest to add def F_polynomials() as an iterator.
  • Maybe I misunderstand the objects, but shouldn't there be A.d_vectors() (and A.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 ; follow-up: Changed 5 years ago by tscrim

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 module-level documentation being somewhat technical. However, I strongly think the main bulk of the documentation should be at the class-level for ClusterAlgebra (with the __init__ only being a short 1-line 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 tscrim

Also the ALGORITHM: should not be indented.

comment:31 in reply to: ↑ 29 Changed 5 years ago by stumpc5

Replying to tscrim:

I am okay with having the module-level 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 git

  • Commit changed from da62aeb841996b9497af0b3f06f69225f86726aa to 8c290be8819933ab781afcea3f445ee4f7108417

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

888e004Removed xrange
600b407Spelling
291fc6ePEP8 E265
25cc544PEP8 E226
211124fFixed indentations within documentation
edf28ddChanged rk() to rank()
677021eStreamlined tuple assignments
71ee4b5Removed useless []
fabb498typo
8c290beReorganization + implementation of F_polynomials

comment:33 follow-up: Changed 5 years ago by etn40ff

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 sage-devel to see what is the general consensus on how to best implement this.
  • same holds for methods not always defined
  • brackets inside prod() and dict() has been removed
  • F_polynomials is now implemented as an iterator
  • I am not implementing c-vectors 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; c-vectors belong to that
  • d_vectors: I did not implement it yet but it is easily doable. This raises a question concerning cluster_variables and F_polynomials. These, as d_vectors would be, are simple syntactic sugar around a map. For example cluster_variables is the same as
    sage: 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)
<ipython-input-7-78bafcb24ef1> 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)
<ipython-input-8-77bd4c9405ba> 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 tscrim

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 follow-up: Changed 5 years ago by etn40ff

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 tscrim

Replying to etn40ff:

Which methods are you referring to? Isn't enough to set Element = ClusterAlgebraElement inside ClusterAlgebra?

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 etn40ff

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 follow-up: Changed 5 years ago by etn40ff

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 tscrim

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 tscrim

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

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 etn40ff

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.

Last edited 5 years ago by etn40ff (previous) (diff)

comment:42 Changed 5 years ago by etn40ff

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 follow-up: Changed 5 years ago by tscrim

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 stumpc5

Replying to etn40ff:

  • d_vectors: I did not implement it yet but it is easily doable. This raises a question concerning cluster_variables and F_polynomials. These, as d_vectors would be, are simple syntactic sugar around a map. For example cluster_variables is the same as
    sage: 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 etn40ff

Replying to tscrim:

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.

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 sage-devel 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 follow-up: Changed 5 years ago by tscrim

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 5 years ago by etn40ff

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 5 years ago by git

  • Commit changed from 8c290be8819933ab781afcea3f445ee4f7108417 to fbbb24fc3c5776bab32dfa8f678f1249506b8b1b

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

56c21e0Merge branch 'public/ticket/21254' of trac.sagemath.org:sage into public/ticket/21254
0cf61b7Initial pass of documentation fixes and added TestSuite (to be fixed).
fbbb24fCreating element class for cluster algebras with prinicpal coefficients.

comment:49 follow-up: Changed 5 years ago by tscrim

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.

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

Version 0, edited 5 years ago by tscrim (next)

comment:50 in reply to: ↑ 49 ; follow-up: Changed 5 years ago by etn40ff

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

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 ; follow-up: Changed 5 years ago by stumpc5

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 ; follow-up: Changed 5 years ago by 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?

comment:53 in reply to: ↑ 52 ; follow-up: Changed 5 years ago by 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.

comment:54 in reply to: ↑ 53 Changed 5 years ago by etn40ff

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 5 years ago by tscrim

  • Keywords days79 added
  • Milestone changed from sage-7.4 to sage-7.5

Here is the short summary of our discussion:

  • We will replace initial_mutate by something called change_initial_seed (the name has not really been decided) to create a new cluster algebra using another seed s as initial data and copy the old data into the new cluster algebra using s (and its mutation sequence).
  • We will remove the decorator and implement the UniqueRepresentation behavior for ClusterAlgebra.
  • 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 5 years ago by git

  • Commit changed from fbbb24fc3c5776bab32dfa8f678f1249506b8b1b to 97d3890a154c39f0d4ccd0f1baa05bf82562918f

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

97d3890First attempt at using UniqueRepresentation, mutate_initial should now be broken

comment:57 Changed 5 years ago by etn40ff

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 5 years ago by tscrim

I cannot reproduce that behavior: I get 3 failures on every run.

comment:59 Changed 5 years ago by git

  • Commit changed from 97d3890a154c39f0d4ccd0f1baa05bf82562918f to 1bb0f903519c4771ed8663b5d0c051b9aa1a422e

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

f3171f6Second pass at using UniqueRepresentatio; still need to remove useless methods like __eq__
84b4942Removed useless methods and got the code to fail with UniqeRepresentation
3356c33Implemented new mutate_initial; this is now consistent with the use of UniqueRepresentation
1bb0f90Removed old mutate_initial; removed mutate_wrapper; refactored mutate

comment:60 Changed 5 years ago by etn40ff

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 5 years ago by git

  • Commit changed from 1bb0f903519c4771ed8663b5d0c051b9aa1a422e to 446619fc0ac79fc0e240df68a09255f3f328b341

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

446619fPolish + removed MethodType

comment:62 Changed 4 years ago by git

  • Commit changed from 446619fc0ac79fc0e240df68a09255f3f328b341 to 1d5bfc793a8564ee3fcea999e35e3262a159b51e

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

872a4d0Update to version 7.5.rc1. Merge branch 'public/ticket/21254' of git://trac.sagemath.org/sage into public/ticket/21254
5341cbd21254: add titles and authors to references LLZ2014 and NZ2012, remove duplicate FZ2007
1d5bfc721254: add cluster algebras to /algebras/catalog.py

comment:63 Changed 4 years ago by egunawan

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 etn40ff

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

Last edited 4 years ago by etn40ff (previous) (diff)

comment:65 Changed 4 years ago by git

  • Commit changed from 1d5bfc793a8564ee3fcea999e35e3262a159b51e to a80a18450a2292348d76af7dbf18c04b752fd8f2

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

a80a184Revert 5341cbd219c650801030dd9df3dd49b34e3c6952

comment:66 Changed 4 years ago by git

  • Commit changed from a80a18450a2292348d76af7dbf18c04b752fd8f2 to f4417101137d2f6f65b9ec5b552d7b068b444d2a

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

f441710Removed blank line

comment:67 Changed 4 years ago by chapoton

+Missing doctests in algebras/cluster_algebra.py: 74 / 75 = 98%

comment:68 Changed 4 years ago by git

  • Commit changed from f4417101137d2f6f65b9ec5b552d7b068b444d2a to dbf42e6c5f4d3ef0c0dbac97d88829b47431a544

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

dbf42e6Added missing doctest

comment:69 Changed 4 years ago by etn40ff

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 chapoton

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 git

  • Commit changed from dbf42e6c5f4d3ef0c0dbac97d88829b47431a544 to 06000a4f2e9a2a2b2bf6455add97c25dd486a798

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

6c2d7a2First attempt at using UniqueRepresentation, mutate_initial should now be broken
130de19Second pass at using UniqueRepresentatio; still need to remove useless methods like __eq__
66e75cfRemoved useless methods and got the code to fail with UniqeRepresentation
bcac0e9Implemented new mutate_initial; this is now consistent with the use of UniqueRepresentation
fdc90e8Removed old mutate_initial; removed mutate_wrapper; refactored mutate
41560eePolish + removed MethodType
5a827a121254: add titles and authors to references LLZ2014 and NZ2012, remove duplicate FZ2007
398129921254: add cluster algebras to /algebras/catalog.py
94a5d1aRevert 5341cbd219c650801030dd9df3dd49b34e3c6952
06000a4Added missing doctest

comment:72 Changed 4 years ago by etn40ff

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 chapoton

  • 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 chapoton

  • Milestone changed from sage-7.5 to sage-7.6

comment:75 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

On 32-bit:

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 etn40ff

On 64-bit it works. Which is the correct way to doctest hash functions independently of the architecture?

comment:77 Changed 4 years ago by tscrim

Copy and paste Volker's output with # 32-bit and # 64-bit. Although I prefer the softest that hash(x) == hash(x).

comment:78 Changed 4 years ago by git

  • Commit changed from 06000a4f2e9a2a2b2bf6455add97c25dd486a798 to 389cdc925fb4ed8b438c2bece4f078cefe248d4a

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

389cdc9Fixed hash doctest

comment:79 Changed 4 years ago by etn40ff

  • Status changed from needs_work to positive_review

Done, thanks. I am setting this back to positive review because I do not have a 32-bit machine to test.

comment:80 Changed 4 years ago by vbraun

  • Branch changed from public/ticket/21254 to 389cdc925fb4ed8b438c2bece4f078cefe248d4a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.