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

FFT for convolution #43

Open
ViliamVadocz opened this issue Apr 17, 2021 · 8 comments
Open

FFT for convolution #43

ViliamVadocz opened this issue Apr 17, 2021 · 8 comments

Comments

@ViliamVadocz
Copy link

I wanted to use this library for a CNN. After a quick look at the source, it doesn't look like you use the the FFT algorithm for fast convolution. It is probably worth implementing.

@xd009642
Copy link
Member

Not currently, I went for a simpler implementation to start with. Also, I'm not sure how well FFT implementations perform for different sized inputs and whether it will really lead to gains for common use cases i.e. the default filters implemented in this crate 🤔.

I've got a lot on my plate right now so probably won't have time to work on this. However, I'm always open to PRs and can offer reviews and guidance. Any PR would also need benchmarks included as well to help measure the performance change. Alternatively, maybe someone on the cv discord would be willing to help. I'll post a link to this issue there https://discord.gg/N82kexYM

@xd009642
Copy link
Member

A comment on the discord:

I believe it reduces the complexity mainly when matrices sizes get bigger. The price of FFT is N log(N) and kernel convolutions are then element-wise multiplications so N. While basic convolution of a Kernel containing K elements (say K = 5x5 = 25) involves N * K multiplications. So as soon as K reaches log(N) it's worth. Imagine we have a full HD image, so 2 million pixels. Log(2000000) = 14.5 = roughly 4x4 kernel. So basically, as soon as your convolution kernel gets bigger than 3x3 it's worth it (roughly)

So the original implementation should stay but maybe be called something like SpatialConvolutionExt, add an FftConvolutionExt and then have ConvolutionExt pick the appropriate one based on input sizes etc

@TYPEmber
Copy link

TYPEmber commented May 2, 2024

Hey, I've made a crate provides N-Dimension FFT acceleration convolution for ndarray.
Maybe ndarray-vision could depend on my crate.
https://crates.io/crates/ndarray-conv
And there is the benchmark result:
image

@TYPEmber
Copy link

TYPEmber commented May 2, 2024

And ndarray-conv also provides normal convolution and a lot of padding and convolution mode.

@xd009642
Copy link
Member

xd009642 commented May 2, 2024

@TYPEmber I've got no issues with depending on your crate, would you be willing to help out with a PR? 👀

@TYPEmber
Copy link

TYPEmber commented May 2, 2024

Of course, but would you like to expose ndarray-conv's interface directly?

@xd009642
Copy link
Member

xd009642 commented May 2, 2024

I've had a brief look and yeah I'm happy to expose the interface directly - it'll be a minor bump when it's released but that's fine

@TYPEmber
Copy link

TYPEmber commented May 2, 2024

OK, I will try to make a PR recently.

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

No branches or pull requests

3 participants