Opened 5 years ago

Closed 4 years ago

#13369 closed enhancement (fixed)

Implementation of the class ClusterSeed

Reported by: stumpc5 Owned by: sage-combinat
Priority: major Milestone: sage-5.8
Component: combinatorics Keywords: cluster algebra, quiver, days45
Cc: Merged in: sage-5.8.beta0
Authors: Christian Stump, Gregg Musiker Reviewers: Salvatore Stella
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #10538 Stopgaps:

Description (last modified by gmoose05)

This class implements cluster seeds for skew-symmetrizable matrices.

The patch contains multiple examples.

sage: S = ClusterSeed(['A',3]); S
A seed for a cluster algebra of rank 3 of type ['A', 3]

Attachments (3)

trac_13369_cluster_seed.patch (37.4 KB) - added by stumpc5 4 years ago.
trac_13369-review.patch (12.5 KB) - added by stumpc5 4 years ago.
trac_13369-second_review.patch (12.2 KB) - added by stumpc5 4 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 5 years ago by stumpc5

  • Status changed from new to needs_review

comment:2 follow-up: Changed 5 years ago by gmoose05

  • trac_13369-cluster_seed-cs-NEW.patch is an exact copy of trac_13369-cluster_seed-cs.patch, except in quiver.py where the change to lines 97-100 now apply to lines 201-204 instead. (Also one typo fixed.)

This was done because trac_13369-cluster_seed-cs.patch no longer applies after recent changes to 10538, although it really was only this four line conflict.

  • More substantially, trac_13369-cluster-seed-gm.patch fixes compatibility issues with recent changes to 10538 and 10527. One can now apply all patches in 10527 in order, then all patches in 10538 in order, then the *two patches*

trac_13369-cluster_seed-cs-NEW.patch followed by trac_13369-cluster_seed-gm.patch,

and then the rebuilt sage passes all doc tests and appears to work correctly.

  • A more substantial review will be forthcoming.

comment:3 Changed 5 years ago by gmoose05

  • Description modified (diff)

comment:4 Changed 5 years ago by stumpc5

  • Description modified (diff)
  • Milestone changed from sage-5.3 to sage-5.4

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

Replying to gmoose05:

I just renamed and uploaded the two relevant patches without any changes.

comment:6 Changed 5 years ago by gmoose05

Also, despite the patchbot's inability to apply the patches, it works fine with 5.3.rc1 on my machine (with the 10527 and 10538 patches applied first).

comment:7 Changed 5 years ago by stumpc5

Both patches rebased according to the new lazy import. No other changes.

comment:8 Changed 4 years ago by etn40ff

  • Reviewers set to Salvatore Stella

comment:9 Changed 4 years ago by stumpc5

Hi,

I updated my first patch (and uploaded Gregg's patch without changes). This resulted in some missing doctests which I then added in yet another patch. We also had some old things in the ClusterVariable class which I removed. If we need that back later, we can do it in another ticket...

Cheers, Christian

comment:10 Changed 4 years ago by stumpc5

  • Authors changed from Christian Stump to Christian Stump, Gregg Musiker
  • Description modified (diff)

Changed 4 years ago by stumpc5

comment:11 Changed 4 years ago by gmoose05

  • Description modified (diff)

Changed 4 years ago by stumpc5

Changed 4 years ago by stumpc5

comment:12 Changed 4 years ago by stumpc5

  • Status changed from needs_review to positive_review

comment:13 Changed 4 years ago by jdemeyer

  • Milestone changed from sage-5.7 to sage-5.8

comment:14 Changed 4 years ago by etn40ff

  • Keywords days45 added

comment:15 Changed 4 years ago by jdemeyer

  • Merged in set to sage-5.8.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.