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

EXAMPLES: Change use of scipy.fftpack to scipy.fft #5986

Closed
srvanrell opened this issue Oct 25, 2021 · 8 comments · Fixed by #5999
Closed

EXAMPLES: Change use of scipy.fftpack to scipy.fft #5986

srvanrell opened this issue Oct 25, 2021 · 8 comments · Fixed by #5999
Labels
🔧 type: Maintenance Refactoring and maintenance of internals

Comments

@srvanrell
Copy link
Contributor

Description

Examples like this band pass or this use of windows are using deprecated scipy.fftpack. They could use scipy.fft module instead. I will kindly make a PR if you consider that is appropiate.

@grlee77
Copy link
Contributor

grlee77 commented Oct 25, 2021

I agree that it is preferable to use scipy.fft, but this may need to wait until we drop support for SciPy older than 1.4 (when scipy.fft was introduced). We try to followed NEP 29, keeping an oldest supported NumPy/SciPy minor version released at least 24 months prior. It looks like SciPy 1.4 was released on Dec. 16, 2019, so we can drop support for SciPy < 1.4 in mid Dec. 2021.

If you want to make a PR for this now, that is great, I just wanted to give a heads up that it would likely have to wait a couple of months before being merged.

@rfezzani rfezzani added the 🔧 type: Maintenance Refactoring and maintenance of internals label Oct 26, 2021
@srvanrell
Copy link
Contributor Author

Great! Thank you @grlee77 for the clarification and the prompt response.

I'll make a PR so you can consider it in the future.

@hmaarrfk
Copy link
Member

All minor versions of numpy released in the 24 months prior to the project, and at minimum the last three minor versions.

Since 1.3.0 was released more than 24 months ago, we aren't "asked" to support it. I would approve a PR that bumped the minimum version requirement of scikit-image as a whole to 1.4

$ nep29 scipy --n_months 24
| version |    date    |
|---------|------------|
|  1.7.0  | 2021-06-20 |
|  1.6.0  | 2020-12-31 |
|  1.5.0  | 2020-06-21 |
|  1.4.0  | 2019-12-16 |

^^^^
I made a tool a while back that checked nep29 dependencies according to the rules set out:

@srvanrell
Copy link
Contributor Author

I submitted a PR on this, just changing examples and setup.py. Would you prefer that I update skimage internal functions too? The detail of files using fftpack is below. Just let me know.

./skimage/registration/_masked_phase_cross_correlation.py:194:    # transform axes unchanged which was not possible with scipy.fftpack's
./skimage/registration/_phase_cross_correlation.py:12:    from scipy.fftpack import fftn, ifftn
./skimage/registration/_phase_cross_correlation.py:13:from scipy.fftpack import fftfreq
./skimage/transform/radon_transform.py:13:    # fallback from scipy.fft to scipy.fftpack instead of numpy.fft
./skimage/transform/radon_transform.py:14:    # (fftpack preserves single precision while numpy.fft does not)
./skimage/transform/radon_transform.py:15:    from scipy.fftpack import fft, ifft
./skimage/_shared/fft.py:15:    from scipy.fftpack import next_fast_len

@hmaarrfk
Copy link
Member

hmaarrfk commented Nov 3, 2021

Yes that would be good.

I think that there is also a TODO in the file TODO.txt

Other (2022)
------------
* Remove conditional import of ``scipy.fft`` in ``skimage._shared.fft`` and
  `skimage.registration._phase_cross_correlation.py` once the minimum supported
  version of ``scipy`` reaches 1.4.
* Remove unneeded `deconv_type` and astype call in `weiner` and
  `unsupervised_wiener` in `skimage.restoration.deconvolution` once the minimum
  supported version of ``scipy`` reaches 1.4.
* Remove two unneeded astype calls in `butterworth` once the minimum supported
  version of ``scipy`` reaches 1.4.

@grlee77
Copy link
Contributor

grlee77 commented Nov 3, 2021

Thanks @hmaarrfk, I was just about to link those!

@srvanrell, If there is any confusion on those items, let me know and I can help out. I added float32 support to some functions relying on FFTs, but unfortunately numpy.fft only supports double precision, so I still had to use astype. Now that skimage._shared.fft can always assume scipy.fft is available, these astype calls won't be necessary since the SciPy FFTs support float32.

@srvanrell
Copy link
Contributor Author

srvanrell commented Nov 3, 2021

Should I completly remove skimage._shared.fft? It seems useless to have that file just to import next_fast_len and scipy.fft, which could be imported elsewhere

@hmaarrfk
Copy link
Member

hmaarrfk commented Nov 3, 2021

yeah, it would be a nice cleanup. It seems only _masked_phase_cross_correlation.p use next_fast_len

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔧 type: Maintenance Refactoring and maintenance of internals
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants