Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Our records indicate we have not received your CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

Note that this is a work in progress; currently, it only contains tests specifying the behaviour of the function, so we can more easily discuss what's the desired behaviour.

Let's name this comb() instead of binomial() please (as requested by me, Mark, and Tim).

Can we work on PR #11414 since it already passes all tests? Added you as a collaborator.

Also, Is it okay to have multiple methods for tests of a single function? All other function in math have their tests encased in a single method which seems easier to maintain.

Is it okay to have multiple methods for tests of a single function?

I would assume as much: I modelled those tests after the ones for factorial, which follow exactly that model (single function per property, with an informative name)

@FR4NKESTI3N The main reasons tests are failing is that the I had to reconcile the testsuite in one branch with the implemented behavior in the other, which would have to be done regardless of which branch we are pushing with.
It's now done (I had initially missed one test where k>n), and tests now pass.

I pushed to this PR rather than #11414 because this was the one I had write access to :)

@csabella, @pablogsal, @scoder: Taking the liberty to ping you, since you commented on #11414

This is the PR and testsuite I started during the discussion on bugs.python.org, and I merged in @FR4NKESTI3N's C implementation that you already reviewed. The principal differences are:

• minor improvements to the exception messages;
• the testsuite for math.comb covers some extra edge cases, and is split in independent functions (as for math.factorial), which are documented by their docstring.

#11414 was already conflict free and had function name as comb, you needlessly worked by naming and renaming etc. The changes that you made in last 7-8 commits were already there. You were already invited as collaborator.
The test method names still need to be changed to testComb. The factorial test suite has 3 functions and is pretty old, and one of those methods needed a decorator. 6+ methods are bound to cause maintenance problems in the future.
Some exception tests are still missing and "combinations" isstill written everywhere except tests. Renaming and writing them all will be useless effort as all that is already done.
I would still insist you to work on that PR or merge that branch here again. Or maybe make the changes in that branch and then merge it with this. There were just some changes in 2 or 3 comments so it will be much more simpler to work on that branch.

@FR4NKESTI3N

#11414 was already conflict free and had function name as comb, you needlessly worked by naming and renaming etc. The changes that you made in last 7-8 commits were already there. You were already invited as collaborator.

Regarding “the last 7-8 commits” you talk of, I have no idea what you are going off, given that all I added since the initial merge 4 days ago (before you commented on this PR or added me as a collaborator) were commits afb3d36 (trivial changes, fixes the testsuite) and e4942bf (trivial rename).

Not sure either what you mean by “conflict-free” since there's no conflict between this and master (nor are there ever been).

On the other hand, recreating all that work on your PR would indeed be unnecessary work.

The test method names still need to be changed to testComb

Not sure that's entirely necessary, but I just did that to avoid having an argument, since it's a quick search-and-replace.

The factorial test suite has 3 functions and is pretty old, and one of those methods needed a decorator. 6+ methods are bound to cause maintenance problems in the future.

I am absolutely counfunded why you would expect a single function, testing a bunch of unrelated properties, to be more maintainable than separate tests.

I assure you, cramming everything into a single test does not make for maintainable testsuites.

Some exception tests are still missing

Which ones? I am pretty sure I kept them all when merging both testsuites.

"combinations" isstill written everywhere except tests. Renaming and writing them all will be useless effort as all that is already done.

Clearly not:

$ grep -i combinations Modules/mathmodule.c Lib/test/test_math.py
[Empty output, no match found]

I would still insist you to work on that PR or merge that branch here again. Or maybe make the changes in that branch and then merge it with this.

All the things you mentionned existed all along (with the exception of a single search-and-replace for test names), so I'm not sure why you want me to recreate all that on the other PR.

There were just some changes in 2 or 3 comments so it will be much more simpler to work on that branch.

There were changes to the testsuite, which was the whole point of that merge in the first place.

Hmm yeah, I just wanted to say said that it was wasteful. I think I was looking at an earlier commit, so I though that re-merging would be better.
Anyway fixed the name in other files.
Still not sure about unittests though but I am no expert.
Should we start on optimizations?
And what about math.perm()

@FR4NKESTI3N I think math.perm ought to be a separate PR.
Regarding optimisations, like @rhettinger I'd rather get this reviewed/merged before we submit optimized code, but it doesn't mean we can't already do the legwork while this goes through the review.

Also thanks for noticing I had missed the documentation page :)

PS: Just pushed a commit documenting under which circumstances math.comb may raise OverflowError.

Having LLONG_MAX doesn't seem right in documentation, maybe we can use something along the lines of "size of long long provided by compiler". This detail is purely implementation based anyway, so I don't know if it should even be there in documentation.

Edit: its minimum of the two btw

@FR4NKESTI3N Good point; we might just mention there is a size beyond which math.comb is guaranteed to raise OverflowError.

The documentation for the math module mentions that things may raise OverflowError, but that's in the context of float overflow, which is a quite different beast altogether.

@KellerFuchs R.Hettinger reviewed the other PR so I have made the changes there. There was some change in implementation so I haven't written anything in docs about OverflowError for until Hettinger decides on implementation.

Please note that the CLA has still not been signed. Without it we cannot accept this PR.

@brettcannon Oh, thanks for the reminder, looks like the CLA signing thing didn't go through properly the first time around (I had some issues with DocuSign); I just redid the thing, so it should be good tomorrow.

PS: Also rebased the branch to deal with the merge conflict.

Closing in favor of PR 11414. Any remaining ideas from this PR can be brought up in a separate PR.

Thanks for the parallel efforts.