Change sage.combinat.combinat.combinations() to use Combinations #13821

ppurka · 2012-12-11T15:21:21Z

As I mentioned a long time ago in #5288, the combinations() method is not ideal for working with Sage objects. This warning is also present in the documentation of this command. There is a much better command Combinations. The combinations command should be made to return a list by calling Combinations instead.

The result of this change in the code will be that

It will also speed up the function considerably, partly because it won't depend on string parsing from GAP.
It will be able to handle Sage objects much better.

Similarly, combinations_iterator should directly return a Combinations object, instead of duplicating stuff.

The attached patch performs this change.

Apply to devel/sage in this order:

Depends on #13723
Depends on #11763
Depends on #13503

CC: @sagetrac-sage-combinat @kini

Component: combinatorics

Author: Punarbasu Purkayastha

Reviewer: Travis Scrimshaw

Merged: sage-5.6.beta3

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

The text was updated successfully, but these errors were encountered:

ppurka · 2012-12-11T16:02:30Z

Author: Punarbasu Purkayastha

ppurka · 2012-12-11T16:02:30Z

comment:1

The attached patch implements this change.

tscrim · 2012-12-11T19:29:32Z

comment:2

Why not deprecate the function? Does this depend on #5288 getting merged into sage?

Thanks,

Travis

ppurka · 2012-12-12T01:02:17Z

comment:3

Replying to @tscrim:

Why not deprecate the function? Does this depend on #5288 getting merged into sage?

Thanks,

Travis

It could be deprecated. But I don't know why it was written in the first place. It doesn't depend on #5288 at all.

tscrim · 2012-12-15T07:04:28Z

comment:4

My guess is that it was from the old code (before Combinations and possibly before the categories model). I would deprecate the function since it is redundant and it just references Combinations. Also your INPUT: list bullets are indented too far and I would prefer the documentation to say returns or references rather than wrapper for for combinations_iterator. Thanks.

ppurka · 2012-12-15T16:28:55Z

comment:5

Ok. I have put deprecation in the functions and done a bunch of cleanups. There is a second patch which replaces all the calls to combinations* with Combinations, that i could find with search_src().

tscrim · 2012-12-15T18:31:08Z

Reviewer: Travis Scrimshaw

tscrim · 2012-12-15T18:31:08Z

comment:6

A few more documenation things:

