Opened 9 years ago

Closed 9 years ago

#13821 closed enhancement (fixed)

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

Reported by: ppurka Owned by: sage-combinat
Priority: major Milestone: sage-5.6
Component: combinatorics Keywords:
Cc: sage-combinat, kini Merged in: sage-5.6.beta3
Authors: Punarbasu Purkayastha Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13723, #11763, #13503 Stopgaps:

Status badges

Description (last modified by ppurka)

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

  1. It will also speed up the function considerably, partly because it won't depend on string parsing from GAP.
  2. 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:

  1. trac_13821-change_combinations.patch
  2. trac_13821-replace_combinations.patch

Attachments (2)

trac_13821-change_combinations.patch (9.6 KB) - added by ppurka 9 years ago.
Apply to devel/sage
trac_13821-replace_combinations.patch (5.8 KB) - added by ppurka 9 years ago.
Apply to devel/sage after above patch

Download all attachments as: .zip

Change History (26)

comment:1 Changed 9 years ago by ppurka

  • Authors set to Punarbasu Purkayastha
  • Description modified (diff)
  • Status changed from new to needs_review

The attached patch implements this change.

comment:2 follow-up: Changed 9 years ago by tscrim

  • Status changed from needs_review to needs_info

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

Thanks,
Travis

comment:3 in reply to: ↑ 2 Changed 9 years ago by ppurka

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.

comment:4 Changed 9 years ago by tscrim

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.

comment:5 Changed 9 years ago by ppurka

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

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().

comment:6 Changed 9 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

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

Changed 9 years ago by ppurka

Apply to devel/sage

comment:7 Changed 9 years ago by ppurka

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

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

comment:8 Changed 9 years ago by tscrim

  • Status changed from needs_review to positive_review

Looks good to me. Thanks.

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

  • Dependencies set to #13723
  • Status changed from positive_review to needs_work

This needs to be rebased on top of #13723.

comment:10 in reply to: ↑ 9 Changed 9 years ago by ppurka

  • Status changed from needs_work to positive_review

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.

comment:11 Changed 9 years ago by jdemeyer

  • Dependencies changed from #13723 to #13723, #11763
  • Status changed from positive_review to needs_work

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
**********************************************************************

Changed 9 years ago by ppurka

Apply to devel/sage after above patch

comment:12 Changed 9 years ago by ppurka

  • Status changed from needs_work to needs_review

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

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

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

comment:14 in reply to: ↑ 13 Changed 9 years ago by ppurka

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.

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

comment:15 Changed 9 years ago by ppurka

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?

comment:16 Changed 9 years ago by ppurka

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

comment:17 Changed 9 years ago by tscrim

  • Status changed from needs_review to positive_review

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.

comment:18 Changed 9 years ago by ppurka

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.

comment:19 Changed 9 years ago by ppurka

  • Cc kini added

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

comment:20 follow-up: Changed 9 years ago by 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...

comment:21 in reply to: ↑ 20 Changed 9 years ago by tscrim

  • Dependencies changed from #13723, #11763 to #13723, #11763, #13503

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.

comment:22 Changed 9 years ago by ppurka

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! :)

comment:23 Changed 9 years ago by kini

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.

comment:24 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.6.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.