Adding cupyx.scipy.signal.fftconvolve #3828

coderforlife · 2020-08-21T15:34:28Z

This adds cupyx.scipy.signal.fftconvolve along with support for method='fft' in cupyx.scipy.signal.convolve and cupyx.scipy.signal.correlate. This function fully supports n-dimensional data.

This PR does not include adjustments to choose_conv_method or _fftconv_faster to intelligently choose between the two methods, that will be done in a future PR.

leofang

Could you post benchmark results? Thanks!

leofang · 2020-08-21T18:19:02Z

cupyx/scipy/signal/_signaltools_core.py

+def _freq_domain_conv(in1, in2, axes, shape, calc_fast_len=False):
+ # See scipy's documentation in scipy.signal.signaltools
+ # TODO: cupyx.scipy.fftpack.get_fft_plan may be useful, however:
+ # * only complex-to-complex planning is possible


This is not true. We've supported all kinds on planning in get_fft_plan.

I wrote this function a few months ago, maybe back then it only worked for complex-to-complex or I was misusing it. I will remove that comment.

Ah yes, I think the R2C/C2R Nd plan was supported also recently. Though I think they are harder to reuse by nature and perhaps my plan cache #3730 would serve better than explicitly managing plans here.

coderforlife · 2020-08-21T20:23:37Z

This is run on a Titan V GPU and an Intel Xeon Gold 5122 (3.6 GHz).

Some benchmarks (in seconds) are as follows for a in1.size == 16777216 and in2.size == 8192. Columns:

numpy.convolve
scipy.signal.convolve with method='direct'
scipy.signal.convolve with method='fft'
cupy.convolve
cupyx.scipy.signal.convolve with method='direct'
cupyx.scipy.signal.convolve with method='fft'

`mode`	1	2	3	4	5	6
`'full'`	21.16	21.17	1.601	0.009914	0.2568	0.009545
`'valid'`	25.96	25.95	1.591	0.009872	0.2533	0.009501
`'same'`	25.98	25.98	1.590	0.009782	0.2567	0.009492

Clearly the FFT methods are faster and cupy.convolve is using FFTs. The new FFT convolution is just mildly faster than the current cupy.convolve FFT method (about 3-4% so not much).

On the small side with in1.size == 8192 and in2.size == 2048:

`mode`	1	2	3	4	5	6
`'full'`	0.002368	0.002339	0.0002659	0.0007699	0.0003114	0.001072
`'valid'`	0.001397	0.001395	0.0002823	0.0007692	0.0003703	0.001084
`'same'`	0.002211	0.002186	0.0002787	0.0007601	0.0003113	0.001064

For numpy/scipy the FFT method is faster but in cupyx.scipy direct is actually faster by this point (about 1/3 of the time). At this point, cupy.convolve is still using FFT (since it is only based on data types) and is a bit faster than the new FFT method but not by much. In any case the direct method is still the best. Note: I have noticed that 0.0010 is the absolute minimum for FFT in the new method - basically the fixed overhead.

cupyx/scipy/signal/_signaltools_core.py

tests/cupyx_tests/scipy_tests/signal_tests/test_signaltools.py

asi1024 · 2020-09-04T22:35:51Z

cupyx/scipy/signal/_signaltools_core.py

+ return axes
+
+
+def _freq_domain_conv(in1, in2, axes, shape, calc_fast_len=False):


calc_fast_len is always True.

However, I have also written up oaconvolve this does pass calc_fast_len as False. I could remove it for now just to add it back when oaconvolve is added.

cupyx/scipy/signal/_signaltools_core.py

… to use it.

asi1024 · 2020-09-17T07:45:14Z

Jenkins, test this please.

chainer-ci · 2020-09-17T08:09:58Z

Jenkins CI test (for commit add37ef, target branch master) failed with status FAILURE.

asi1024 · 2020-09-17T09:30:53Z

