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

ENH: Add Scipy and Cupy as fft interfaces #8

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

PinkShnack
Copy link

@PinkShnack PinkShnack commented Aug 26, 2024

About

I'd like to be able to use Cupy for Fourier transforms. Cupy is a python package that uses numpy-like syntax for using CUDA and GPU.

To do

  • add scipy
  • add cupy
    • allow fftn for cupy, so that we can pass more than one hologram array at a time to the GPU.
      • We should probably do this by having a special FFTFilterCupy3D, so that the default input of a 2D image is kept by the default FFTFilter classes, and the user has to know what they are doing to use FFTFilterCupy3D.
      • Then we would also need a interfere.base.BaseInterferogram3D, seeing as the current base class works on the premise that it is processing one 2D image. Might be able to combine the functionality because it uses numpy.
    • Benchmark against other methods.
  • add scipy real (some difficulties here, requires padding)
  • docs: why don't my docstring sphinx links work?!
  • cicd passed
  • update changelog

@PinkShnack
Copy link
Author

Hey @paulmueller, let me know if you think this fits in the qpretrieve world. Could be cool!! I've added an example to the docs also.


sphinx
sphinxcontrib.bibtex
sphinx_rtd_theme
Copy link
Author

Choose a reason for hiding this comment

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

I removed the pinned versions, as some sphinx dependency did not like the old sphinx version.

n_transforms = 100

# one transform
results_1 = {}
Copy link
Author

Choose a reason for hiding this comment

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

I need to clean this script up a bit to make it simpler

@@ -104,8 +104,10 @@ def get_filter_array(filter_name, filter_size, freq_pos, fft_shape):
# TODO: avoid the np.roll, instead use the indices directly
alpha = 0.1
rsize = int(min(fx.size, fy.size) * filter_size) * 2
tukey_window_x = signal.tukey(rsize, alpha=alpha).reshape(-1, 1)
tukey_window_y = signal.tukey(rsize, alpha=alpha).reshape(1, -1)
tukey_window_x = signal.windows.tukey(
Copy link
Author

Choose a reason for hiding this comment

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

scipy's new versions imports tukey from signal.window, not signal

FFTFilterPyFFTW,
FFTFilterNumpy,
FFTFilterScipy,
FFTFilterCupy,
Copy link
Author

Choose a reason for hiding this comment

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

This isn't necessarily the "perferred order" we want, but I'd like to keep it as is due to old default pipelines.

@@ -70,8 +70,11 @@ def __init__(self, data, subtract_mean=True, padding=2, copy=True):
else:
# convert integer-arrays to floating point arrays
dtype = float
if not copy:
# numpy v2.x behaviour requires asarray with copy=False
Copy link
Author

Choose a reason for hiding this comment

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

numpy has made copying a bit awkward. When an array can't be copied (with either np.array or np.asarray), and the user has copy=False, then it throws an error!

data_gpu = cp.asarray(data)
# likely an inefficiency here, could use `set_global_backend`
with sp.fft.set_backend(cufft):
fft_gpu = sp.fft.fft2(data_gpu)
Copy link
Author

Choose a reason for hiding this comment

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

Not ideal, this is something to work on.

"FFTW": "pyfftw>=0.12.0",
# manually install 'cupy-cuda11x' if you have older CUDA.
# See https://cupy.dev/
"CUPY": "cupy-cuda12x",
Copy link
Author

Choose a reason for hiding this comment

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

Will have to check this version.

@paulmueller paulmueller marked this pull request as draft September 9, 2024 06:10
@paulmueller
Copy link
Member

Hello, This is absolutely in the scope of qpretrieve!

It would probably make sense to address all CI issues (GH actions, readthedocs) and dependencies (e.g. I would be fine with bumping numpy to >= 2.0) in a separate PR.

I have converted this PR to a draft for now, so you can work on it in stealth mode.

@PinkShnack
Copy link
Author

It would probably make sense to address all CI issues (GH actions, readthedocs) and dependencies (e.g. I would be fine with bumping numpy to >= 2.0) in a separate PR.

Sounds good, I will revert the actions changes. However, the docs requirements just don't build on my side because they are too restrictive.

I have converted this PR to a draft for now, so you can work on it in stealth mode.

🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants