Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement cluster algebras using the Parent-Element framework #21254

Closed
Etn40ff opened this issue Aug 15, 2016 · 110 comments
Closed

Implement cluster algebras using the Parent-Element framework #21254

Etn40ff opened this issue Aug 15, 2016 · 110 comments

Comments

@Etn40ff
Copy link
Contributor

Etn40ff commented Aug 15, 2016

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.

Depends on #22160

CC: @sagetrac-gmoose05 @sagetrac-drupel @stumpc5 mdpressland@bath.edu @tscrim @egunawan

Component: algebra

Keywords: cluster algebras, SageDays64.5, days79

Author: Dylan Rupel, Salvatore Stella

Branch/Commit: 389cdc9

Reviewer: Frédéric Chapoton

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

@Etn40ff Etn40ff added this to the sage-7.4 milestone Aug 15, 2016
@Etn40ff
Copy link
Contributor Author

Etn40ff commented Aug 15, 2016

Commit: b76ba97

@Etn40ff
Copy link
Contributor Author

Etn40ff commented Aug 15, 2016

Branch: public/ticket/21254

@Etn40ff
Copy link
Contributor Author

Etn40ff commented Aug 15, 2016

Dependencies: #19538

@Etn40ff

This comment has been minimized.

@Etn40ff Etn40ff self-assigned this Aug 15, 2016
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 20, 2016

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

e281061Added warning to functions depending on _sd_iter

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 20, 2016

Changed commit from b76ba97 to e281061

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 21, 2016

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

2c64756Spaaaaaace (again)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 21, 2016

Changed commit from e281061 to 2c64756

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 22, 2016

Changed commit from 2c64756 to e272302

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 22, 2016

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

e272302Catch only StopIteration

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 23, 2016

Changed commit from e272302 to 673926d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 23, 2016

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

673926dAdded hash function to ClusterAlgebraSeed

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2016

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

31a51a1Figured out how to continue seeds after a KeyboardInterrupt

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2016

Changed commit from 673926d to 31a51a1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2016

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

13625a1Rafactored all fuctions based on seeds()

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2016

Changed commit from 31a51a1 to 13625a1

@fchapoton
Copy link
Contributor

comment:8

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)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2016

Changed commit from 13625a1 to 2e4ab20

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2016

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

bb66742Better interrupt handling
2e4ab20Fixed missing :

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2016

Changed commit from 2e4ab20 to 3dd81b4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2016

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

3dd81b4Fixed missing empty lines

@Etn40ff
Copy link
Contributor Author

Etn40ff commented Aug 25, 2016

comment:11

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 26, 2016

Changed commit from 3dd81b4 to 8f0480a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 26, 2016

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

8f0480aMissing digit in doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 5, 2016

Changed commit from 8f0480a to 7400632

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 9, 2017

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

f441710Removed blank line

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 9, 2017

Changed commit from a80a184 to f441710

@fchapoton
Copy link
Contributor

comment:67

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 14, 2017

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

dbf42e6Added missing doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 14, 2017

Changed commit from f441710 to dbf42e6

@Etn40ff
Copy link
Contributor Author

Etn40ff commented Jan 14, 2017

comment:69

Thank you for catching this Frédéric. I hope I added the test in the right place.

@fchapoton
Copy link
Contributor

comment:70

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 14, 2017

Changed commit from dbf42e6 to 06000a4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 14, 2017

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

@Etn40ff
Copy link
Contributor Author

Etn40ff commented Feb 14, 2017

comment:72

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.

@fchapoton
Copy link
Contributor

comment:73

ok, let it be

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@vbraun
Copy link
Member

vbraun commented Feb 17, 2017

comment:75

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

@Etn40ff
Copy link
Contributor Author

Etn40ff commented Feb 17, 2017

comment:76

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

@tscrim
Copy link
Collaborator

tscrim commented Feb 17, 2017

comment:77

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 17, 2017

Changed commit from 06000a4 to 389cdc9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 17, 2017

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

389cdc9Fixed hash doctest

@Etn40ff
Copy link
Contributor Author

Etn40ff commented Feb 17, 2017

comment:79

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

@vbraun
Copy link
Member

vbraun commented Feb 23, 2017

Changed branch from public/ticket/21254 to 389cdc9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants