Conversation
src/fft2d.rs
Outdated
| pub struct FFT2D<T: FftNum> { | ||
| fft0: Arc<dyn Fft<T>>, | ||
| fft1: Arc<dyn Fft<T>>, | ||
| phantom: PhantomData<T>, |
There was a problem hiding this comment.
PhantomData is only needed if nothing else references the type, so this can be removed
There was a problem hiding this comment.
At some point, I was trying to have generic FFTs here, and I forgot to remove 🤔
I ended up going with Arc<dyn Fft<T>> just because it was easier
src/fft2d.rs
Outdated
| } | ||
| pub fn get_immutable_scratch_len(&self) -> usize { | ||
| let [w, h] = self.len(); | ||
| lcm( |
There was a problem hiding this comment.
I don't quite understand what the LCM accomplishes here. Would self.fft0.get_immutable_scratch_len().max(self.fft1.get_immutable_scratch_len()) suffice?
There was a problem hiding this comment.
I think I swapped scratch with buffer... but the max should work fine
|
RustFFT is a pretty old project now. I wrote the original code almost 10 years ago. Way back when it was in active development, const generics weren't available yet, and without those, any utility for generic m-d ffts would require allocation, and kept me from committing to it. Now that const generics are available, I think that's the way to go for a multi-dimensional FFT utility. What I'd like to see is a struct just like this, but instead of just being pure 2D, it takes a const generic for number of dimensions. The hardest part (for me at least) would be implementing a generic multidimensional transpose. But the fact that that's hard means it would be a good value to provide to library users. |
|
Don't worry about the 1.61 failures that's unrelated to this PR, I forsee upgrading our MSRV in the next release so we aren't falling too behind utilities like this. |
|
I was thinking whether it would be good to make it more abstract, and that's certainly possible, but I thought most applications would only need 2D. You'd want a generic version before merging though, eh? |
|
Should this be part of RustFFT, or a separate crate? A separate crate can make breaking changes much easier, might be useful, especially in the beginning. When I started on the realfft crate I had some ideas that it should be part of RustFFT, but it has been quite useful to have it separate. |
|
More often than not, people want to use FFT for images, so I think that keeping it in the same library makes more sense than requiring someone to hunt for two libraries. It's also a relatively small chunk of code (for now). If it were to become bigger, then it may make sense to split it out, but I don't want to make a micro-library. As for versioning/stability within a crate, it could be hidden behind a feature flag temporarily, although that's a bit of a pain. |
3 participants
Add utilities for computing 2D FFTs
I didn't add doc comments yet, since I'm not sure if this is the way that the author would want this written. If it is, then I can create doc comments and clean it up a bit more.
Doesn't address 3D transforms i.e. (#85), but fixes #146.