Opened 8 years ago

Closed 8 years ago

#5600 closed enhancement (fixed)

[with patch, positive review] Cleanup of integer compositions

Reported by: nthiery Owned by: nthiery
Priority: major Milestone: sage-4.1.2
Component: combinatorics Keywords: integer compositions
Cc: sage-combinat, jbandlow Merged in: Sage 4.1.2.alpha0
Authors: Nicolas M. Thiéry Reviewers: Dan Drake, Jason Bandlow, Minh Van Nguyen
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by nthiery)

  • Documentation improvements
  • Fixes some of http://wiki.sagemath.org/combinat/Weirdness
  • Composition(l) accepts any iterable l, and in particular a tuple
  • New functionalities:
    • concatenation (as add and sum)
    • size
    • fatter, finer, fatten (refinement of compositions)
  • Uses IntegerListsLex? (fast iteration, ...) instead of not any better specific code Note: this changes the iteration order to inverse lexicographic, and iteration changes the iteration order for set partitions, skew partitions, and skew tableaux.

Attachments (4)

compositions-cleanup-5600-nt.patch (40.4 KB) - added by nthiery 8 years ago.
trac_5600-compositions_cleanup-nt.2.patch (41.5 KB) - added by nthiery 8 years ago.
trac_5600-compositions_cleanup-nt.patch (41.5 KB) - added by nthiery 8 years ago.
Apply only this one, yes, this one!
trac_5600-reviewer.patch (6.8 KB) - added by mvngu 8 years ago.
reviewer patch

Download all attachments as: .zip

Change History (16)

comment:1 Changed 8 years ago by nthiery

  • Description modified (diff)

comment:2 Changed 8 years ago by nthiery

  • Description modified (diff)

comment:3 Changed 8 years ago by nthiery

  • Cc sage-combinat-devel@… removed
  • Description modified (diff)

comment:4 Changed 8 years ago by nthiery

  • Cc sage-combinat added

comment:5 Changed 8 years ago by nthiery

  • Description modified (diff)
  • Keywords integer compositions added
  • Summary changed from Cleanup of integer compositions to [with patch, needs review] Cleanup of integer compositions

Changed 8 years ago by nthiery

comment:6 Changed 8 years ago by nthiery

  • Status changed from new to assigned

comment:7 Changed 8 years ago by ddrake

  • Summary changed from [with patch, needs review] Cleanup of integer compositions to [with patch, needs work] Cleanup of integer compositions

