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:

Status badges

Description (last modified by tscrim)

This patch implements Suter's dihedral group action on (subsets of) the partition lattice, and fixes some typos in docstrings.


Apply:

Attachments (3)

trac_14763-suter_slides-dg.patch (7.8 KB) - added by darij 8 years ago.
Updated version, now also computes the inverse of the Suter slide without iterating it n-1 times
trac_14763-rereview-dg.patch (4.3 KB) - added by darij 8 years ago.
minor changes to Travis's review
trac_14763-review-ts.patch (10.8 KB) - added by tscrim 8 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 8 years ago by darij

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

comment:2 Changed 8 years ago by darij

  • Component changed from PLEASE CHANGE to combinatorics

Changed 8 years ago by darij

Updated version, now also computes the inverse of the Suter slide without iterating it n-1 times

comment:3 Changed 8 years ago by tscrim

  • 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.

Changed 8 years ago by darij

minor changes to Travis's review

comment:4 Changed 8 years ago by darij

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 darij

  • Description modified (diff)

comment:6 Changed 8 years ago by tscrim

  • 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 darij

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 tscrim

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 darij

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 tscrim

So positive review?

comment:11 Changed 8 years ago by darij

  • Status changed from needs_review to positive_review

Yes.

comment:12 Changed 8 years ago by darij

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 tscrim

It is possible to change it, and I've done so.

comment:14 Changed 8 years ago by jdemeyer

trac_14763-review-ts.patch is missing some metadata, in particular the author name.

comment:15 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_info

Changed 8 years ago by tscrim

comment:16 Changed 8 years ago by tscrim

  • Status changed from needs_info to needs_review

Forgot to export, sorry about that.

comment:17 Changed 8 years ago by tscrim

  • Status changed from needs_review to positive_review

comment:18 Changed 8 years ago by jdemeyer

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