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

Add scipy.fft to cupyx #2355

Open
wants to merge 15 commits into
base: master
from

Conversation

@peterbell10
Copy link

peterbell10 commented Aug 1, 2019

SciPy version 1.4 will have a new submodule scipy.fft with an interface very similar to numpy.fft. This adds a subset of that interface to cupyx.scipy and also adds uarray-based backend support allowing cupy FFTs to be called through the scipy.fft interface e.g.

import cupy
import cupyx.scipy.fft as cu_fft
import scipy.fft

x = cupy.arange(10)
scipy.fft.fft(x)  # ValueError: object __array__ method not producing an array

with scipy.fft.set_backend(cu_fft):
    scipy.fft.fft(x)  # Calls into cupyx, returning a cupy array

Tests are currently failing but at least some of the failures seem to be bugs in cupy.fft. I need to investigate further.

@grlee77

This comment has been minimized.

Copy link
Contributor

grlee77 commented Aug 1, 2019

Thanks, Peter. I will take a look soon.

Some additional context for the CuPy team: This work is associated with a SciPy Google Summer of Code project. Any feedback on the implementation is greatly appreciated. Relevant SciPy PRs for additional details are:
new scipy.fft module: scipy/scipy#10238
uarray-based backend support: scipy/scipy#10383

Starting with SciPy 1.4.0, the recommendation to users is to call FFTs via the new scipy.fft module in preference to scipy.fftpack. The fftpack interface will continue to be available for backwards compatibility.

@leofang

This comment has been minimized.

Copy link
Contributor

leofang commented Aug 1, 2019

Hi @grlee77 I suppose it'd work like cupy.fft in which 1D/ND cuFFT plans are generated under the hood?

@grlee77

This comment has been minimized.

Copy link
Contributor

grlee77 commented Aug 1, 2019

Yes, this is using the same underlying implementations from cupy.fft so is not adding too much new complexity here. There are a few differences in API from NumPy/scipy.fftpack. Off the top of my head, scipy.fft supports an overwrite_x argument like scipy.fftpack but the R2C/C2R transforms operate more like the ones in numpy.fft than the scipy.fftpack ones. This PR provides the new API for CuPy users and allows CuPy to become an FFT backend for SciPy as in the example Peter gave above.

@peterbell10

This comment has been minimized.

Copy link
Author

peterbell10 commented Aug 1, 2019

Just to add to that. The _fft and _fftn functions in cupy.fft all support overwrite_x already, so this is just a slightly different interface to those same functions.

@leofang

This comment has been minimized.

Copy link
Contributor

leofang commented Aug 1, 2019

I see, thanks for the clarification, guys.

