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)
Change History (8)
comment:1 Changed 8 years ago by
Changed 8 years ago by
comment:2 Changed 8 years ago by
- Status changed from new to needs_review
comment:3 Changed 8 years ago by
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
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
comment:5 Changed 8 years ago by
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
- 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
- Merged in set to sage-5.7.beta3
- Resolution set to fixed
- Status changed from positive_review to closed
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.