Opened 9 years ago

Closed 9 years ago

Last modified 9 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:

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 9 years ago.
New patch after Nicolas review.
subsets_subwords-5200-review.patch (2.1 KB) - added by hivert 9 years ago.
Last review patch.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 9 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 9 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 9 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 9 years ago by hivert

  • Status changed from new to assigned

comment:5 Changed 9 years ago by hivert

  • Description modified (diff)

comment:6 Changed 9 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 9 years ago by hivert

New patch after Nicolas review.

comment:7 Changed 9 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 9 years ago by nthiery

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

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

Changed 9 years ago by hivert

Last review patch.

comment:9 Changed 9 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 9 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 9 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 9 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 9 years ago by nthiery

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