Do you know what's the official long term plan for scipy.fftpack? Would it be deprecated after scipy.fft becomes polished? I wonder if it's possible to add a plan argument to scipy.fft (as we did before, see #1942, #2033), so that explicit cuFFT plans can still be used. Does scipy.fft.set_backend set up any rules against changes like this in the scipy.fft API?

@grlee77

This comment has been minimized.

Copy link
Contributor

grlee77 commented Aug 1, 2019

also including @larsoner here (not sure if he already follows this repo)

@peterbell10

This comment has been minimized.

Copy link
Author

peterbell10 commented Aug 1, 2019

Do you know what's the official long term plan for scipy.fftpack?

There are no plans to deprecate it currently. It will be documented as "long term legacy".

I wonder if it's possible to add a plan argument to scipy.fft (as we did before, see #1942, #2033), so that explicit cuFFT plans can still be used. Does scipy.fft.set_backend set up any rules against changes like this in the scipy.fft API?

You won't be able to pass a plan argument through scipy.fft unless the argument is added to scipy itself.

@leofang

This comment has been minimized.

Copy link
Contributor

leofang commented Aug 1, 2019

You won't be able to pass a plan argument through scipy.fft unless the argument is added to scipy itself.

OK, perhaps it's better to move the further discussion on plan to a separate issue: #2356.

peterbell10 and others added 3 commits Jul 30, 2019
By sorting the axes before applying _cook_shape it would resize the wrong axes.
* API: add missing norm kwarg to irfft
* BUG: _fftn handle axes=None case when shape is specified
@peterbell10 peterbell10 force-pushed the peterbell10:scipy_fft branch from 85eeb5b to fac51e8 Aug 5, 2019
@peterbell10 peterbell10 force-pushed the peterbell10:scipy_fft branch from c177035 to 697d1c1 Aug 7, 2019
peterbell10 and others added 3 commits Aug 7, 2019
Co-Authored-By: Leo Fang <leofang@bnl.gov>
@peterbell10 peterbell10 force-pushed the peterbell10:scipy_fft branch from 65b13e6 to b6462ee Aug 7, 2019
@peterbell10

This comment has been minimized.

Copy link
Author

peterbell10 commented Aug 7, 2019

I still have some test failures locally for rfft and hfft but I've confirmed that the cupy.fft tests are failing in the same way on master so it can't be my code. @grlee77 has also confirmed that these tests pass on his machine.

Question: Should I add tests for the scipy.fft backend functionality, given that it would require a recent weekly build of SciPy to run?

@larsoner

This comment has been minimized.

Copy link
Contributor

larsoner commented Aug 7, 2019

Question: Should I add tests for the scipy.fft backend functionality, given that it would require a recent weekly build of SciPy to run?

FWIW many other repositories I know of and/or work on use a line like the following in some Travis build:

pip install -f "https://7933911d6844c6c53a7d-47bd50c35cd79bd838daf386af554a83.ssl.cf2.rackcdn.com" --pre numpy scipy;

It helps ensure that there are no unhappy surprises leading up to new NumPy and SciPy releases.

It helps to add this functionality in a PR separate from whatever functionality actually needs it, though, since using the weekly (?) builds themselves already sometimes exposes issues.

@grlee77

This comment has been minimized.

Copy link
Contributor

grlee77 commented Aug 7, 2019

Question: Should I add tests for the scipy.fft backend functionality, given that it would require a recent weekly build of SciPy to run?

I would skip that for now unless one of the core devs here expresses support for adding it. I did not test the backend part locally yet, but can do that soon.

As is, the automated CI on Travis/Appveyor does not actually run any tests on the GPU. Once you think this is ready, you can ping the maintainers here to trigger a build on their internal Jenkins CI which tests on actual GPU hardware.

@peterbell10

This comment has been minimized.

Copy link
Author

peterbell10 commented Aug 7, 2019

Okay, I've tested all the functions via the scipy.fft interface locally and everything seems to be working correctly. If there's no desire for automated tests then I think that's everything.

@peterbell10 peterbell10 marked this pull request as ready for review Aug 7, 2019
@larsoner

This comment has been minimized.

Copy link
Contributor

larsoner commented Aug 8, 2019

I can confirm that pytest tests/cupyx_tests/scipy_tests/fft_tests passes on my machine with Tesla K40 hardware and CUDA 10.1 on Ubuntu 19.04.

cupyx/scipy/fft/fft.py Outdated Show resolved Hide resolved
Copy link

hameerabbasi left a comment

A few comments on __ua_convert__

cupyx/scipy/fft/fft.py Outdated Show resolved Hide resolved
cupyx/scipy/fft/fft.py Outdated Show resolved Hide resolved
Could be needed for forward compatibility if more dispatchables are added in
future.
@jakirkham

This comment has been minimized.

Copy link
Contributor

jakirkham commented Sep 19, 2019

@hameerabbasi @larsoner @leofang, looks like there have been some changes pushed to address your concerns. Would you have an opportunity to do another review? 🙂

@grlee77

This comment has been minimized.

Copy link
Contributor

grlee77 commented Nov 19, 2019

Hey CuPy devs (pinging @takagi, @asi1024 @toslunar for past FFT-related PR involvement): what are the chances of including this PR for 7.0?

SciPy branched 1.4.x over the weekend and should have a 1.4.0rc1 available soon. Final release of 1.4 is currently anticipated around Dec. 10th. As of that release this scipy.fft module is officially recommended over the legacy scipy.fftpack, so it would be nice for cupyx.scipy to have the same interface available.

cupyx/scipy/fft/fft.py Outdated Show resolved Hide resolved
cupy/fft/fft.py Show resolved Hide resolved
cupyx/scipy/fft/fft.py Outdated Show resolved Hide resolved
Copy link

hameerabbasi left a comment

A couple of minor nits.

cupyx/scipy/fft/fft.py Outdated Show resolved Hide resolved
cupyx/scipy/fft/fft.py Outdated Show resolved Hide resolved
@leofang

This comment has been minimized.

Copy link
Contributor

leofang commented Nov 22, 2019

I am wondering whether it makes sense or not to expose cupyx.scipy.fftpack.get_fft_plan() to the cupyx.scipy.fft namespace. Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.