@coderforlife Could you check the Jenkins failure?

coderforlife · 2020-09-17T15:58:59Z

It looks like there are 2 tests that fail, both when using float32 and the actual and desired outputs are just barely missing the tolerances. I will relax the tolerances for float32s for this test.

asi1024 · 2020-10-20T08:11:44Z

@coderforlife Any updates?

asi1024 · 2020-12-18T08:12:03Z

Jenkins, test this please.

chainer-ci · 2020-12-18T09:22:30Z

Jenkins CI test (for commit bb4d644, target branch master) failed with status FAILURE.

coderforlife · 2020-12-18T23:31:18Z

I forgot which values were off by and by how much and Jenkins info was removed due to being too long ago. Was able to use the newest results and hopefully will pass this one last time. Thanks!

leofang · 2020-12-19T02:51:37Z

I forgot which values were off by and by how much and Jenkins info was removed due to being too long ago. Was able to use the newest results and hopefully will pass this one last time. Thanks!

Hi @coderforlife, in case you don't know already, or if needed, it is now possible to set tolerance per dtype: #4269.

coderforlife · 2020-12-19T03:48:52Z

I should the reset the tolerances so it is more strict for the float64, don't run the code through Jenkins yet, I will update soon.

coderforlife · 2020-12-19T05:06:29Z

It should be better now! Can we run it on Jenkins (which needs different tolerances than on my machine in some cases so may fail even though it is working on my own machine). Thanks!

leofang · 2020-12-19T05:11:02Z

