use the faster binomial in combinat #39385
Conversation
There was a problem hiding this comment.
Looks good, a suggestion and a question before I approve.
Suggestion: I think some of your casts to ZZ/Integer might be redundant, in that I think it's already set to be a Sage integer by the parent/elsewhere. The redundant casts to ZZ adds a non-negligible performance cost so remove those if they aren't needed. (a = Integer(5) takes about 215 ns on my machine, a = b where b = ZZ(5) takes about 15 ns).
Question: What is the difference between ZZ(x) and Integer(x)? In some places you use ZZ(x) and in some places you changed ZZ(x) to Integer(x).
| @@ -77,13 +76,13 @@ def __init__(self, parent, lst, check=True): | |||
| """ | |||
| if check: | |||
| lst = [ZZ(val) for val in lst] | |||
| self.k = parent.k | |||
| self.k = ZZ(parent.k) | |||
There was a problem hiding this comment.
Is this ZZ call necessary? In the __init__ for AffinePermutationGroupGeneric, you have self.k = ZZ(ct.n) so I think parent.k will always be a Sage integer unless there's some other code path that bypasses the existing cast.
Even in Sage 10.5 I don't see how to construct this object in a way that you don't get a Sage integer here.
| @@ -332,7 +332,7 @@ def __init__(self, data, frozen=None, is_principal=False, user_labels=None, user | |||
| else: | |||
| labelset = set(user_labels) | |||
| # Sanitizes our ``user_labels`` to use Integers instead of ints | |||
| user_labels = [ZZ(x) if x in ZZ else x for x in user_labels] | |||
| user_labels = [Integer(x) if x in ZZ else x for x in user_labels] | |||
There was a problem hiding this comment.
What's the purpose of this change?
There was a problem hiding this comment.
this is marginally faster
There was a problem hiding this comment.
Then shouldn't we be using Integer throughout this PR instead of ZZ?
|
Documentation preview for this PR (built with commit db52fb0; changes) is ready! 🎉 |
| @@ -2014,13 +2014,13 @@ def __init__(self, cartan_type): | |||
| """ | |||
| Parent.__init__(self, category=AffineWeylGroups()) | |||
| ct = CartanType(cartan_type) | |||
| self.k = ct.n | |||
| self.k = Integer(ct.n) | |||
There was a problem hiding this comment.
Same question as earlier: from my reading of the code, ct.n will always be a Sage integer, so this cast should be unnecessary. Is there any reason you have Integer(ct.n) here instead of ct.n? I did some timing experiments and calling Integer does slow things down, and from the PR title this is supposed to be an optimization PR.
There are a few other locations in the code where you're calling Integer on something that I think is already going to be a Sage integer.
|
I have double-checked "poset", "pieri_factors" and "shuffle_product" and The Integer around |
All I wanted was to double-check that the uses of LGTM! |
as the binomial method of integers is way faster than the general
purpose binomial from `arith.misc`
also a few other minor code changes
### 📝 Checklist
- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
URL: sagemath#39385
Reported by: Frédéric Chapoton
Reviewer(s): Frédéric Chapoton, Vincent Macri
as the binomial method of integers is way faster than the general
purpose binomial from `arith.misc`
also a few other minor code changes
### 📝 Checklist
- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
URL: sagemath#39385
Reported by: Frédéric Chapoton
Reviewer(s): Frédéric Chapoton, Vincent Macri
as the binomial method of integers is way faster than the general
purpose binomial from `arith.misc`
also a few other minor code changes
### 📝 Checklist
- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
URL: sagemath#39385
Reported by: Frédéric Chapoton
Reviewer(s): Frédéric Chapoton, Vincent Macri
as the binomial method of integers is way faster than the general
purpose binomial from `arith.misc`
also a few other minor code changes
### 📝 Checklist
- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
URL: sagemath#39385
Reported by: Frédéric Chapoton
Reviewer(s): Frédéric Chapoton, Vincent Macri
as the binomial method of integers is way faster than the general purpose binomial from
arith.miscalso a few other minor code changes
📝 Checklist