Opened 8 years ago
Closed 8 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: |
Description (last modified by )
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:
Attachments (2)
Change History (26)
comment:1 Changed 8 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 8 years ago by
- 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 8 years ago by
comment:4 Changed 8 years ago by
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 8 years ago by
- 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 8 years ago by
- Reviewers set to Travis Scrimshaw
A few more documenation things:
- The
INPUT:
forcombintions_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 htmlIt 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. asobjects. ::
).
Thanks,
Travis
comment:7 Changed 8 years ago by
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 8 years ago by
- Status changed from needs_review to positive_review
Looks good to me. Thanks.
comment:9 follow-up: ↓ 10 Changed 8 years ago by
- 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 8 years ago by
- 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 8 years ago by
- 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 **********************************************************************
comment:12 Changed 8 years ago by
- 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: ↓ 14 Changed 8 years ago by
ppurka: this is a change which requires an actual review.
comment:14 in reply to: ↑ 13 Changed 8 years ago by
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.
comment:15 Changed 8 years ago by
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 8 years ago by
patchbot apply trac_13821-change_combinations.patch trac_13821-replace_combinations.patch
comment:17 Changed 8 years ago by
- 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 8 years ago by
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 8 years ago by
- 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: ↓ 21 Changed 8 years ago by
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 8 years ago by
- Dependencies changed from #13723, #11763 to #13723, #11763, #13503
comment:22 Changed 8 years ago by
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 8 years ago by
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 8 years ago by
- Merged in set to sage-5.6.beta3
- Resolution set to fixed
- Status changed from positive_review to closed
The attached patch implements this change.