Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#13243 closed enhancement (fixed)

new methods for compositions

Reported by: saliola Owned by: sage-combinat
Priority: major Milestone: sage-5.3
Component: combinatorics Keywords: compositions, ncsf, sd40
Cc: chrisjamesberg, sage-combinat, zabrocki Merged in: sage-5.3.beta0
Authors: Franco Saliola, Nicolas M. Thiéry, Florent Hivert, Chris Berg Reviewers: Chris Berg, Mike Zabrocki
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13109 Stopgaps:

Status badges

Description (last modified by jdemeyer)

new functionality for the composition constructor:

  • construct a composition from a subset

new functionality for compositions:

  • parent -- returns Compositions()
  • reversed - reverses
  • refinement_splitting
  • refinement_splitting_lengths
  • deprecated refinement in favour of refinement_splitting_lengths
  • partial_sums(self, final=True):
  • to_subset(self, final=False):
  • to_partition(self):
  • shuffle_product(self, other, overlap=False):

new functionality for the set of all compositions:

  • put it in the category of InfiniteEnumeratedSets
  • subset method to extract subsets of composition (by size)

Apply:

Attachments (6)

trac_13243-new_methods_for_compositions-fs.patch (29.3 KB) - added by saliola 9 years ago.
only apply this one
trac_13243-new_methods_for_compositions-fs.2.patch (226 bytes) - added by saliola 9 years ago.
empty patch (this can be deleted)
trac_13243-rebased_deprecated_function_alias-fs.patch (1.2 KB) - added by saliola 9 years ago.
needed for 5.2.rc0 (updated the call to deprecated_function_alias)
trac_13243-review_changes-fs.patch (3.3 KB) - added by saliola 9 years ago.
changes based on Mike's review
trac_13243-documentation_fixes-fs.patch (4.9 KB) - added by saliola 9 years ago.
(more) documentation improvements based on first review
trac_13243-documentation_fixes_2-fs.patch (3.6 KB) - added by saliola 9 years ago.
improvements to documentation for shuffle_product

Download all attachments as: .zip

Change History (34)

comment:1 Changed 9 years ago by saliola

  • Status changed from new to needs_review

comment:2 Changed 9 years ago by chrisjamesberg

  • Status changed from needs_review to positive_review

comment:3 Changed 9 years ago by saliola

  • Status changed from positive_review to needs_work

Need to fix a dependency error. Patch coming 'soon'.

Changed 9 years ago by saliola

only apply this one

Changed 9 years ago by saliola

empty patch (this can be deleted)

comment:4 Changed 9 years ago by saliola

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

The errors were mainly due to the fact that the new category of graded enumerated sets does not yet exist (it exists on the sage-combinat queue, which is why tests passed on my machine). So I removed the two lines related to that.

I also modified a couple of doctests in permutation.py, which was caused by the change the descents method.

comment:5 Changed 9 years ago by saliola

Apply: trac_13243-new_methods_for_compositions-fs.patch

comment:6 Changed 9 years ago by saliola

  • Cc zabrocki added

Changed 9 years ago by saliola

needed for 5.2.rc0 (updated the call to deprecated_function_alias)

comment:7 Changed 9 years ago by saliola

  • Dependencies set to #13109

Apply: trac_13243-new_methods_for_compositions-fs.patch, trac_13243-rebased_deprecated_function_alias-fs.patch

(for the patchbot)

comment:8 Changed 9 years ago by saliola

  • Description modified (diff)

comment:9 Changed 9 years ago by saliola

Note that the patches attached to this ticket are newer than the patches on the sage-combinat queue.

Also, I'm in the process of updating this patch with improvements suggested by Mike. New version will be available soon.

Changed 9 years ago by saliola

changes based on Mike's review

comment:10 Changed 9 years ago by saliola

  • Reviewers changed from Chris Berg to Chris Berg, Mike Zabrocki

Here is a patch (trac_13243-review_changes-fs.patch), which should be applied on top of the earlier patch, that includes improvements suggested by Mike.

Patchbot:

Apply: trac_13243-new_methods_for_compositions-fs.patch, trac_13243-rebased_deprecated_function_alias-fs.patch, trac_13243-review_changes-fs.patch, trac_13243-documentation_fixes-fs.patch