The INPUT: for combintions_iterator() is still indented.
You have ..note:: but there should be a space there: .. NOTE:: (personally I prefer the capitalized form since it is consistent with the dev-guide, but if you prefer the lowercase, that's fine).
I don't like the formatting of

It is possible to take combinations of Sage objects.::

as it will output in the html

It is possible to take combinations of Sage objects.:

Thus either remove the period or put a space between the period and the colon (i.e. as objects. ::).

Thanks,

Travis

ppurka · 2012-12-15T20:35:54Z

Apply to devel/sage

ppurka · 2012-12-15T20:37:27Z

comment:7

Attachment: trac_13821-change_combinations.patch.gz

Thanks for the review. These issues should be fixed now.

Patchbot: apply trac_13821-change_combinations.patch trac_13821-replace_combinations.patch

tscrim · 2012-12-16T17:13:06Z

comment:8

Looks good to me. Thanks.

jdemeyer · 2012-12-22T07:01:56Z

Dependencies: #13723

jdemeyer · 2012-12-22T07:01:56Z

comment:9

This needs to be rebased on top of #13723.

ppurka · 2012-12-22T13:35:34Z

comment:10

Replying to @jdemeyer:

This needs to be rebased on top of #13723.

This should be fixed now. Only change needed was the removal of an import.

jdemeyer · 2012-12-30T12:11:13Z

comment:11

There are two doctest failures coming from #11763:

sage -t  -force_lib devel/sage/sage/geometry/polyhedron/base_ZZ.py
**********************************************************************
File "/release/merger/sage-5.6.beta2/devel/sage-main/sage/geometry/polyhedron/base_ZZ.py", line 227:
    sage: list( P.fibration_generator(2) )
Expected:
    [A 2-dimensional polyhedron in ZZ^4 defined as the convex hull of 3 vertices]
Got:
    doctest:239: DeprecationWarning: Use Combinations(mset,k) instead.
    See http://trac.sagemath.org/13821 for details.
    [A 2-dimensional polyhedron in ZZ^4 defined as the convex hull of 3 vertices]
**********************************************************************

and

sage -t  -force_lib devel/sage/sage/graphs/generic_graph.py
**********************************************************************
File "/release/merger/sage-5.6.beta2/devel/sage-main/sage/graphs/generic_graph.py", line 11862:
    sage: G.triangles_count()
Expected:
    0
Got:
    doctest:11913: DeprecationWarning: Use Combinations(mset,k) instead.
    See http://trac.sagemath.org/13821 for details.
    0
**********************************************************************

jdemeyer · 2012-12-30T12:11:13Z

Changed dependencies from #13723 to #13723, #11763

ppurka · 2012-12-30T16:36:18Z

Attachment: trac_13821-replace_combinations.patch.gz

Apply to devel/sage after above patch

ppurka · 2012-12-30T16:37:59Z

comment:12

Updated the patch. I will set it to positive review if the patchbot turns green with envy.

jdemeyer · 2012-12-31T09:41:51Z

comment:13

ppurka: this is a change which requires an actual review.

ppurka · 2012-12-31T09:51:59Z

comment:14

Replying to @jdemeyer:

ppurka: this is a change which requires an actual review.

I don't mind. But the patchbot seems to have failed in exactly the same position. This is unexpected. I didn't see any other use of combination from search_src().

Moreover, my local testing gave no errors on those files.

ppurka · 2012-12-31T09:58:34Z

comment:15

From the patchbot logs:

2012-12-30 23:17:49 +0000
Looking at #13821
Looking at #13723
#13723 already applied (5.6.beta0 <= 5.6.beta1)
Looking at #11763
#11763 already applied (5.6.beta1 <= 5.6.beta1)
Patches for #13821:
    trac_13821-change_combinations.patch#06e5f20c6df61a1803698a5b80ee0240
    trac_13821-replace_combinations.patch#5356795e4cae7f89cd58c17dbe122f7f
hg qpop trac_13821-replace_combinations.patch
qpop: trac_13821-replace_combinations.patch is already at the top
hg qapplied
trac_13821-change_combinations.patch
trac_13821-replace_combinations.patch

Why is it using the old patches? Shouldn't it be trying with the new patches?

ppurka · 2012-12-31T12:03:04Z

comment:16

patchbot apply trac_13821-change_combinations.patch trac_13821-replace_combinations.patch

tscrim · 2012-12-31T17:36:17Z

comment:17

Works for me as well:

travis@travis-virtualbox:~/sage-5.5.rc0/devel/sage-reviews/sage$ sage -t graphs/generic_graph.py geometry/polyhedron/base_ZZ.py 
sage -t  "devel/sage-reviews/sage/geometry/polyhedron/base_ZZ.py"
	 [24.3 s]
sage -t  "devel/sage-reviews/sage/graphs/generic_graph.py"  
	 [91.9 s]
 
----------------------------------------------------------------------
All tests passed!
Total time for all tests: 116.3 seconds

(For whomever is interested, the replace patch depends on #13503)

I've had this issue of the patchbot using old patches come up a few places before. I've kicked the patchbot, so hopefully next time the patchbot will pass all tests. Since the tests pass for me, I'm going to set this to positive review.

ppurka · 2012-12-31T17:42:05Z

comment:18

Thanks.

I didn't know it was a more common occurrence with the patchbot. It looks like it doesn't clear the queue while applying new patches.

ppurka · 2012-12-31T17:43:41Z

comment:19

@kini - can you have a look at the patchbot? I think you are one of the few people who know how it works.

kini · 2012-12-31T18:36:35Z

comment:20

Hmm, I have no idea. I tried running my 5.6.beta0 patchbot on it but it failed even more strangely. Building 5.6.beta1 now to try that...

tscrim · 2013-01-01T05:35:24Z

comment:21

Replying to @kini:

Hmm, I have no idea. I tried running my 5.6.beta0 patchbot on it but it failed even more strangely. Building 5.6.beta1 now to try that...

Did it fail to apply? My guess is that is because #13503 was merged in 5.6.beta1. I've formally added it as a dependency.

tscrim · 2013-01-01T05:35:24Z

Changed dependencies from #13723, #11763 to #13723, #11763, #13503

ppurka · 2013-01-01T10:40:32Z

comment:22

It should apply fine to beta0, since it applies fine to beta1. Anyway, the patchbot seems to have caught up or kini did some miracle! :)

kini · 2013-01-01T19:05:01Z

comment:23

I didn't do anything. Vanilla 5.6.beta1 doesn't even pass tests on my machine, and my patchbot failed to apply #11763 to beta0 for some reason. Volker's patchbot is the one that came along and gave this ticket a green light.

jdemeyer · 2013-01-07T20:56:34Z

Merged: sage-5.6.beta3

ppurka added this to the sage-5.6 milestone Dec 11, 2012

ppurka added c: combinatorics labels Dec 11, 2012

ppurka assigned sagetrac-sage-combinat Dec 11, 2012

This comment has been minimized.

Sign in to view

ppurka added the s: needs review label Dec 11, 2012

tscrim added s: needs info and removed s: needs review labels Dec 11, 2012

This comment has been minimized.

Sign in to view

ppurka added s: needs review and removed s: needs info labels Dec 15, 2012

tscrim added s: positive review and removed s: needs review labels Dec 16, 2012

jdemeyer added s: needs work and removed s: positive review labels Dec 22, 2012

ppurka added s: positive review and removed s: needs work labels Dec 22, 2012

jdemeyer added s: needs work and removed s: positive review labels Dec 30, 2012

ppurka added s: needs review and removed s: needs work labels Dec 30, 2012

tscrim added s: positive review and removed s: needs review labels Dec 31, 2012

jdemeyer removed the s: positive review label Jan 7, 2013

jdemeyer closed this as completed Jan 7, 2013

dcoudert mentioned this issue Jan 12, 2013

Gromov hyperbolicity of graphs #13808

Closed

nathanncohen mannequin mentioned this issue Mar 6, 2014

Remove deprecated code from combinat/ #15466

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change sage.combinat.combinat.combinations() to use Combinations #13821

Change sage.combinat.combinat.combinations() to use Combinations #13821

ppurka commented Dec 11, 2012

ppurka commented Dec 11, 2012

ppurka commented Dec 11, 2012

This comment has been minimized.

tscrim commented Dec 11, 2012

ppurka commented Dec 12, 2012

tscrim commented Dec 15, 2012

ppurka commented Dec 15, 2012

This comment has been minimized.

tscrim commented Dec 15, 2012

tscrim commented Dec 15, 2012

ppurka commented Dec 15, 2012

ppurka commented Dec 15, 2012

tscrim commented Dec 16, 2012

jdemeyer commented Dec 22, 2012

jdemeyer commented Dec 22, 2012

ppurka commented Dec 22, 2012

jdemeyer commented Dec 30, 2012

jdemeyer commented Dec 30, 2012

ppurka commented Dec 30, 2012

ppurka commented Dec 30, 2012

jdemeyer commented Dec 31, 2012

ppurka commented Dec 31, 2012

ppurka commented Dec 31, 2012

ppurka commented Dec 31, 2012

tscrim commented Dec 31, 2012

ppurka commented Dec 31, 2012

ppurka commented Dec 31, 2012

kini commented Dec 31, 2012

tscrim commented Jan 1, 2013

tscrim commented Jan 1, 2013

ppurka commented Jan 1, 2013

kini commented Jan 1, 2013

jdemeyer commented Jan 7, 2013

Change sage.combinat.combinat.combinations() to use Combinations #13821

Change sage.combinat.combinat.combinations() to use Combinations #13821

Comments

ppurka commented Dec 11, 2012

ppurka commented Dec 11, 2012

ppurka commented Dec 11, 2012

This comment has been minimized.

tscrim commented Dec 11, 2012

ppurka commented Dec 12, 2012

tscrim commented Dec 15, 2012

ppurka commented Dec 15, 2012

This comment has been minimized.

tscrim commented Dec 15, 2012

tscrim commented Dec 15, 2012

ppurka commented Dec 15, 2012

ppurka commented Dec 15, 2012

tscrim commented Dec 16, 2012

jdemeyer commented Dec 22, 2012

jdemeyer commented Dec 22, 2012

ppurka commented Dec 22, 2012

jdemeyer commented Dec 30, 2012

jdemeyer commented Dec 30, 2012

ppurka commented Dec 30, 2012

ppurka commented Dec 30, 2012

jdemeyer commented Dec 31, 2012

ppurka commented Dec 31, 2012

ppurka commented Dec 31, 2012

ppurka commented Dec 31, 2012

tscrim commented Dec 31, 2012

ppurka commented Dec 31, 2012

ppurka commented Dec 31, 2012

kini commented Dec 31, 2012

tscrim commented Jan 1, 2013

tscrim commented Jan 1, 2013

ppurka commented Jan 1, 2013

kini commented Jan 1, 2013

jdemeyer commented Jan 7, 2013