Fix hfftn/ihfftn passing garbage dim values when dim is null#1563
Fix hfftn/ihfftn passing garbage dim values when dim is null#1563alinpahontu2912 wants to merge 5 commits intodotnet:mainfrom
Conversation
PyTorch's fft.hfft/hfft2/hfftn require complex input tensors. The tests were passing real-valued tensors (Float32/Float64), which causes 'NYI' errors in newer PyTorch versions. Fixed by using complex64/complex128 input types and ihfft for inverse verification. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The test method was incorrectly calling fft.hfft2() instead of fft.hfftn(), and using ihfft2() instead of ihfftn() for verification. Updated the TestOf attribute to match the correct function being tested. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Removed Float32HFFT and Float64HFFT as they were duplicates of ComplexFloat versions. Renamed Float32HFFT2/N and Float64HFFT2/N to ComplexFloat... to reflect input type. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The FFT functions (fft2, ifft2, hfft2, ihfft2, hfftn, ihfftn) created c10::IntArrayRef from a temporary initializer_list in a ternary expression. The backing data was destroyed at the end of the expression, leaving a dangling pointer. On Windows this caused garbage dimension values (e.g. 740900711656) to be passed to PyTorch, triggering 'Dimension out of range' errors. Fixed by declaring the default dim array with proper lifetime before the ternary expression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When dim is null, construct all-dimensions array (matching Python's
dim=None behavior) instead of relying on PyTorch's C++ default {-2,-1}.
Unlike fftn/ifftn which take OptionalIntArrayRef for dim, hfftn/ihfftn
take IntArrayRef (non-optional), so nullopt cannot be used. Building
the full dim range from the tensor's ndim correctly matches the Python
API semantics.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
alinpahontu2912
force-pushed
the
fix-hfft-real-input-test
branch
from
ef72c4f to
4bfd13b
Compare
There was a problem hiding this comment.
Pull request overview
Fixes a native interop bug in TorchSharp’s FFT bindings where hfftn/ihfftn could receive invalid (“garbage”) dimension values when dim is omitted, aligning default-dimension behavior with the fftn/ifftn pattern and updating tests accordingly.
Changes:
- Update native
THSTensor_hfftn/THSTensor_ihfftnto avoid passing invalid defaultdimvalues whendim == null. - Replace unsafe initializer-list based default dims in several 2D FFT wrappers with a stable local array-backed
IntArrayRef. - Adjust/rename FFT unit tests to use complex inputs for Hermitian FFT APIs and to call the correct
hfftn/ihfftnentry points.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/Native/LibTorchSharp/THSFFT.cpp |
Fixes default dim handling for hfftn/ihfftn and stabilizes default 2D dim storage to prevent dangling references. |
test/TorchSharpTest/TestTorchTensor.cs |
Updates Hermitian FFT tests to use complex inputs and corrects coverage for hfftn/ihfftn default-dim behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // hfftn takes IntArrayRef (not optional) for dim. When dim is null, | ||
| // use all dimensions to match Python's dim=None behavior. | ||
| std::vector<int64_t> allDims; |
There was a problem hiding this comment.
This new std::vector<int64_t> usage relies on transitive includes (THSFFT.cpp doesn't include directly). Please add an explicit #include in this file to avoid brittle build failures if upstream headers change.
| // hfftn takes IntArrayRef (not optional) for dim. When dim is null, | ||
| // use all dimensions to match Python's dim=None behavior. | ||
| std::vector<int64_t> allDims; |
There was a problem hiding this comment.
The PR description says to pass c10::nullopt for dim (to fully match PyTorch's dim=None behavior), but this implementation materializes an explicit [0..ndim-1] dim list instead. If the goal is to let PyTorch infer dimensions (including any interactions with the provided s argument), consider using the same optional/nullopt pattern as THSTensor_fftn/ifftn; otherwise please update the PR description to match the actual behavior.
| std::vector<int64_t> allDims; | ||
| if (dim == nullptr) { | ||
| auto ndim = (*tensor).dim(); | ||
| allDims.resize(ndim); | ||
| for (int64_t i = 0; i < ndim; i++) allDims[i] = i; |
There was a problem hiding this comment.
Same as THSTensor_hfftn: the PR description calls for passing c10::nullopt when dim is null, but here an explicit all-dim list is constructed. Consider aligning ihfftn with the fftn/ifftn optional/nullopt pattern (or update the PR description if explicit dims are intended).
Problem
ComplexFloat32HFFTN test fails with:
Dimension out of range (expected to be in range of [-4, 3], but got 464974753960)
Root Cause
THSTensor_hfftn and THSTensor_ihfftn hardcoded a 2-element default dim array {-2, -1} when dim was null. This pattern (copied from the 2D variants hfft2/ihfft2) is incorrect for the n-dimensional variants, which should pass c10::nullopt to let PyTorch use all dimensions, matching fftn/ifftn.
Fix
Replace the hardcoded default with c10::nullopt, matching the fftn/ifftn pattern.