Opened 5 years ago
Closed 5 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 )
This ticket contains additional features for computing mutation classes of ClusterSeeds and ClusterQuivers.
Attachments (1)
Change History (22)
comment:1 Changed 5 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 5 years ago by
comment:3 Changed 5 years ago by
- Reviewers set to Gregg Musiker
comment:4 follow-up: ↓ 5 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
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 5 years ago by
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 5 years ago by
comment:9 Changed 5 years ago by
- Keywords days45 added
comment:10 Changed 5 years ago by
- Reviewers changed from Gregg Musiker to Gregg Musiker, Salvatore Stella
comment:11 Changed 5 years ago by
- Status changed from needs_review to positive_review
comment:12 Changed 5 years ago by
- Milestone changed from sage-5.7 to sage-5.8
comment:13 Changed 5 years ago by
- Status changed from positive_review to needs_work
comment:14 follow-up: ↓ 15 Changed 5 years ago by
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 5 years ago by
comment:16 Changed 5 years ago by
- Status changed from needs_work to needs_review
Is there anything left here? This question is in particular for Salvatore!
comment:17 follow-up: ↓ 18 Changed 5 years ago by
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.
comment:18 in reply to: ↑ 17 Changed 5 years ago by
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 5 years ago by
comment:19 Changed 5 years ago by
- Status changed from needs_review to positive_review
comment:20 Changed 5 years ago by
- Milestone changed from sage-5.8 to sage-5.9
comment:21 Changed 5 years ago by
- Merged in set to sage-5.9.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
Rebased slightly now that tickets 10527 and 10538 are in Sage. I will modify further over the next few weeks.