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

Implement np.lcm #4904

Merged
merged 2 commits into from Dec 9, 2019
Merged

Implement np.lcm #4904

merged 2 commits into from Dec 9, 2019

Conversation

ivirshup
Copy link
Contributor

Building on #4780, this PR implements np.lcm as a ufunc. It's very similar to that PR, and uses much of the same boiler-plate.

@ivirshup
Copy link
Contributor Author

@esc, I think this is ready for review – though that might be easier once the gcd branch merges. The only relevant commits here are the most recent two. Two notes:

  • This doesn't have overflow guards, but neither does numpy.
  • I've only tested this like the other gufuncs are tested, but this method also doesn't exist in math, so there are fewer tests running on the underlying implementation.

Also, here is a quick benchmark:

import numpy as np
from numba import njit

def lcm(a, b):
    np.lcm(a, b)

nlcm = njit(lcm)
plcm = njit(parallel=True)(lcm)

a = np.random.randint(0, 10000000, 10000)  
b = np.random.randint(0, 10000000, 10000)

# warmups
nlcm(a, b) 
plcm(a, b)

%timeit lcm(a, b)
1.45 ms ± 22.4 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

%timeit nlcm(a, b)
591 µs ± 6.99 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

%timeit plcm(a, b)
133 µs ± 14.4 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

@esc
Copy link
Member

esc commented Nov 29, 2019

@ivirshup thanks for updating this, I have changed the label of the PR accordingly. The benchmarks look impressive from a first glance. 👍

@stuartarchibald
Copy link
Contributor

/AzurePipelines run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

@stuartarchibald
Copy link
Contributor

@ivirshup #4780 is merged if you want to rebase/merge in?

@stuartarchibald stuartarchibald added this to the Numba 0.47 RC milestone Dec 4, 2019
@ivirshup
Copy link
Contributor Author

ivirshup commented Dec 5, 2019

@stuartarchibald, done rebasing

Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Looks good.

@stuartarchibald stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge and removed 3 - Ready for Review labels Dec 5, 2019
@seibert seibert merged commit 88c459f into numba:master Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants