add support for DFT with onesided=True and inverse=True (irfft)#27028
add support for DFT with onesided=True and inverse=True (irfft)#27028justinchuby merged 12 commits intomicrosoft:mainfrom
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.
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). |
|
Bumping this. Does this require a new release of ONNX to support? |
Sorry I lost track of this. I think it is good to merge, thanks |
Contributions welcome! The current implementation is very unoptimized. |
|
I think since onnxruntime uses onnx shape inferencer and it currently raises an error, we still do need a new onnx version (end of Feb) for the models to run. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
onnxruntime/core/providers/cpu/signal/dft.cc:447
number_of_samplesis anint64_t, but whendft_lengthis provided it is assigned viastatic_cast<int>(...). This can truncate values on platforms whereintis 32-bit and lead to incorrect output shapes/loop bounds for largedft_length. Assign asint64_t(no narrowing cast) and keep the existing> 0validation on the full 64-bit value.
const auto& dft_length_shape = dft_length->Shape();
ORT_RETURN_IF(!dft_length_shape.IsScalar(), "dft_length must be a scalar value.");
number_of_samples = static_cast<int>(signal::get_scalar_value_from_tensor<int64_t>(dft_length));
ORT_RETURN_IF(number_of_samples <= 0, "dft_length must be greater than zero.");
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
onnx 1.21.0, which contains onnx/onnx#7574, is released two weeks ago. Can this be pushed forward? Missing support for changed DFT nodes in onnxruntime forces us to use older onnxscript and torch, and thus fixes from newer libraries cannot be used. |
|
Thanks for the reminder. @titaiwangms do you know if we have the new onnx dependency in? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@simonbyrne could you check review comments? |
Head branch was pushed to by a user without write access
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Looks like there are errors: https://github.com/microsoft/onnxruntime/actions/runs/24424918958/job/71375726468?pr=27028 could you fix or skip? |
|
I'm not sure why they're failing on that platform. |
|
Is there a way to debug? or skip? |
Looks like DirectML provider has its own shape inferrer and does not allow onesided=True && inverse=True yet: I assume the shape inferrer is related, as the file in the error message ( Here's an excerpt of CI logs: |
CI Failure Diagnosis (Copilot)1. Windows GPU DML CI Pipeline — 8 IRFFT test failuresRoot cause: The DirectML (DML) execution provider has its own shape inferrer for DFT that explicitly rejects if (isInverse && isOnesided)
{
throw new std::exception("onesided and inverse attributes cannot be enabled at the same time");
}When the DML provider is registered in the test runner, it intercepts the DFT op and its shape inferrer throws before the CPU kernel can run, causing all 8 IRFFT tests to fail. 2. wasm_Release / build-wasm — same 8 IRFFT test failuresThe WASM build has the same 8 test failures. There is no JSEP/WebNN DFT kernel, so the tests fall through to the CPU provider compiled to WASM via Emscripten. Since all native CPU builds (Linux x64, Windows x64, ARM64, etc.) pass, the issue is WASM-specific. Without the detailed error messages (shape mismatch vs value mismatch vs kernel error), the exact root cause is uncertain, but likely candidates are:
Suggested FixesFor DML (definitive fix): Exclude the DML execution provider from IRFFT tests since DML does not yet support For WASM: The detailed WASM error messages would help narrow down the exact cause. If the failures are similar to DML (an EP-level rejection), the same (Credit to @yan12125 who also identified the DML shape inferrer issue in an earlier comment.) |
|
For wasm_Release build, somehow I can download logs on my phone but not on laptop. Anyway, here are excerpted logs: Neither looks related to DFT. I guess merging with main again will fix it. |
|
i've merged in main, it needs an approval to run CI again |
Head branch was pushed to by a user without write access
|
Regarding "Linux TensorRT CI", this pipeline has been failing for a while until a few hours ago: |
|
bump? |
|
For "ONNX Runtime WebGPU Builds", apparent all failed tests are caused by DXGI_ERROR_DEVICE_REMOVED. For example, This looks more like an issue in CI builders, not this pull request. I'm not sure if pull requests are allowed to be merged without getting green in all CI builders or not. If not, unstable CI can cause issues for contributors. Maybe @justinchuby can comment on PR merging policy. |
|
/azp run Linux QNN CI Pipeline,Win_TRT_Minimal_CUDA_Test_CI,Windows ARM64 QNN CI Pipeline,Windows GPU Doc Gen CI Pipeline,Windows x64 QNN CI Pipeline |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Thanks a lot! |
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.