Add FFT (fast fourier transform) operator #218
Conversation
| if window is not None: | ||
| selected_src_values = selected_src_values * window | ||
| fft_res = np.fft.fft(selected_src_values)[:num_spectral_lines] | ||
| ft_ampl = np.abs(fft_res).astype(np.float32) |
There was a problem hiding this comment.
It would be great to have an option to return real and imaginary parts instead of magnitude only (phase information is lost).
Maybe we can add the arg. in a future PR. But we have to consider that the number of output features changes, and naming too. Thoughts?
There was a problem hiding this comment.
+1 for phrase / real / img parts. There are multiple options available e.g., return multiple event-sets, return a dataclass of event set with the corresponding fields, having a input arg to specify what the output should be, rename the function "rfft", etc.
While we don't have complex operators like that for now, we will likely have some in the future. How to address this case is a great brainstorming question :)
|
|
||
| The operator returns the amplitude of each spectral line as | ||
| separate tp.float32 features named "a0", "a1", "a2", etc. By default, | ||
| `num_events // 2` spectral line are returned. |
There was a problem hiding this comment.
Conceptually, the FFT only makes sense with uniform sampling.
I'm not sure but maybe we should add a note here, or introduce a resample or check_sampling arguments. Any thoughts on this?
There was a problem hiding this comment.
Gonzalo found out there's a non-uniform fft:
https://www.mathworks.com/help/matlab/ref/double.nufft.html#mw_f93dca1a-78a2-4dd4-8b98-84462b3233c3
In python, there's this:
https://jyhmiinlin.github.io/pynufft/overview/pynufft.html
I didn't know about any of that but seems exciting! Let's talk about it tomorrow.
There was a problem hiding this comment.
What I propose:
- I'll rename the op from "fft" to "fast_fourier_transform" and require for "num_events" to be passed by wargs.
- In a next PR, I'll implement the "fourier_transform" operator (that support non-uniform sampling).
- Once we have a solution from the brainstorming, in a next PR, I'll make the update.
Does this make sense?
| num_events: int, | ||
| window: Optional[str] = None, | ||
| num_spectral_lines: Optional[int] = None, | ||
| ) -> EventSetOrNode: |
There was a problem hiding this comment.
The most common case is to have a hop_size parameter as well, usually a half or quarter of window_size.
Maybe it should be added in a future PR but the num_events name sounds weird to me. Maybe window_events or window_n? The fact that it's an int already makes pretty clear that this is not duration in seconds.
There was a problem hiding this comment.
- Adding "hop_size" argument.
- For the window_size, I want to make it super clear that this is not a window_length (both for code writer and readers). While I don't think "num_events" is very good, I think "window_size" is too close to "window_length". However, if we use "hop_size", "window_size" make sense...
WDYT?
Commonly used operation in TS processing.
Open question:
num_events. FFT is the first operation like that, and we can probably expect a few more. Q: How to make sure the user does not use "num_events" as a window_length.