Skip to content

Commit

Permalink
DEP: Remove "old behavior" of fftconvolve, so it's the same as convol…
Browse files Browse the repository at this point in the history 
…ve/correlate, etc.
  • Loading branch information
@endolith @rgommers
endolith authored and rgommers committed Feb 13, 2013
1 parent 0be0fac commit 66fe7c8
Showing 1 changed file with 7 additions and 6 deletions.
13 changes: 7 additions & 6 deletions scipy/signal/signaltools.py
View file
Expand Up @@ -163,13 +163,14 @@ def fftconvolve(in1, in2, mode="full"):
if mode == "full":
return ret
elif mode == "same":
if product(s1, axis=0) > product(s2, axis=0):
osize = s1
else:
osize = s2
return _centered(ret, osize)
return _centered(ret, s1)
elif mode == "valid":
return _centered(ret, abs(s2 - s1) + 1)
for d1, d2 in zip(in1.shape, in2.shape):
if not d1 >= d2:
raise ValueError(
"in1 should have at least as many items as in2 in "
"every dimension for 'valid' mode.")
return _centered(ret, s1 - s2 + 1)


def convolve(in1, in2, mode='full'):
Expand Down

3 comments on commit 66fe7c8

@jseabold
Copy link
Contributor

Choose a reason for hiding this comment

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

This commit broke a test in statsmodels. I'm not sure whether it's my fault for swapping the arguments to fftconvolve, but code that worked formerly for mode == "valid" won't work now, and it's not stated in the documentation what exactly is expected.

@rgommers
Copy link
Member

Choose a reason for hiding this comment

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

Deprecation warning in 0.12.x would make sense. I'll put that on my todo list.

@rgommers
Copy link
Member

Choose a reason for hiding this comment

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

Should be fixed by gh-477. Can you check that that works for you?

Please sign in to comment.