Last edited 9 years ago by saliola (previous) (diff)

comment:11 Changed 9 years ago by saliola

  • Status changed from needs_review to needs_work

comment:12 Changed 9 years ago by saliola

  • Status changed from needs_work to needs_review

Changed 9 years ago by saliola

(more) documentation improvements based on first review

comment:13 Changed 9 years ago by saliola

Hello Mike,

Here is a final patch fixing some the issues with the documentation. The documentation now builds and renders nicely. Thanks for your suggestions.

I made separate patches so that it is easier for you to look at my recent changes (but if you prefer, I can post one patch). You only need to look at the following two files to see the changes I made after your review of the first patch:

  • trac_13243-review_changes-fs.patch
  • trac_13243-documentation_fixes-fs.patch

Franco

comment:14 Changed 9 years ago by saliola

  • Status changed from needs_review to needs_work
  • Work issues set to documentation improvements

Mike suggested some more documentation improvements. New patch on the way.

Changed 9 years ago by saliola

improvements to documentation for shuffle_product

comment:15 Changed 9 years ago by saliola

  • Status changed from needs_work to needs_review

Here is a patch with improvements to the documentation of shuffle product.

Patchbot:

Apply: trac_13243-new_methods_for_compositions-fs.patch, trac_13243-rebased_deprecated_function_alias-fs.patch, trac_13243-review_changes-fs.patch, trac_13243-documentation_fixes-fs.patch, trac_13243-documentation_fixes_2-fs.patch

comment:16 Changed 9 years ago by saliola

  • Work issues documentation improvements deleted

comment:17 follow-up: Changed 9 years ago by zabrocki

  • Status changed from needs_review to positive_review

comment:18 in reply to: ↑ 17 Changed 9 years ago by aschilling

Hi Franco,

You promised to import these patches also to the sage-combinat queue, since the current patch there completely breaks sage-5.2.rc0. I am confused as to which patches are supposed to be applied. For example trac_13243-rebased_deprecated_function_alias-fs.patch also has a newer version trac_13243-rebased_deprecated_function_alias-fs.2.patch, but this is not the one listed in your list.

Thanks,

Anne

comment:19 follow-up: Changed 9 years ago by jdemeyer

Please clarify which patches have to be applied.

comment:20 Changed 9 years ago by jdemeyer

  • Status changed from positive_review to needs_info

comment:21 in reply to: ↑ 19 ; follow-up: Changed 9 years ago by saliola

Replying to jdemeyer:

Please clarify which patches have to be applied.

If I understand correctly, the patchbot doesn't look at the description for the "apply" directive, so I put it in the comments. Should I also include the directive in the description? Is there a preferred syntax for this?

Apply: trac_13243-new_methods_for_compositions-fs.patch, trac_13243-rebased_deprecated_function_alias-fs.patch, trac_13243-review_changes-fs.patch, trac_13243-documentation_fixes-fs.patch, trac_13243-documentation_fixes_2-fs.patch

For humans:

comment:22 Changed 9 years ago by saliola

  • Status changed from needs_info to needs_review

comment:23 Changed 9 years ago by saliola

  • Status changed from needs_review to positive_review

comment:24 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:25 in reply to: ↑ 21 ; follow-up: Changed 9 years ago by jdemeyer

Replying to saliola:

If I understand correctly, the patchbot doesn't look at the description for the "apply" directive, so I put it in the comments.

I think that's correct yes.

Should I also include the directive in the description?

Yes please!

comment:26 in reply to: ↑ 25 Changed 9 years ago by saliola

Replying to jdemeyer:

Replying to saliola:

If I understand correctly, the patchbot doesn't look at the description for the "apply" directive, so I put it in the comments.

I think that's correct yes.

Should I also include the directive in the description?

Yes please!

Thank you. I'll make sure to do this from now on!

comment:27 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.3.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:28 Changed 9 years ago by jdemeyer

  • Authors changed from Franco Saliola, Nicolas Thiery, Florent Hivert, Chris Berg to Franco Saliola, Nicolas M. Thiéry, Florent Hivert, Chris Berg
Note: See TracTickets for help on using tickets.