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

BUG: signal.convolve takes longer than it needs to #3623

Closed
endolith opened this issue May 7, 2014 · 2 comments
Closed

BUG: signal.convolve takes longer than it needs to #3623

endolith opened this issue May 7, 2014 · 2 comments
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.signal
Milestone

Comments

@endolith
Copy link
Member

endolith commented May 7, 2014

While looking into #2651, I noticed that convolve takes longer if the first input is shorter than the second input, even though they are computing the same thing (convolution is commutative):

In [54]: a = rand(2)

In [55]: b = rand(500)

In [56]: timeit convolve(a, b, 'full')
100 loops, best of 3: 5.47 ms per loop

In [57]: timeit convolve(b, a, 'full')
10000 loops, best of 3: 67.9 us per loop

In [60]: allclose(convolve(a, b, 'full'), convolve(b, a, 'full'))
Out[60]: True

In [58]: timeit fftconvolve(a, b, 'full')
1000 loops, best of 3: 146 us per loop

In [59]: timeit fftconvolve(b, a, 'full')
10000 loops, best of 3: 147 us per loop

So the algorithm is taking 80 times as long as it needs to, and could be modified to take the same amount of time regardless of input order, maybe by just checking which input is longer and then internally rearranging them so that the shorter one comes last.

@endolith
Copy link
Member Author

endolith commented May 9, 2014

In [107]: short = rand(2)
In [112]: long = rand(500)

In [113]: timeit signal.convolve(short, long, 'full')  (SLOW)
100 loops, best of 3: 5.08 ms per loop

In [114]: timeit signal.convolve(long, short, 'full')
10000 loops, best of 3: 67.9 us per loop




In [115]: short = rand(2, 50)

In [116]: long = rand(50, 50)

In [117]: timeit signal.convolve(short, long, 'full')   (SLOW)
1 loops, best of 3: 333 ms per loop

In [118]: timeit signal.convolve(long, short, 'full')
100 loops, best of 3: 14 ms per loop


In [119]: short = rand(50, 2)

In [120]: timeit signal.convolve(short, long, 'full')  (SLOW)
1 loops, best of 3: 354 ms per loop

In [121]: timeit signal.convolve(long, short, 'full')
100 loops, best of 3: 14.3 ms per loop

So size(a) > size(b) is probably what it wants? len() won't work on ND arrays.

Maybe the underlying C code is optimized for the len(a) > len(b) case, since this was true 100% of the time with the "old behavior" that numpy.convolve still uses, where the inputs are internally swapped so the larger one is first?

@endolith
Copy link
Member Author

endolith commented Jun 5, 2014

Fixed by #3664

@endolith endolith closed this as completed Jun 5, 2014
@rgommers rgommers added this to the 0.15.0 milestone Jun 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.signal
Projects
None yet
Development

No branches or pull requests

2 participants