add support for DFT with onesided=True and inverse=True (irfft) #27028
Conversation
Signed-off-by: Simon Byrne <sbyrne@nvidia.com>
There was a problem hiding this comment.
Pull request overview
Adds CPU EP support for DFT with onesided=True and inverse=True (IRFFT semantics) and expands test coverage for IRFFT and rank-2 inputs.
Changes:
- Implement IRFFT handling in radix-2 FFT path (conjugate symmetry reconstruction + real-valued output).
- Implement IRFFT handling in Bluestein path (conjugate symmetry reconstruction + real-valued output).
- Add new unit tests for IRFFT (radix-2, Bluestein, round-trip) and rank-2 real/complex DFT inputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
onnxruntime/core/providers/cpu/signal/dft.cc |
Adds IRFFT support and adjusts shape/output handling for inverse && onesided. |
onnxruntime/test/providers/cpu/signal/signal_ops_test.cc |
Adds IRFFT test cases and rank-2 DFT/RFFT coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Simon Byrne <sbyrne@nvidia.com>
This fixes the onnxscript export for the irfft function. Fixes onnx/onnx#5920, and adds support to the changes in onnx/onnx#7574 and microsoft/onnxruntime#27028. Most of the diff is due to the onnx_opset generated code changes from onnx/onnx#5920. That can be removed if you would prefer. --------- Signed-off-by: Simon Byrne <sbyrne@nvidia.com>
Signed-off-by: Simon Byrne <sbyrne@nvidia.com>
Signed-off-by: Simon Byrne <sbyrne@nvidia.com>
Signed-off-by: Simon Byrne <sbyrne@nvidia.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // For opset 17-19, just pass through | ||
| } else { | ||
| // For opset 20, pass dft_length and axis to IRFFT | ||
| assert(graph_inputs.size() == 3); |
There was a problem hiding this comment.
Using plain assert(...) here can be compiled out in release builds, which would turn a violated assumption (e.g., unexpected graph_inputs size) into an out-of-bounds access when indexing graph_inputs[1/2]. Prefer ASSERT_*/EXPECT_* (gtest) or ORT_ENFORCE so the check remains active and fails with a clear message.
| assert(graph_inputs.size() == 3); | |
| ASSERT_EQ(graph_inputs.size(), static_cast<size_t>(3)); |
There was a problem hiding this comment.
assert is used elsewhere: is there a convention in the codebase?
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Simon Byrne <sbyrne@nvidia.com>
|
Hi @simonbyrne! |
Yes, you should use an existing optimized FFT library (e.g. FFTW or cuFFT) 😄 If you want a simpler option, you may just be able to call into PocketFFT (which is used by NumPy, and should already included as its a dependency) |
|
PocketFFT is actually header-only (https://github.com/mreineck/pocketfft), that would be the easiest option by far (FFTW is fast, but it is GPL licensed which might be a sticking point). |
2 participants
Description
Adds support for the DFT operator when onesided=True and inverse=True (corresponding to the irfft operation in numpy and pytorch).
Motivation and Context
This addresses issue onnx/onnx#5920, and adds support to the changes in onnx/onnx#7574.