Cleanup in choose_nk, split_nk, subword, subset

[videlec]

• deprecate ChooseNK (in sage.combinat.choose_nk) and SplitNK (in sage.combinat.split_nk).
• clean sage.combinat.subword and sage.combinat.subset
  • inheritance from Parent
  • much better iteration/random generation
  • ...

And become more pep8 compliant ;=)

see also: #16472

CC: @sagetrac-sage-combinat

Component: combinatorics

Keywords: generation, set, subset, partition

Author: Vincent Delecroix, Frédéric Chapoton

Branch/Commit: 714f266

Reviewer: Florent Hivert, Frédéric Chapoton, Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/10534

[hivert]

Changed author from vdelecroix to Vincent Delecroix

[hivert]

Reviewer: Florent Hivert

[hivert]

comment:9

Hi Vincent,

Your patch looks good except a few remarks:

• the field Author below is for credits. You should put your real name and
  not your login.

• the documentation now says:

  [...]
  The elements of the output are tuples of Python int (and not Sage Integer).
  [...]
  It's element are returned as plain list of python int.
  

  At least one of those two sentences is wrong. Also they seems a little
  redundent. This should be fixed.

I'm sorry for not having the time to put a review patch, nor finish the review
now.

[videlec]

comment:10

• the field Author below is for credits. You should put your real name and
  not your login.

Thank you for modifying this.

• the documentation now says:

  [...]
  The elements of the output are tuples of Python int (and not Sage Integer).
  [...]
  It's element are returned as plain list of python int.
  

That's right. Output value are now tuples and it is fully specified.

[hivert]

comment:11

Attachment: trac_10534-generation_of_subsets_and_set_partitions.patch.gz

Hi Vincent,

I need more time to review your long patch. I just notice the following:

file: sage/combinat/choose_nk.py
85	            sage: [0,2] in c52   # trac XXXX 

You probably want to replace XXXX by an actual number. I'll continue my review tomorrow (hopefully)

Florent

[videlec]

comment:12

Hi Florent

I need more time to review your long patch.

Certainly. Thanks for the review.

I have three important design questions relative to that patch and the Combinatorial classes which it modify

• should the attributes of a CombinatorialClass be visible like S.w or do we prefer S._w (some code in graphs uses a direct call to S.w) ?
• what should be the inheritance/relations between say Subsets (subset of a given set) and Subsets_k (subset of size k of a given set) ?
• what should be the output of Subwords ? tuple, list, strings, any user defined type ? Actually, if you initialize it with list (resp. tuple, string) it output lists (resp. tuple, string).

I just notice the following:

file: sage/combinat/choose_nk.py
85	            sage: [0,2] in c52   # trac XXXX 

You probably want to replace XXXX by an actual number.

Done

[videlec]

comment:14

Nathan, you should be more explicit when asking for more info...

[vbraun]

comment:52

sage -t --long src/sage/misc/sageinspect.py  # 1 doctest failed

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

d9637bd

Trivial doctest fix due to line number changes.

[sagetrac-git]

Changed commit from 9146bde to d9637bd

[vbraun]

comment:55

Try running the testsuite...

[tscrim]

comment:56

I did (again):

travis@travis-sage:~/sage/src/sage/misc$ sage -tp sageinspect.py 
Running doctests with ID 2014-06-15-09-38-19-7033a8ad.
Doctesting 1 file using 4 threads.
sage -t sageinspect.py
    [251 tests, 24.24 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 24.6 seconds
    cpu time: 4.7 seconds
    cumulative wall time: 24.2 seconds
travis@travis-sage:~/sage/src/sage/misc$ sage -tp --long sageinspect.py 
Running doctests with ID 2014-06-15-09-40-50-0dd08e9e.
Doctesting 1 file using 4 threads.
sage -t --long sageinspect.py
    [251 tests, 24.16 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 24.5 seconds
    cpu time: 4.7 seconds
    cumulative wall time: 24.2 seconds

Is there a failure elsewhere (I also checked the failures that the patchbot reported and those passed)?

[videlec]

comment:57

Same problem as in #16472

sage -t --long structure/sage_object.pyx
**********************************************************************
File "structure/sage_object.pyx", line 1346, in sage.structure.sage_object.unpickle_all
Failed example:
    sage.structure.sage_object.unpickle_all()  # (4s on sage.math, 2011)
Expected
...
Got:
...
    Failed:
    _class__sage_combinat_choose_nk_ChooseNK__.sobj
    _class__sage_combinat_split_nk_SplitNK_nk__.sobj

If the approach with __setstate__ is more standard then we should go for it here as well.

[tscrim]

comment:58

Will you be taking care of it Vincent or do you want me to?

[videlec]

comment:59

Replying to @tscrim:

Will you be taking care of it Vincent or do you want me to?

No, no. I will do it along your lines. Thanks for proposing.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

714f266

trac #10534: fix pickling

[sagetrac-git]

Changed commit from d9637bd to 714f266

[videlec]

comment:61

Hey Travis,

Please double check that everything is fine.

Vincent

[tscrim]

comment:62

LGTM. Thanks Vincent.

[vbraun]

Changed branch from public/10534 to 714f266