Changes to `convolve`, `fftconvolve`, ... #337

Closed
wants to merge 14 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

endolith commented Oct 12, 2012

  1. Made the error messages for invalid sizes consistent
  2. convolve2d had the check for 'valid' mode size but correlate2d didn't (just output an empty array), so I added it.
  3. The same check should be added to fftconvolve, I think. Currently it uses whichever array is bigger, which I think is the "old behavior" that was removed from the other functions here.
  4. Used rfftn for real-input fftconvolve to double the speed, as I suggested on the numpy trac (now here). (I hope I did this right and it doesn't break in weird cases)
  5. docstring cleanup, typos, etc.
  6. fftconvolve dies with zero-size inputs, so maybe should add some checks to reject those? Should probably be fixed at the fftn level, too, though.
  7. Added docstring to fftconvolve (though I think the functionality of fftconvolve should be folded into convolve/correlate/convolve2d/correlate2d and enabled with an option like fft={True, False, 'auto'} or method={'naive', 'fft', 'heuristic'} and then fftconvolve gets deprecated, as in the numpy feature request, but that requires more extensive discussion)
@@ -6,6 +6,7 @@
from scipy import linalg
from scipy.fftpack import fft, ifft, ifftshift, fft2, ifft2, fftn, \
ifftn, fftfreq
+from numpy.fft import rfftn, irfftn
@rgommers

rgommers Nov 26, 2012

Owner

Why import these from numpy, they're in scipy.fftpack too?

@endolith

endolith Nov 26, 2012

Contributor

That's what I tried first but it didn't work:

In [4]: from scipy.fftpack import rfftn
------------------------------------------------------------
Traceback (most recent call last):
  File "<ipython console>", line 1, in <module>
ImportError: cannot import name rfftn

Maybe this was fixed in a later version?

@rgommers

rgommers Nov 26, 2012

Owner

Oh yes, they behave differently. Need to check that the numpy version is what's required here.

@rgommers

rgommers Nov 26, 2012

Owner

The n-dimensional versions don't even exist in scipy. No idea why. These things should really be kept consistent, it's easy to do. So never mind my initial comment.

+ rely on the zero-padding.
+ ``same``
+ The output is the same size as `in1`, centered
+ with respect to the 'full' output.
@rgommers

rgommers Nov 26, 2012

Owner

Why this change? It takes up more lines, content is the same.

@endolith

endolith Nov 26, 2012

Contributor

so that it matches the format of convolve, convolve2d, correlate2d. Now it's a definition list instead of an unordered list, and should look better on the web docs after this change: http://docs.scipy.org/doc/scipy-dev/reference/generated/scipy.signal.correlate.html#scipy.signal.correlate http://docs.scipy.org/doc/scipy-dev/reference/generated/scipy.signal.convolve.html#scipy.signal.convolve

@rgommers

rgommers Nov 27, 2012

Owner

OK, makes sense.

scipy/signal/signaltools.py
if not complex_result:
+ IN1 = rfftn(in1, fsize)
+ IN1 *= rfftn(in2, fsize)
+ ret = irfftn(IN1)[fslice].copy()
@rgommers

rgommers Nov 26, 2012

Owner

Can you put these 3 lines in a single expression? Then the del IN1 statement can be removed too. Same for the else clause.

scipy/signal/signaltools.py
- (in1.shape[i], in2.shape[i]))
+ raise ValueError(
+ "in1 should have at least as many items as in2 in "
+ "every dimension for 'valid' mode.")
@rgommers

rgommers Nov 26, 2012

Owner

The dimension check for mode == 'valid' occurs 4 times in this file, can you factor it out as _check_valid_mode_inputs() or something like that?

