Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Faster fft with pyfftw #281

Merged
merged 3 commits into from
Nov 8, 2024
Merged

Faster fft with pyfftw #281

merged 3 commits into from
Nov 8, 2024

Conversation

ljgray
Copy link
Contributor

@ljgray ljgray commented Nov 5, 2024

This adds a basic class and api to do ffts using the fftw backend, which is implemented by pyfftw and is quite a bit faster than the backend used by numpy.fft and scipy.fft.

In particular, I found that fftw scales much better when multiple cores are available, but even single-threaded benchmarks show a speedup of around 3x.

The pyfftw api is a bit cumbersome to use, so I put this module together to try to abstract it a bit for some common fft functions with minimal overhead.

So far, I've just implemented a few functions which I've found useful. It's easy to expand as needed, but I want to keep this fairly minimal - as things get more complex, there's less of a reason to re-implement them.

@ljgray ljgray requested a review from ketiltrout November 5, 2024 00:41
@ljgray ljgray force-pushed the ljg/mpi-cpu-count branch from a39fc27 to f75db33 Compare November 5, 2024 00:51
@ljgray ljgray marked this pull request as draft November 7, 2024 01:16
@ljgray ljgray changed the title Helper function to get cpu count per MPI process Faster fft with pyfftw Nov 7, 2024
@ljgray ljgray force-pushed the ljg/mpi-cpu-count branch 6 times, most recently from ba8a8ad to e069d2f Compare November 7, 2024 20:35
@ljgray ljgray marked this pull request as ready for review November 7, 2024 20:35
Copy link
Member

@ketiltrout ketiltrout left a comment

Choose a reason for hiding this comment

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

Other than that, I think this looks okay as an implementation.

Though, what do I gain by setting forward or backward to false in the initialiser? Is it just a runtime savings?

Some unit tests, even very rudimentary ones would be nice.

Also, making pyfftw a hard requirement, forces anyone wanting to install caput at all to also install the FFTW clib, even if they're not FFT-ing with caput. I would prefer it be optional (with the understanding that not installing it means this module won't work). As-is this is going to force us to install FFTW on, say, wind, just so we can access ch_ephem.

caput/fftw.py Outdated Show resolved Hide resolved
caput/fftw.py Outdated Show resolved Hide resolved
caput/fftw.py Outdated Show resolved Hide resolved
caput/fftw.py Show resolved Hide resolved
@ljgray
Copy link
Contributor Author

ljgray commented Nov 7, 2024

Re. the forward and backward arguments - as you guessed, the idea was just for runtime savings, especially for small arrays

@ljgray ljgray force-pushed the ljg/mpi-cpu-count branch 3 times, most recently from 975d2bf to 38dd756 Compare November 7, 2024 22:38
@ljgray ljgray requested a review from ketiltrout November 7, 2024 22:40
@ljgray
Copy link
Contributor Author

ljgray commented Nov 7, 2024

I've added unit tests and made the pyfftw dependency optional. Note that I've also restricted this module to complex->complex transforms only for now - this is mostly due to laziness because of the extra overhead to manage differing array sizes for real->complex fft and complex->real ifft, and the fact that I don't really need it, but I guess we could implement it down the road if anyone want

Copy link
Member

@ketiltrout ketiltrout left a comment

Choose a reason for hiding this comment

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

Very nice.

I guess we could implement it down the road if anyone want

Yeah, that definitely feels like something to leave until someone actually wants it for something.

caput/fftw.py Outdated Show resolved Hide resolved
@ljgray ljgray force-pushed the ljg/mpi-cpu-count branch from 38dd756 to 09ce0aa Compare November 7, 2024 23:06
@ljgray ljgray force-pushed the ljg/mpi-cpu-count branch from 09ce0aa to 496d6fa Compare November 7, 2024 23:25
@ljgray ljgray merged commit 0a38d5d into master Nov 8, 2024
7 checks passed
@ljgray ljgray deleted the ljg/mpi-cpu-count branch November 8, 2024 19:06
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants