Opened 8 years ago

Closed 8 years ago

#14048 closed defect (fixed)

Fix a bug in class_size for QuiverMutationType (in types F and G)

Reported by: hthomas Owned by: sage-combinat
Priority: major Milestone: sage-5.7
Component: combinatorics Keywords: cluster algebras, class_size, exceptional types
Cc: gmoose05, stumpc5 Merged in: sage-5.7.beta3
Authors: Hugh Thomas Reviewers: Christian Stump
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Jeroen ran "pyflakes" on the sage code, which detects certain kinds of problems (which can then be confirmed by testing).

sage: X=QuiverMutationType('F',4,(2,1))
sage: X.class_size()
...
NameError: global name 'twist' is not defined

Same issue for type G.

Attachments (1)

trac_14048-fix-cluster-class_size-bug-ht.patch (1.7 KB) - added by hthomas 8 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 8 years ago by hthomas

Oh, also, turns out we were checking that twist be a tuple, and it is never a tuple, so this wasn't the only problem in the code!

Patch coming soon.

comment:2 Changed 8 years ago by hthomas

  • Status changed from new to needs_review

comment:3 Changed 8 years ago by hthomas

It would also be fine with me if you want to fold this into one of the big cluster patches.

comment:4 Changed 8 years ago by gmoose05

Thanks for catching this Jeroen and Hugh!

Just to clarify, when you say "this wasn't the only problem in the code", does trac_14048-fix-cluster-class_size-bug-ht.patch appear to fix the issue that Jereon found or was there an additional issue that we had to deal with?

Also to update you: 10527 and 10538 are merged into 5.5 and 5.6.

Christian, Salvatore, and I are nearing completion of 13369 (perhaps by next week), and 13424 should be ready shortly after.

If the ticket really is just these couple lines, and I don't believe 13369 or 13424 alter quiver_mutation_type.py, we could double-check that this fix would be independent of our current work and then possibly give it a quick positive review.

Also, Christian and I will both be at ICERM Feb 11-15 so can discuss it there, if this ticket is still outstanding then.

Gregg

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

comment:5 Changed 8 years ago by hthomas

I fixed the issue Jeroen found and also the issue I found. So this ticket is ready to go and, so far as I know, makes class_size work correctly.

It sounds like the easiest thing might be if you can check it doesn't play badly with the new patches, and give it a green light.

cheers,

Hugh

comment:6 Changed 8 years ago by stumpc5

  • Reviewers set to Christian Stump
  • Status changed from needs_review to positive_review

It commutes with #13369, it catches the bug, and is documented -- thanks Hugh!

comment:7 Changed 8 years ago by jdemeyer

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