def convolve(in1, in2, mode='full'):
"""
Convolve two N-dimensional arrays.
- Convolve `in1` and `in2` with output size determined by `mode`.
+ Convolve `in1` and `in2`, with the output size determined by the
+ `mode` argument.
@rgommers

rgommers Nov 26, 2012

Owner

Can you keep the summary line to a single line of <= 75 chars? It's extracted into a table sometimes.

@endolith

endolith Nov 26, 2012

Contributor

Isn't the summary line Convolve two N-dimensional arrays.?

@rgommers

rgommers Nov 26, 2012

Owner

Hmm, I should stop for tonight. Will re-read my own comments later:(

Owner

rgommers commented Nov 26, 2012

  1. and 2) look good. For 3) I agree that that same check should be added.

For 4) I need to check in detail. Looks like the size of the returned array could change:

In [13]: x = np.arange(5)

In [14]: x.size
Out[14]: 5

In [15]: np.fft.rfft(x).size
Out[15]: 3

In [16]: np.fft.irnp.fft.rfft(x)).size
np.fft.irfft   np.fft.irfft2  np.fft.irfftn  

In [16]: np.fft.irfft(np.fft.rfft(x)).size
Out[16]: 4
  1. overall looks good.

  2. indeed, should be handled and a test added to check that all functions are OK with zero-size or empty input.

  3. sounds OK to me to add that as an option.

endolith added some commits Dec 2, 2012

MAINT: Use a function to check "valid" mode shapes, remove check from…
… convolve() since correlate() will handle it.
MAINT: combine fftconvolve lines into one,
add asarray and zero-rank check to correlate(), same as convolve(),
move same-rank check from convolve to correlate,
remove unnecessary mode check from correlate
Contributor

endolith commented Dec 2, 2012

ok, did all the valid mode size checks in one function, combined the lines and removed the del. also:

convolve calls correlate, and has several checks that are already handled by correlate, so I removed them.

Should all 5 of these functions use asarray() at the beginning so they also accept lists, etc? convolve does, the others don't. I added it to correlate for now.

convolve has a check for rank zero arrays:

In [3]: convolve(array(3), array(4))
Out[3]: array([12])

but correlate does not:

In [13]: correlate(array(3), array(4))
------------------------------------------------------------
Traceback (most recent call last):
  File "<ipython console>", line 1, in <module>
  File "scipy\scipy\signal\signaltools.py", line 113, in correlate
    in1zpadded[sc] = in1.copy()
IndexError: 0-d arrays can't be indexed.

So I added that to correlate().

also this in correlate:

    else:
        raise ValueError("Unknown mode %s" % mode)

seems unnecessary since this is already handled by val = _valfrommode(mode):

ValueError: Acceptable mode flags are 'valid' (0), 'same' (1), or 'full' (2).

so I removed it

Contributor

endolith commented Dec 14, 2012

Ok, I added a docstring to fftconvolve, and any deprecation or folding it into other functions can wait for another pull request?

I still need to update the tests to match the current functionality (and add tests for null inputs?), but otherwise I don't see any more changes to make here. See any problems with these changes?

Contributor

endolith commented Dec 14, 2012

Would it make sense to add an axis= parameter, like https://github.com/nipy/nitime/blob/master/nitime/utils.py#L835 ?

Contributor

endolith commented Dec 16, 2012

As for 4), I didn't think the size of the returned array could change, because fsize is always a power of 2 and therefore an even number, which is unambiguous for irfftn, but I forgot about the only odd power of 2: fftconvolve(array([5]), array([3])) raises a ValueError. Adding an explicit size for irfftn fixes it.

Contributor

endolith commented Dec 16, 2012

I don't understand testing very well, but I think the tests for convolve2d are not being run at all, because TestConvolve2d is commented out? _TestConvolve2d would fail if it were run, because convolve2d doesn't accept lists as inputs. Should I take this as evidence that all these functions are intended to accept array_like and not just ndarray? Is asarray the best way to do that?

Owner

rgommers commented Dec 16, 2012

Adding asarray and documenting the parameters as accepting array_like looks right to me.

Taking over the axis=None kw from nitime seems fine; for convolve and correlate it looks like the C code (sigtools._correlateND) doesn't support that.

Owner

rgommers commented Dec 16, 2012

Looks like you're right that the tests for convolve2d aren't run. Seems to have been an oversight on my part (and whoever reviewed my patch) when removing the old behavior in 6b32937.

Owner

rgommers commented Dec 16, 2012

They should simply be enabled, although there's one failure right now, test_same_mode.

Owner

rgommers commented Dec 16, 2012

If with "null inputs" you mean empty arrays, that is always good to test. Empty array handling isn't very consistent across numpy/scipy.

Owner

rgommers commented Dec 16, 2012

Oh, I see now - with null input you mean a regression test for http://projects.scipy.org/scipy/ticket/1745. That would be needed too.

Owner

rgommers commented Dec 16, 2012

I get two failures for your branch:

======================================================================
ERROR: test_real_valid_mode (test_signaltools.TestFFTConvolve)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/rgommers/Code/scipy/scipy/signal/tests/test_signaltools.py", line 167, in test_real_valid_mode
    c = signal.fftconvolve(a,b,'valid')
  File "/home/rgommers/Code/scipy/scipy/signal/signaltools.py", line 190, in fftconvolve
    _check_valid_mode_shapes(s1, s2)
  File "/home/rgommers/Code/scipy/scipy/signal/signaltools.py", line 60, in _check_valid_mode_shapes
    "in1 should have at least as many items as in2 in "
ValueError: in1 should have at least as many items as in2 in every dimension for 'valid' mode.

======================================================================
FAIL: test_real_same_mode (test_signaltools.TestFFTConvolve)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/rgommers/Code/scipy/scipy/signal/tests/test_signaltools.py", line 162, in test_real_same_mode
    assert_array_almost_equal(c,d)
  File "/usr/lib/python2.7/dist-packages/numpy/testing/utils.py", line 800, in assert_array_almost_equal
    header=('Arrays are not almost equal to %d decimals' % decimal))
  File "/usr/lib/python2.7/dist-packages/numpy/testing/utils.py", line 600, in assert_array_compare
    raise AssertionError(msg)
AssertionError: 
Arrays are not almost equal to 6 decimals

(shapes (3,), (9,) mismatch)
 x: array([ 35.,  41.,  47.])
 y: array([  9.,  20.,  25.,  35.,  41.,  47.,  39.,  28.,   2.])
Contributor

endolith commented Dec 16, 2012

This also fails:

======================================================================
FAIL: test_same_mode (test_signaltools.TestConvolve2d)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\Jonathan\Documents\Programming\scipy\scipy\signal\tests\test_signaltools.py", line 132, in test_same_mode
    assert_array_equal(g,h)
  File "C:\Python27\lib\site-packages\numpy\testing\utils.py", line 707, in assert_array_equal
    verbose=verbose, header='Arrays are not equal')
  File "C:\Python27\lib\site-packages\numpy\testing\utils.py", line 636, in assert_array_compare
    raise AssertionError(msg)
AssertionError: 
Arrays are not equal

(mismatch 100.0%)
 x: array([[ 22,  28,  34],
       [ 80,  98, 116]])
 y: array([[ 80,  98, 116],
       [ 70,  82,  94]])

This test doesn't match actual behavior of convolve2d or convolve:

In [20]: e = [[1,2,3],[3,4,5]]

In [21]: f = [[2,3,4,5,6,7,8],[4,5,6,7,8,9,10]]

In [22]: convolve2d(e,f,'same')
Out[22]: 
array([[ 22,  28,  34],
       [ 80,  98, 116]])

In [23]: convolve(e,f,'same')
Out[23]: 
array([[ 22,  28,  34],
       [ 80,  98, 116]])

Matlab's conv2 'same' mode centers differently from SciPy, too: https://gist.github.com/4303970 Not sure if it should be changed to match Matlab or if the test should be changed to match how it actually works.

TST: update tests for convolve/correlate/etc:
change tests so they work with "new behavior" of 'valid' and 'same' modes
add tests for empty arrays, single-element arrays, and zero-rank arrays
Owner

rgommers commented Dec 16, 2012

The test should be changed to match actual behavior it looks like.

The difference with Matlab comes from making a different choice, not because of a bug, correct? In that case the choice should be documented, not changed.

Contributor

endolith commented Dec 16, 2012

I don't know if it's intentional or not, but I'll change the test to match.

TST: Enable convolve2d tests, change test to match actual centering b…
…ehavior of convolve2d() and convolve() in 'same' mode
Contributor

endolith commented Jan 8, 2013

http://scipy.github.com/faq.html#what-is-the-difference-between-matrices-and-arrays says:

Unfortunately, a few of NumPy’s many functions use asarray() when they should use asanyarray(), so from time to time you may find your matrices accidentally get converted into arrays. Just use asmatrix() on the output of these operations, and consider filing a bug.

so I guess it should use that instead? So people can convolve matrices?

Owner

rgommers commented Jan 9, 2013

asanyarray is not necessary. Matrix input is still accepted, you just get an ndarray back. Which is what we want.

Owner

rgommers commented Feb 13, 2013

Rebased this, added some tests and minor fixes, and merged in 9cad94f. Thanks @endolith.

@rgommers rgommers closed this Feb 13, 2013

@endolith endolith deleted the endolith:fftconvolve branch May 5, 2013

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