I very much want this patch to get in, since I almost opened a ticket for one of the problems that it fixes. (Composition() doesn't take tuples as input.) One thing that needs to change, though, is with the docstring for Composition: right now, it tells the user to do Composition_class?, but that doesn't work since Composition_class is not in the default namespace. I think Composition? should simply display the correct docstring; there's no need to annoy the user by sending him or her bouncing from one docstring to the next.

Another issue is that the AUTHORS block is deleted from the head of the file. This should not be done. In fact, you should add yourself to it as the developer's guide suggests: something like "Nicolas M. Thiery (2009-05-25): several cleanups, new functions, and improvements (trac #5600)". I like listing ticket numbers because interested developers can go see exactly what you changed.

Tiny grammar/spelling issues:

  • At the top of compositions.py, you write "a compositions c of..."; you should use the singular. "A composition c of..."
  • line 56: you say a composition is a list of positive integers, but it should be a list of nonnegative integers.

Finally, I am seeing a doctest failure with this patch applied to vanilla 4.0.rc0:

**********************************************************************
File "/var/tmp/sage-4.0.rc0/devel/sage/sage/combinat/ribbon_tableau.py", line 875:
    sage: SemistandardMultiSkewTableaux([sp[0], sp[-1]],[2,2,2]).list()
Expected:
    [[[[1], [2], [3]], [[1, 2, 3]]]]
Got:
    [[[[1, 1, 2]], [[None, None, 3], [None, 3], [2]]], [[[1, 2, 2]], [[None, None, 3], [None, 3], [1]]], [[[1, 1, 2]], [[None, None, 3], [None, 2], [3]]], [[[1, 2, 2]], [[None, None, 3], [None, 1], [3]]], [[[1, 1, 2]], [[None, None, 2], [None, 3], [3]]], [[[1, 2, 2]], [[None, None, 1], [None, 3], [3]]], [[[1, 1, 3]], [[None, None, 3], [None, 2], [2]]], [[[1, 2, 3]], [[None, None, 3], [None, 2], [1]]], [[[1, 2, 3]], [[None, None, 3], [None, 1], [2]]], [[[2, 2, 3]], [[None, None, 3], [None, 1], [1]]], [[[1, 1, 3]], [[None, None, 2], [None, 3], [2]]], [[[1, 2, 3]], [[None, None, 2], [None, 3], [1]]], [[[1, 2, 3]], [[None, None, 1], [None, 3], [2]]], [[[2, 2, 3]], [[None, None, 1], [None, 3], [1]]], [[[1, 1, 3]], [[None, None, 2], [None, 2], [3]]], [[[1, 2, 3]], [[None, None, 2], [None, 1], [3]]], [[[1, 2, 3]], [[None, None, 1], [None, 2], [3]]], [[[2, 2, 3]], [[None, None, 1], [None, 1], [3]]], [[[1, 3, 3]], [[None, None, 2], [None, 2], [1]]], [[[1, 3, 3]], [[None, None, 2], [None, 1], [2]]], [[[1, 3, 3]], [[None, None, 1], [None, 2], [2]]], [[[2, 3, 3]], [[None, None, 2], [None, 1], [1]]], [[[2, 3, 3]], [[None, None, 1], [None, 2], [1]]], [[[2, 3, 3]], [[None, None, 1], [None, 1], [2]]]]
**********************************************************************

comment:8 Changed 8 years ago by nthiery

  • Authors set to Nicolas M. Thiéry
  • Cc jbandlow added
  • Milestone changed from sage-combinat to sage-4.1.1
  • Reviewers set to Dan Drake, Jason Bandlow
  • Summary changed from [with patch, needs work] Cleanup of integer compositions to [with patch, needs review] Cleanup of integer compositions

The updated patch fixes the (the failure was actually actually a trivial thing in the doctest; thanks to Jason for spotting this!)

Changed 8 years ago by nthiery

Changed 8 years ago by nthiery

Apply only this one, yes, this one!

comment:9 Changed 8 years ago by jbandlow

  • Summary changed from [with patch, needs review] Cleanup of integer compositions to [with patch, positive review] Cleanup of integer compositions

Doctests pass, and it looks like Dan's issues have been resolved. Positive review from me.

Changed 8 years ago by mvngu

reviewer patch

comment:10 follow-up: Changed 8 years ago by mvngu

I'm attaching the reviewer patch trac_5600-reviewer.patch. It fixes a number of typos introduced by trac_5600-compositions_cleanup-nt.patch, and edits some docstrings so they look a bit nicer when one views the corresponding pages in the reference manual. If people are happy with my reviewer patch trac_5600-reviewer.patch, then patches should be merged in this order:

  1. trac_5600-compositions_cleanup-nt.patch
  2. trac_5600-reviewer.patch

comment:11 in reply to: ↑ 10 Changed 8 years ago by nthiery

Replying to mvngu:

I'm attaching the reviewer patch trac_5600-reviewer.patch. It fixes a number of typos introduced by trac_5600-compositions_cleanup-nt.patch, and edits some docstrings so they look a bit nicer when one views the corresponding pages in the reference manual. If people are happy with my reviewer patch trac_5600-reviewer.patch, then patches should be merged in this order:

  1. trac_5600-compositions_cleanup-nt.patch
  2. trac_5600-reviewer.patch

Thanks much for fixing all those typos! positive review on your reviewer's patch.

comment:12 Changed 8 years ago by mvngu

  • Merged in set to Sage 4.1.2.alpha0
  • Resolution set to fixed
  • Reviewers changed from Dan Drake, Jason Bandlow to Dan Drake, Jason Bandlow, Minh Van Nguyen
  • Status changed from assigned to closed

Merged patches in this order:

  1. trac_5600-compositions_cleanup-nt.patch
  2. trac_5600-reviewer.patch
Note: See TracTickets for help on using tickets.