Skip to content
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

ENH: use math.factorial for exact factorials #5693

Merged
merged 1 commit into from
Jan 8, 2016
Merged

Conversation

ewmoore
Copy link
Member

@ewmoore ewmoore commented Jan 8, 2016

math.factorial is substantially faster than the naive multiplication algorithm.

In [89]: timeit special.factorial(100, True)
100000 loops, best of 3: 9.1 µs per loop

In [90]: timeit math.factorial(100)
1000000 loops, best of 3: 1.43 µs per loop

Ideally i think that special.factorial wouldn't be a thing, but since it is, there isn't any good reason to use a poor algorithm when a better one already exists.

When n isn't an integer, this will still raise a ValueError, but the text will be different.

math.factorial is substantially faster than the naive multiplication
algorithm.
@argriffing
Copy link
Contributor

Looks good to me if it passes the CI. According to the python docs math.factorial() was new in 2.6 so this should be OK.

@codecov-io
Copy link

@@            master   #5693   diff @@
======================================
  Files          234     234       
  Stmts        43096   43092     -4
  Branches      8154    8152     -2
  Methods          0       0       
======================================
- Hit          33410   33408     -2
+ Partial       2605    2604     -1
+ Missed        7081    7080     -1

Review entire Coverage Diff as of f6012da

Powered by Codecov. Updated on successful CI builds.

@rgommers rgommers added this to the 0.18.0 milestone Jan 8, 2016
rgommers added a commit that referenced this pull request Jan 8, 2016
ENH: use math.factorial for exact factorials
@rgommers rgommers merged commit 14ac4f7 into scipy:master Jan 8, 2016
@rgommers
Copy link
Member

rgommers commented Jan 8, 2016

LGTM too, merged.

@endolith
Copy link
Member

The docstring says If n<0, the return value is 0. but this is no longer true:

factorial(-1, True)
Traceback (most recent call last):
...
ValueError: factorial() not defined for negative values

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants