Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#5200 closed enhancement (fixed)

[with patch, positive review] subsets and subwords bug fix + improvements.

Reported by: hivert Owned by: hivert
Priority: major Milestone: sage-3.4.1
Component: combinatorics Keywords: subsets, subwords
Cc: sage-combinat Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by hivert)

This patches improves subwords and subsets and deals with several issues:

  1. It implements subsets for finite multisets (sets with repetitions).
    sage: Subsets([2,2,3], multiset=True).list() 
    [[], [2], [3], [2, 2], [2, 3], [2, 2, 3]] 
    
  2. It implement __contains__ which was missing for subsets and subwords: Before:
    sage: st = Subsets([1,2,2,3]); Set([1,2]) in st 
    --------------------------------------------------------------------------- 
    NotImplementedError                       Traceback (most recent call last) 
    
    After:
    sage: st = Subsets([1,2,2,3]); Set([1,2]) in st 
    True 
    
  3. It fixes a bug in smallest_positions: Before:
    sage: sage.combinat.subword.smallest_positions([2,4,3,3,1,2],[1,3,3]) 
    [4, 4, 4] 
    
    After:
    sage.combinat.subword.smallest_positions([2,4,3,3,1,2],[1,3,3]) 
    False 
    
    which means that 113 is not a subword of 243312.
  4. It finally improves the doc and the tests.

Cheers,

Florent

Attachments (2)

subsets_subwords-5200-submitted.2.patch (17.0 KB) - added by hivert 12 years ago.
New patch after Nicolas review.
subsets_subwords-5200-review.patch (2.1 KB) - added by hivert 12 years ago.
Last review patch.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 12 years ago by hivert

  • Summary changed from [with patch, needs review] subsets and subwords bug fix + improvements. to [with patch, needs review, doc needs work] subsets and subwords bug fix + improvements.

comment:2 Changed 12 years ago by hivert

It has been decided (direct talk with Nicolas + irssi with Mike) that the user has to explicitely set that he want multisets. Therefore, on the contrary that is anounced, the following is not working:

sage: Subsets([2,2,3]).list()
[[], [2], [3], [2, 2], [2, 3], [2, 2, 3]]

Instead one should write

sage: Subsets([2,2,3], multiset=True).list()
[[], [2], [3], [2, 2], [2, 3], [2, 2, 3]]

comment:3 Changed 12 years ago by hivert

  • Description modified (diff)
  • Owner changed from mhansen to hivert
  • Summary changed from [with patch, needs review, doc needs work] subsets and subwords bug fix + improvements. to [with patch, needs review] subsets and subwords bug fix + improvements.

comment:4 Changed 12 years ago by hivert

  • Status changed from new to assigned

comment:5 Changed 12 years ago by hivert

  • Description modified (diff)

comment:6 Changed 12 years ago by hivert

Sorry for the mess with the description... I just realize because of my mistake that it is possible to change the description. :-)

I've uploaded a new patch according to remark of Nicolas and Mike. It should be ready for review and hopefully integration.

Cheers,

Florent

Changed 12 years ago by hivert

New patch after Nicolas review.

comment:7 Changed 12 years ago by hivert

Nicolas sent me a review on sage-combinat-devel. The new subsets_subwords-5200-submitted.2.patch patch fixes the various problems pointed out by the review (typos + code improvements).

Florent

comment:8 Changed 12 years ago by nthiery

Florent: feel free to switch the title to with review after the following micro updates:

  • iterate -> iterates
  • of all -> ""

Changed 12 years ago by hivert

Last review patch.

comment:9 Changed 12 years ago by hivert

  • Summary changed from [with patch, needs review] subsets and subwords bug fix + improvements. to [with patch, with review] subsets and subwords bug fix + improvements.

The review patch I just submitted address Nicolas last comment. Sorry for the mess with several version of the patch. The correct patch are:

According to Nicolas la remarks I allow myself to change the review title. Please tell me if it's not allowed by the review rules.

Florent

comment:10 Changed 12 years ago by mabshoff

  • Summary changed from [with patch, with review] subsets and subwords bug fix + improvements. to [with patch, needs review] subsets and subwords bug fix + improvements.

"with review" is meaningless :)

Nicolas: Can you give this a formal positive review?

Cheers,

Michael

comment:11 Changed 12 years ago by mabshoff

  • Summary changed from [with patch, needs review] subsets and subwords bug fix + improvements. to [with patch, positive review] subsets and subwords bug fix + improvements.

After re-reading Nicola's comment I am giving this patch a positive review in his name. The two patches also pass doctests.

Cheers,

Michael

comment:12 Changed 12 years ago by mabshoff

  • Milestone changed from sage-combinat to sage-3.4.1
  • Resolution set to fixed
  • Status changed from assigned to closed

Merged both patches in Sage 3.4.1.alpha0.

Cheers,

Michael

comment:13 Changed 12 years ago by nthiery

  • Cc sage-combinat added
Note: See TracTickets for help on using tickets.