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

improving speed of factorial function #1170

Merged
merged 10 commits into from Jul 16, 2018

Conversation

Projects
None yet
3 participants
@honeybar
Contributor

honeybar commented Jul 15, 2018

Hi, I modified factorial functions in gamma.js, permutation.js and combination.js for regular number to speed up the run time by modifying how product is calculated. (see: productrange.js for more info on how it is done). I tried to implement the same algorithm for big number but it ends up slower compared to the original, which is a bit of a surprise since, on regular number, the trend is that it will outperform the original method. I am still rather new to open source contribution so feedback is appreciated.

Thank you.

here is the log of performance for permutation function original vs new:

ans for original algorithm for 50P25: 1.9607814681608194e+39
2.525ms
ans for newer algorithm for 50P25: 1.9607814681608194e+39
0.187ms

here is one for combination function:

ans for original algorithm for 50C25: 126410606437752
11.590ms
ans for new algorithm for 50C25: 126410606437752
0.686ms

here is one for factorial:

ans for original algorithm for 50! : 3.0414093201713376e+64
2.647ms
ans for new algorithm for 50! : 3.0414093201713376e+64
0.703ms

honeybar added some commits Jul 15, 2018

Improving factorial function
use divide and conquer to improve factorial for both number and bignumber
improving factorial function
improving speed of bignumber and number
improving factorial function
speed up factorial function for number

@josdejong josdejong changed the base branch from master to develop Jul 15, 2018

@josdejong

This comment has been minimized.

Show comment
Hide comment
@josdejong

josdejong Jul 15, 2018

Owner

wow, that's impressive!!! Thanks a lot @honeybar .

I think your work is ready to be merged as is, though I have a few remarks:

  1. How about renaming the function productrange.js to product.js since that is the only function that is in there?
  2. Can you write some (minimal) comments on top of the function product to explain what it does and what goes in and out?
  3. It would be great if you could improve the performance of the BigNumber implementations too, would that be doable?
Owner

josdejong commented Jul 15, 2018

wow, that's impressive!!! Thanks a lot @honeybar .

I think your work is ready to be merged as is, though I have a few remarks:

  1. How about renaming the function productrange.js to product.js since that is the only function that is in there?
  2. Can you write some (minimal) comments on top of the function product to explain what it does and what goes in and out?
  3. It would be great if you could improve the performance of the BigNumber implementations too, would that be doable?
@josdejong

This comment has been minimized.

Show comment
Hide comment
@josdejong

josdejong Jul 15, 2018

Owner

I tried to implement the same algorithm for big number but it ends up slower compared to the original,

ah sorry, now I see. Forget about my point 3.

Owner

josdejong commented Jul 15, 2018

I tried to implement the same algorithm for big number but it ends up slower compared to the original,

ah sorry, now I see. Forget about my point 3.

@harrysarson

Looks good, just a quick question 👍

Show outdated Hide outdated src/function/probability/productrange.js Outdated

honeybar added some commits Jul 15, 2018

@josdejong

This comment has been minimized.

Show comment
Hide comment
@josdejong

josdejong Jul 15, 2018

Owner

Thanks for the updates @honeybar .

Just curious: have you also tried a plain old for loop to calculate the product, instead of the recursive solution that you have now?

Owner

josdejong commented Jul 15, 2018

Thanks for the updates @honeybar .

Just curious: have you also tried a plain old for loop to calculate the product, instead of the recursive solution that you have now?

@honeybar

This comment has been minimized.

Show comment
Hide comment
@honeybar

honeybar Jul 15, 2018

Contributor

@josdejong I haven't tried converting the divide and conquer solution to for-loop. I think it might be possible, but it would be rather complex to do so, since divide and conquer is inherently recursive process.

Contributor

honeybar commented Jul 15, 2018

@josdejong I haven't tried converting the divide and conquer solution to for-loop. I think it might be possible, but it would be rather complex to do so, since divide and conquer is inherently recursive process.

@josdejong

This comment has been minimized.

Show comment
Hide comment
@josdejong

josdejong Jul 16, 2018

Owner

Thanks for your latest updates @honeybar!

Owner

josdejong commented Jul 16, 2018

Thanks for your latest updates @honeybar!

@josdejong josdejong merged commit d67c374 into josdejong:develop Jul 16, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json No dependency changes
Details
@josdejong

This comment has been minimized.

Show comment
Hide comment
@josdejong

josdejong Jul 22, 2018

Owner

The improved function factorial is now available in v5.0.4, thanks again!

Owner

josdejong commented Jul 22, 2018

The improved function factorial is now available in v5.0.4, thanks again!

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