Happy to help! (I haven't read the changes as I am not in a right condition.)

Jenkins, test this please

coderforlife · 2020-12-19T06:15:18Z

One question about the next step when I add oaconvolve: in cupy.core.internal._normalize_axis_indices it only accepts tuples as sequences. Is there any reason to not also accept lists and possibly any other iterables? All that would be require would be to change the elif condition on line 424 to something like not isinstance(axes, (tuple, list)) or not isinstance(axes, collections.abc.Iterable) or possibly something a bit more complex like:

else:
    try:
        iter(axes)
    except TypeError:
        axes = axes,

chainer-ci · 2020-12-19T07:01:15Z

Jenkins CI test (for commit 9d7bc1e, target branch master) succeeded!

asi1024 · 2020-12-21T05:51:44Z

Is there any reason to not also accept lists and possibly any other iterables?

It is because numpy does not support non-tuple axes. More specifically, for 2-dim x of numpy.ndarray, numpy.sum(x, axis=[1, 2]) and numpy.sum(x, axis=range(2)) raises TypeError.

asi1024 · 2020-12-21T05:54:45Z

LGTM!

coderforlife · 2020-12-21T06:48:05Z

It is because numpy does not support non-tuple axes. More specifically, for 2-dim x of numpy.ndarray, numpy.sum(x, axis=[1, 2]) and numpy.sum(x, axis=range(2)) raises TypeError.

Okay! Thanks, fixed the oaconvolve code itself to generate tuples for axes.

Adding fftconvolve.

5b5ed6a

coderforlife mentioned this pull request Aug 21, 2020

Possible combination of cupy.convolve and cupyx.scipy.signal.convolve #3829

Open

leofang reviewed Aug 21, 2020

View reviewed changes

kmaehashi assigned asi1024 Aug 24, 2020

kmaehashi added cat:feature New features/APIs prio:medium labels Aug 24, 2020

Dahlia-Chehata mentioned this pull request Aug 25, 2020

Add cupy.poly #3547

Closed

asi1024 reviewed Sep 4, 2020

View reviewed changes

coderforlife added 3 commits September 5, 2020 22:39

Updating comment.

c78f6e0

Minor updates for code quality.

69bfd14

Removing duplicate code.

54e4b9f

asi1024 reviewed Sep 8, 2020

View reviewed changes

coderforlife added 3 commits September 12, 2020 00:15

Add sort_axes param to _normalize_axis_indices and update signal code…

856e0ee

… to use it.

Merge remote-tracking branch 'upstream/master' into signaltools

377e702

Refactoring for cupy.util to cupy._util name change.

add37ef

kmaehashi added the st:awaiting-author Awaiting response from author label Sep 29, 2020

coderforlife mentioned this pull request Oct 23, 2020

Supporting (optional) FFT-based convolution in ndimage.convolve #3378

Open

coderforlife added 4 commits December 14, 2020 17:04

Merge remote-tracking branch 'upstream/master' into signaltools

58b408b

Updating to match code restructuring.

071d621

Decrease sensitivity of tests due to float32 inaccuracies.

d4e2c2b

Fix flake8 errors.

bb4d644

Making tolerances work for float32 values as well

bfe2ad9

coderforlife added 2 commits December 19, 2020 00:02

Using new per-dtype-type tolerances.

daff184

Minor code cleanup.

9d7bc1e

asi1024 merged commit 666d68a into cupy:master Dec 21, 2020

asi1024 added this to the v9.0.0b1 milestone Dec 21, 2020

asi1024 removed the st:awaiting-author Awaiting response from author label Dec 21, 2020

andfoy mentioned this pull request Feb 14, 2023

Tracker: cupyx.scipy.signal #7403

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding cupyx.scipy.signal.fftconvolve #3828

Adding cupyx.scipy.signal.fftconvolve #3828

coderforlife commented Aug 21, 2020

leofang left a comment

leofang Aug 21, 2020

coderforlife Aug 21, 2020

leofang Aug 21, 2020

coderforlife commented Aug 21, 2020

asi1024 Sep 4, 2020

coderforlife Sep 6, 2020

asi1024 commented Sep 17, 2020

chainer-ci commented Sep 17, 2020

asi1024 commented Sep 17, 2020

coderforlife commented Sep 17, 2020

asi1024 commented Oct 20, 2020

asi1024 commented Dec 18, 2020

chainer-ci commented Dec 18, 2020

coderforlife commented Dec 18, 2020

leofang commented Dec 19, 2020

coderforlife commented Dec 19, 2020

coderforlife commented Dec 19, 2020

leofang commented Dec 19, 2020

coderforlife commented Dec 19, 2020

chainer-ci commented Dec 19, 2020

asi1024 commented Dec 21, 2020

asi1024 commented Dec 21, 2020

coderforlife commented Dec 21, 2020

		return axes


		def _freq_domain_conv(in1, in2, axes, shape, calc_fast_len=False):

Adding cupyx.scipy.signal.fftconvolve #3828

Adding cupyx.scipy.signal.fftconvolve #3828

Conversation

coderforlife commented Aug 21, 2020

leofang left a comment

Choose a reason for hiding this comment

leofang Aug 21, 2020

Choose a reason for hiding this comment

coderforlife Aug 21, 2020

Choose a reason for hiding this comment

leofang Aug 21, 2020

Choose a reason for hiding this comment

coderforlife commented Aug 21, 2020

asi1024 Sep 4, 2020

Choose a reason for hiding this comment

coderforlife Sep 6, 2020

Choose a reason for hiding this comment

asi1024 commented Sep 17, 2020

chainer-ci commented Sep 17, 2020

asi1024 commented Sep 17, 2020

coderforlife commented Sep 17, 2020

asi1024 commented Oct 20, 2020

asi1024 commented Dec 18, 2020

chainer-ci commented Dec 18, 2020

coderforlife commented Dec 18, 2020

leofang commented Dec 19, 2020

coderforlife commented Dec 19, 2020

coderforlife commented Dec 19, 2020

leofang commented Dec 19, 2020

coderforlife commented Dec 19, 2020

chainer-ci commented Dec 19, 2020

asi1024 commented Dec 21, 2020

asi1024 commented Dec 21, 2020

coderforlife commented Dec 21, 2020