Add support for np.correlate and np.convolve #2777
Conversation
As title. Lack of FFT support prevents the implementation of efficient convolution alg. Closes numba#2500
stuartarchibald
force-pushed
the
wip/correlate_convolve
branch
from
a3a7a57
to
6ac35c0
Compare
Codecov Report
@@ Coverage Diff @@
## master #2777 +/- ##
==========================================
- Coverage 86.19% 86.05% -0.15%
==========================================
Files 323 324 +1
Lines 66721 67720 +999
Branches 7455 7617 +162
==========================================
+ Hits 57513 58276 +763
- Misses 8016 8229 +213
- Partials 1192 1215 +23 |
numba/targets/arraymath.py
Outdated
| # For "direction", +1 to write the return values out in order 0->N | ||
| # -1 to write them out N->0. | ||
|
|
||
| if not (mode == 0 or mode == 2): |
There was a problem hiding this comment.
Can the 0 and 2 be named as sth like CORR_FULL=0 or use IntEnum for better readability?
There was a problem hiding this comment.
Also, I curious to know why 1 is skipped.
There was a problem hiding this comment.
This function is shared by correlate and convolve, in NumPy these functions have a mode kwarg (which is not in this PR) and the default mode is different in each function. The mode ends up in C code as having value 0, 1, 2 based on the string passed to mode (one of 'valid', 'same', 'full'), this function does the conversion https://github.com/numpy/numpy/blob/v1.14.0/numpy/core/numeric.py#L862-L870. To try and reduce future confusion, I've kept the magic numbers the same as in the NumPy impl (made a note here https://github.com/numba/numba/pull/2777/files#diff-4b9c6924f184d92f076cc50b84bcc442R1512). Good idea RE the IntEnum, will add one with members based on the strings for mode to enumerate the magic numbers.
There was a problem hiding this comment.
Oh and 1 is skipped as the mode same is not in use at present. :)
Respond to review comments.
|
@sklam fixes done. Thanks for the review. |
As title. Lack of FFT support prevents the implementation of
efficient convolution alg.
Closes #2500