Skip to content

Add Cupy as optional dep and as FFTFilter#16

Merged
paulmueller merged 39 commits intomainfrom
10_cupy_fftfilter
Nov 5, 2025
Merged

Add Cupy as optional dep and as FFTFilter#16
paulmueller merged 39 commits intomainfrom
10_cupy_fftfilter

Conversation

@PinkShnack
Copy link
Copy Markdown
Contributor

@PinkShnack PinkShnack commented Sep 23, 2025

As described in #10, it would be good to have a gpu implementation built in as an FFTFilter.
A lot of this was done in #8, but we have split that up into #9 and here #16.

Todos

  • Add 2d cupy implementation.
  • Add 3d (see Move from 2D to 3D array operations: breaking changes #9) cupy implementation. Check to see the difference between the 3D and 2D FFT algorithms with 2D and 3D inputs. write tests to show the diffference and show speed differences. Likely that Cupy3D is NOT required.
    • Cupy3D not needed, this idea was a stayover from when qpretrieve did not by default process FFTs in 3D
  • All FFTFilters use axes inputs
  • add as optional dep.
  • Gracefully handle importing of cupy filter when not in env
  • Test: ensure the outputted FFT is identical.
  • Examples: show difference between first FFT, warmed up single FFT (pyfftw), and bulk 3D FFT
    • put batch and fft speeds in one graph.
  • Add documentation (benchmarking etc)

@paulmueller paulmueller marked this pull request as draft September 23, 2025 14:28
@paulmueller
Copy link
Copy Markdown
Member

Converted to draft. Please ping me when you need a review 👍

Comment thread .github/workflows/check.yml Outdated
@PinkShnack
Copy link
Copy Markdown
Contributor Author

How the tests act with and without optional dependencies using the new test helper method:

  • no_pyfftw-no_cupy
    no_pyfftw-no_cupy
  • with_pyfftw-no_cupy
    with_pyfftw-no_cupy
  • with-pyfftw-with_cupy
    with-pyfftw-with_cupy

@PinkShnack
Copy link
Copy Markdown
Contributor Author

@paulmueller please review this, when you have time. :)

@PinkShnack PinkShnack mentioned this pull request Oct 30, 2025
5 tasks
Comment thread .github/workflows/check.yml Outdated
Comment thread docs/sec_installation.rst Outdated
Comment thread examples/fft_batch_speeds.png
Comment thread examples/fft_batch_speeds.py Outdated
Comment thread examples/fft_batch_speeds.py Outdated
Comment thread tests/test_cupy_gpu/test_oah_from_qpimage_cupy.py Outdated
Comment thread tests/test_cupy_gpu/test_oah_from_qpimage_cupy.py Outdated
Comment thread tests/test_cupy_gpu/test_oah_from_qpimage_cupy.py Outdated
Comment thread tests/test_cupy_gpu/test_oah_from_qpimage_cupy.py Outdated
Comment thread tests/test_fourier_base.py Outdated
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Nov 4, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 33.33333% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.42%. Comparing base (4446d0d) to head (88fb701).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
qpretrieve/fourier/ff_cupy.py 11.11% 16 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #16      +/-   ##
==========================================
+ Coverage   85.75%   87.42%   +1.67%     
==========================================
  Files          11       13       +2     
  Lines         400      525     +125     
==========================================
+ Hits          343      459     +116     
- Misses         57       66       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@PinkShnack
Copy link
Copy Markdown
Contributor Author

Hey @paulmueller I made the changes you requested. Check out the new ci matrix. I added a relatively nice way of formatting it. Do you think we need the optional dep to be individually tested also? It baloons the number of jobs.

@paulmueller paulmueller marked this pull request as ready for review November 5, 2025 07:57
@paulmueller
Copy link
Copy Markdown
Member

IMO optional dependencies do not need to be tested individually.
The only thing left to do is make the CI tests pass (there are some errors).
If there is no GPU on GHA free plan, you can also remove the corresponding extra.
If there are issues with "Python 3.X" (which is now 3.14), you could also remove that.

@PinkShnack
Copy link
Copy Markdown
Contributor Author

PinkShnack commented Nov 5, 2025

IMO optional dependencies do not need to be tested individually.

Great

The only thing left to do is make the CI tests pass (there are some errors). If there are issues with "Python 3.X" (which is now 3.14), you could also remove that.

I assumed that 3.14 would work fine. But you know what they say when you assume...

If there is no GPU on GHA free plan, you can also remove the corresponding extra.

There is none. I will remove the extra with a comment explaining why

@paulmueller looks like this is ready. I removed 3.14, as packages such as cupy and scikit-image do not yet have wheels for it.

@paulmueller paulmueller merged commit 9fe81ab into main Nov 5, 2025
24 checks passed
@PinkShnack PinkShnack deleted the 10_cupy_fftfilter branch November 6, 2025 10:42
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.

3 participants