{"action":"created","issue":{"url":"https://api.github.com/repos/scipy/scipy/issues/5608","repository_url":"https://api.github.com/repos/scipy/scipy","labels_url":"https://api.github.com/repos/scipy/scipy/issues/5608/labels{/name}","comments_url":"https://api.github.com/repos/scipy/scipy/issues/5608/comments","events_url":"https://api.github.com/repos/scipy/scipy/issues/5608/events","html_url":"https://github.com/scipy/scipy/pull/5608","id":122187971,"number":5608,"title":"Adds keyword argument to choose faster convolution method","user":{"login":"stsievert","id":1320475,"avatar_url":"https://avatars.githubusercontent.com/u/1320475?v=3","gravatar_id":"","url":"https://api.github.com/users/stsievert","html_url":"https://github.com/stsievert","followers_url":"https://api.github.com/users/stsievert/followers","following_url":"https://api.github.com/users/stsievert/following{/other_user}","gists_url":"https://api.github.com/users/stsievert/gists{/gist_id}","starred_url":"https://api.github.com/users/stsievert/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stsievert/subscriptions","organizations_url":"https://api.github.com/users/stsievert/orgs","repos_url":"https://api.github.com/users/stsievert/repos","events_url":"https://api.github.com/users/stsievert/events{/privacy}","received_events_url":"https://api.github.com/users/stsievert/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/scipy/scipy/labels/enhancement","name":"enhancement","color":"bfe5bf"},{"url":"https://api.github.com/repos/scipy/scipy/labels/scipy.signal","name":"scipy.signal","color":"207de5"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":108,"created_at":"2015-12-15T03:53:53Z","updated_at":"2016-06-14T00:50:26Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/scipy/scipy/pulls/5608","html_url":"https://github.com/scipy/scipy/pull/5608","diff_url":"https://github.com/scipy/scipy/pull/5608.diff","patch_url":"https://github.com/scipy/scipy/pull/5608.patch"},"body":"As mentioned in #2651, this PR adds the keyword argument `method` to `scipy.signal.convolve` to choose the choose the convolution method. `method` can take vales `'auto'`, `'fft'` or `'direct'`.\r\n\r\nIn [scikit-image PR 1792][PR] we chose the faster convolution method, either the direct method or with fftconvolve. We merged these changes in, and I am merging these changes upstream.\r\n\r\n### Timing comparison\r\nIn the plot below, the ratio `fft_time / direct_time` is plotted with a log scaling. (more detail can be found in the [scikit-image PR][PR]). These plots are lifted from [the scikit-image PR][PR].\r\n\r\n![06ad4864-9f78-11e5-99a4-abbd7a5f63ed](https://cloud.githubusercontent.com/assets/1320475/11801224/77b9f1ae-a2a9-11e5-92ee-6b0bfda0bf4d.png)\r\n\r\nIn the plot below, the fit values to this ratio are shown. If this ratio is less than 1, this PR chooses to use fftconvolve.\r\n\r\n![7cfdbe20-a1aa-11e5-918c-cfd2f187b560](https://cloud.githubusercontent.com/assets/1320475/11801211/630dad2c-a2a9-11e5-846f-0af1712e1954.png)\r\n\r\nThe complete code and data to generate these plots are in [a gist]. These timings were performed on a Mid-2012 Macbook Air.\r\n\r\n### Tests\r\nTests on my machine failed to build. However, my own test passes:\r\n\r\n```python\r\nfor pwr in [4, 5, 6, 7, 8, 9, 10, 11, 12]:\r\n    for _ in range(int(1e3)):\r\n        x = np.random.rand(2**pwr)\r\n        h = np.random.rand((2**pwr)//10)\r\n\r\n        y1 = convolve(x, h, method='fft')\r\n        y2 = convolve(x, h, method='direct')\r\n\r\n        assert np.allclose(y1, y2), \"convolve should give equivalent answers\"\r\n```\r\n\r\n### Documentation\r\nThis PR includes documentation of this keyword argument in the docstring. I tried to follow the form of the `mode` keyword argument.\r\n\r\n[a gist]:https://gist.github.com/scottsievert/01ba818e2b16fcdec34d\r\n[PR]:https://github.com/scikit-image/scikit-image/pull/1792"},"comment":{"url":"https://api.github.com/repos/scipy/scipy/issues/comments/225751135","html_url":"https://github.com/scipy/scipy/pull/5608#issuecomment-225751135","issue_url":"https://api.github.com/repos/scipy/scipy/issues/5608","id":225751135,"user":{"login":"endolith","id":58611,"avatar_url":"https://avatars.githubusercontent.com/u/58611?v=3","gravatar_id":"","url":"https://api.github.com/users/endolith","html_url":"https://github.com/endolith","followers_url":"https://api.github.com/users/endolith/followers","following_url":"https://api.github.com/users/endolith/following{/other_user}","gists_url":"https://api.github.com/users/endolith/gists{/gist_id}","starred_url":"https://api.github.com/users/endolith/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/endolith/subscriptions","organizations_url":"https://api.github.com/users/endolith/orgs","repos_url":"https://api.github.com/users/endolith/repos","events_url":"https://api.github.com/users/endolith/events{/privacy}","received_events_url":"https://api.github.com/users/endolith/received_events","type":"User","site_admin":false},"created_at":"2016-06-14T00:50:26Z","updated_at":"2016-06-14T00:50:26Z","body":"> fftconvolve returns an array the same size as the first input while np.convolve returns an array the same size as the largest input. I was going to try and change this but didn't want to introduce backwards-compatibility issues. \r\n\r\nYeah that's why I had the special case for same mode in the numpy \"fastpath\".  I think that has to stay the way it is.\r\n\r\nAlso, this is never going to be perfect, so when is it good enough to merge?  It can always be improved in other PRs.  If it generally does a good job of choosing the faster path without slowing things down, and the API is what we want, can it be merged?"}}
{"actor":{"display_login":"endolith"}}