Opened 5 years ago

Closed 4 years ago

#13424 closed enhancement (fixed)

Compute Mutation Class for Cluster Algebra Seed or Cluster Quiver

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

Description (last modified by stumpc5)

This ticket contains additional features for computing mutation classes of ClusterSeeds and ClusterQuivers.

Attachments (1)

trac_13424-mutation_classes.patch (70.9 KB) - added by stumpc5 4 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 5 years ago by stumpc5

  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 4 years ago by gmoose05

Rebased slightly now that tickets 10527 and 10538 are in Sage. I will modify further over the next few weeks.

comment:3 Changed 4 years ago by gmoose05

  • Authors changed from Gregg Musiker, Christian Stump to Christian Stump
  • Reviewers set to Gregg Musiker

comment:4 follow-up: Changed 4 years ago by gmoose05

New upload: added documentation and examples for new commands.

Still to do:

1) Add a function _has_two_cycles

2) Fix documenation for, or possibly delete "old-methods"

3) More thorough check of code and trying to break functions or find corner cases.

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

Replying to gmoose05:

2) Fix documenation for, or possibly delete "old-methods"

I uploaded a new version where I fixed several minor things: the patch should now apply and should have 100% coverage. I moreover deleted several methods that we do not need for this ticket (we might need to get them back later for #13425).

Please download the newest version of the patch!

Cheers, Christian

comment:6 Changed 4 years ago by gmoose05

Just added the missing _has_two_cycles function.

Christian's recent changes seems to have solidified the documentation and coverage. (Although I don't see ClusterSeed? or MutationClass?.py commands showing up)

Pending 13369 and some additional testing to make sure no corner cases, should be ready to go.

comment:7 Changed 4 years ago by stumpc5

Here is a first review:

  • we should add some tests that the computed class sizes coincide with the expected class sizes in QuiverMutationType?
  • the non-iter methods (mutation_class <-> mutation_class_iter) should also contain examples since these are the typical used methods
  • l1318: "# runs forever without the mutation type recognition patch applied" we should make clear what this means (I don't even know currently).
  • recheck that the file mutation_class.py is organized well.
  • building the doctest and going through the docs.

Beside these, the patch looks good to me: the code is a little complicated but the methods are well-documented.

comment:8 Changed 4 years ago by gmoose05

  • Authors changed from Christian Stump to Christian Stump, Gregg Musiker

comment:9 Changed 4 years ago by etn40ff

  • Keywords days45 added

comment:10 Changed 4 years ago by etn40ff

  • Reviewers changed from Gregg Musiker to Gregg Musiker, Salvatore Stella

comment:11 Changed 4 years ago by etn40ff

  • Status changed from needs_review to positive_review

comment:12 Changed 4 years ago by jdemeyer

  • Milestone changed from sage-5.7 to sage-5.8

comment:13 Changed 4 years ago by gmoose05

  • Status changed from positive_review to needs_work

comment:14 follow-up: Changed 4 years ago by jdemeyer

You should not use

except:

without exceptions, otherwise you catch KeyboardInterrupt. Catch specific exceptions or

except StandardError:

if you really want "everything".

comment:15 in reply to: ↑ 14 Changed 4 years ago by stumpc5

Replying to jdemeyer:

except StandardError:

fixed this issue (among others).

cheers, christian

comment:16 Changed 4 years ago by stumpc5

  • Status changed from needs_work to needs_review

Is there anything left here? This question is in particular for Salvatore!

comment:17 follow-up: Changed 4 years ago by gmoose05

I just posted a fourth review patch that

-slightly improves the documentation for up_to_equivalence, and

-starts depth counting at 0 when show_depth=True. This is most relevant for the iterator functions.

I uploaded this a second time (meaning to rewrite over the original) but forgot to check the "replace" box.

Consequently, I replaced the first file with a 216 byte empty patch.

Hopefully the patchbot will be happy with this. Christian, if you get a chance, feel free to delete the 216 byte patch.

Pending the patchbot and other further issues, I think this ticket is now ready for a positive review.

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

comment:18 in reply to: ↑ 17 Changed 4 years ago by stumpc5

Replying to gmoose05:

I just posted a fourth review patch that

-slightly improves the documentation for up_to_equivalence, and

-starts depth counting at 0 when show_depth=True. This is most relevant for the iterator functions.

I am fine with all your changes, thanks! From my side, you can give it a positive review as soon as we get a green light from the patchbot.

Cheers, Christian

Changed 4 years ago by stumpc5

comment:19 Changed 4 years ago by stumpc5

  • Status changed from needs_review to positive_review

comment:20 Changed 4 years ago by jdemeyer

  • Milestone changed from sage-5.8 to sage-5.9

comment:21 Changed 4 years ago by jdemeyer

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