-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-35431: Implemented math.comb #11414
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use Jebelean’s exact division algorithm to speed up the division as if you factor out the coefficients as the product of(n-k+1)* PROD(i,2,k,(n-k+i)/i), i will always divide the already calculated product.
@pablogsal I haven't started working on optimization yet since Raymond Hettinger suggested to wait till everyone has agreed on the behaviour : https://bugs.python.org/issue35431#msg332838 I was also considering using prime factorization for smaller k values : https://dl.acm.org/citation.cfm?id=26272 but seems like an overkill now |
This is the result of merging python#11018 and python#11414
@FR4NKESTI3N Thanks a lot for starting on this. I merged our PRs in #11018, as the testsuite I had started there was more complete (and used as the basis for discussing the desired behaviour, in the bugs.python.org issue). |
PS: I added you as a collaborator on my repository, so if you accept the invite you can push directly to that PR's branch. |
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I have made the requested changes; please review again |
Thanks for making the requested changes! @rhettinger: please review the changes made to this pull request. |
This is the result of merging python#11018 and python#11414
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Use
PyNumber_Index()
for converting arguments to Python integers. Seegcd()
for example. - Do not convert
n
to C long long.comb(2**1000, 5)
should work. - Do not use
PyNumber_Long()
.
It is convenient to implement perm(n, k)
first. comb(n, k)
can be easily implemented as perm(n, k) // factorial(k)
(or perm(n, n-k) // factorial(n-k)
if k > n-k
).
@serhiy-storchaka So boilerplate should be changed back to have n and k as objects right? |
…nversion to long long.
Implemented the function math.comb to calculate binomial coefficient. Simple O(k) algorithm is used for now. ValueError if condition 0 <= k <= n is not satisfied and TypeError if non-integer arguments passed to function.
https://bugs.python.org/issue35431