Opened 8 years ago
Closed 8 years ago
#14763 closed enhancement (fixed)
Suter's diagonal slides on subsets of the partition lattice
Reported by: | darij | Owned by: | tbd |
---|---|---|---|
Priority: | major | Milestone: | sage-5.11 |
Component: | combinatorics | Keywords: | days49, partitions |
Cc: | sage-combinat, tscrim, | Merged in: | sage-5.11.beta3 |
Authors: | Darij Grinberg | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #8392 | Stopgaps: |
Description (last modified by )
This patch implements Suter's dihedral group action on (subsets of) the partition lattice, and fixes some typos in docstrings.
Apply:
Attachments (3)
Change History (21)
comment:1 Changed 8 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 8 years ago by
- Component changed from PLEASE CHANGE to combinatorics
Changed 8 years ago by
comment:3 Changed 8 years ago by
- Description modified (diff)
- Reviewers set to Travis Scrimshaw
Review patch which removes non-ascii characters, reworks the doc, and checks that the input is valid. If you're happy with my changes, go ahead and set this to positive reivew.
comment:4 Changed 8 years ago by
I agree with most of your review; thanks for catching the non-ascii copypasta in particular. I've made checking optional and reinstated the "length = m" condition in the docstring since various people (like me) consider (2, 1), (2, 1, 0) and (2, 1, 0, 0, 0, ...) to be three equivalent ways to write one and the same partition.
comment:5 Changed 8 years ago by
- Description modified (diff)
comment:6 Changed 8 years ago by
- Description modified (diff)
Here's the new review patch. Come find me if you want anything else changed.
Apply: trac_14763-suter_slides-dg.patch trac_14763-review-ts.patch
comment:7 Changed 8 years ago by
Please put the exp vs. n/2 check back.
Lines 1781-1784 are redundant. That's my bad, sorry. I thought if the empty partition case was necessary in the positive exp case, it would also be in the negative exp one; but it isn't.
Lines 1785-1786 have a (now redundant) check.
Thanks for reviewing!
comment:8 Changed 8 years ago by
The empty partition case is necessary in both positive and negative exp
; there's even a doctest on it on line 1696.
Apply: trac_14763-suter_slides-dg.patch trac_14763-review-ts.patch
comment:9 Changed 8 years ago by
I'm fine with it either way; I just don't see how anything will change if lines 1783-1786 are removed. Not like checking an int for being 0 is a time issue...
comment:10 Changed 8 years ago by
So positive review?
comment:12 Changed 8 years ago by
Oh, here's something I forgot to write. Line 1781 could (and I think should) be a simple "else", since we're in the while-loop. If you agree, Travis, I'll change it.
comment:13 Changed 8 years ago by
It is possible to change it, and I've done so.
comment:14 Changed 8 years ago by
trac_14763-review-ts.patch is missing some metadata, in particular the author name.
comment:15 Changed 8 years ago by
- Status changed from positive_review to needs_info
Changed 8 years ago by
comment:16 Changed 8 years ago by
- Status changed from needs_info to needs_review
Forgot to export, sorry about that.
comment:17 Changed 8 years ago by
- Status changed from needs_review to positive_review
comment:18 Changed 8 years ago by
- Merged in set to sage-5.11.beta3
- Resolution set to fixed
- Status changed from positive_review to closed
Updated version, now also computes the inverse of the Suter slide without iterating it n-1 times