Remove duplicate normalisation in FFT methods and enable relevant tests #1469
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merged
|
Due to resource constraints I have been testing only the "usual" use cases (i.e.
and vice versa), and the results seem to be consistent with those from the original pytorch run on CPU. |
|
@Wetitpig Thanks for your PR. Since I have completed most of the debugging (seems these cases are not fixed in this PR) based on my old branches, |
Wetitpig
changed the title
at::_fft_r2c and at::_fft_c2r methods with OneMKL
|
Now targeting only the repeated normalisation in |
CuiYifeng
requested changes
Apr 30, 2025
There was a problem hiding this comment.
Glad to see the refinement. Please ensure that the changes won't break the following cases:
Show Failed cases in op_ut with skip
=========================================================================
test_ops_xpu.py::TestCommonXPU::test_out_fft_fft2_xpu_float32
test_ops_xpu.py::TestCommonXPU::test_out_fft_fft_xpu_float32
test_ops_xpu.py::TestCommonXPU::test_out_fft_fftn_xpu_float32
test_ops_xpu.py::TestCommonXPU::test_out_fft_hfft2_xpu_float32
test_ops_xpu.py::TestCommonXPU::test_out_fft_hfft_xpu_float32
test_ops_xpu.py::TestCommonXPU::test_out_fft_hfftn_xpu_float32
test_ops_xpu.py::TestCommonXPU::test_out_fft_ifft2_xpu_float32
test_ops_xpu.py::TestCommonXPU::test_out_fft_ifftn_xpu_float32
test_ops_xpu.py::TestCommonXPU::test_out_fft_irfft2_xpu_float32
test_ops_xpu.py::TestCommonXPU::test_out_fft_irfft_xpu_float32
test_ops_xpu.py::TestCommonXPU::test_out_fft_irfftn_xpu_float32
test_ops_xpu.py::TestCommonXPU::test_out_fft_rfft2_xpu_float32
test_ops_xpu.py::TestCommonXPU::test_out_fft_rfft_xpu_float32
test_ops_xpu.py::TestCommonXPU::test_out_fft_rfftn_xpu_float32
test_ops_xpu.py::TestCommonXPU::test_out_warning_fft_fft2_xpu
test_ops_xpu.py::TestCommonXPU::test_out_warning_fft_fft_xpu
test_ops_xpu.py::TestCommonXPU::test_out_warning_fft_fftn_xpu
test_ops_xpu.py::TestCommonXPU::test_out_warning_fft_hfft2_xpu
test_ops_xpu.py::TestCommonXPU::test_out_warning_fft_hfft_xpu
test_ops_xpu.py::TestCommonXPU::test_out_warning_fft_hfftn_xpu
test_ops_xpu.py::TestCommonXPU::test_out_warning_fft_ifft2_xpu
test_ops_xpu.py::TestCommonXPU::test_out_warning_fft_ifftn_xpu
test_ops_xpu.py::TestCommonXPU::test_out_warning_fft_irfft2_xpu
test_ops_xpu.py::TestCommonXPU::test_out_warning_fft_irfft_xpu
test_ops_xpu.py::TestCommonXPU::test_out_warning_fft_irfftn_xpu
test_ops_xpu.py::TestCommonXPU::test_out_warning_fft_rfft2_xpu
test_ops_xpu.py::TestCommonXPU::test_out_warning_fft_rfft_xpu
test_ops_xpu.py::TestCommonXPU::test_out_warning_fft_rfftn_xpu
test_meta_xpu.py::TestMetaXPU::test_dispatch_meta_outplace_fft_fft_xpu_bool
test_meta_xpu.py::TestMetaXPU::test_dispatch_meta_outplace_fft_fft_xpu_float32
test_meta_xpu.py::TestMetaXPU::test_dispatch_meta_outplace_fft_fft_xpu_float64
test_meta_xpu.py::TestMetaXPU::test_dispatch_meta_outplace_fft_fft_xpu_int16
test_meta_xpu.py::TestMetaXPU::test_dispatch_meta_outplace_fft_fft_xpu_int32
test_meta_xpu.py::TestMetaXPU::test_dispatch_meta_outplace_fft_fft_xpu_int64
test_meta_xpu.py::TestMetaXPU::test_dispatch_meta_outplace_fft_fft_xpu_int8
test_meta_xpu.py::TestMetaXPU::test_dispatch_meta_outplace_fft_fft_xpu_uint8
test_meta_xpu.py::TestMetaXPU::test_dispatch_meta_outplace_stft_xpu_float32
test_meta_xpu.py::TestMetaXPU::test_dispatch_meta_outplace_stft_xpu_float64
test_meta_xpu.py::TestMetaXPU::test_dispatch_symbolic_meta_outplace_all_strides_fft_fft_xpu_float32
test_meta_xpu.py::TestMetaXPU::test_dispatch_symbolic_meta_outplace_all_strides_stft_xpu_float32
test_meta_xpu.py::TestMetaXPU::test_dispatch_symbolic_meta_outplace_fft_fft_xpu_bool
test_meta_xpu.py::TestMetaXPU::test_dispatch_symbolic_meta_outplace_fft_fft_xpu_float32
test_meta_xpu.py::TestMetaXPU::test_dispatch_symbolic_meta_outplace_fft_fft_xpu_float64
test_meta_xpu.py::TestMetaXPU::test_dispatch_symbolic_meta_outplace_fft_fft_xpu_int16
test_meta_xpu.py::TestMetaXPU::test_dispatch_symbolic_meta_outplace_fft_fft_xpu_int32
test_meta_xpu.py::TestMetaXPU::test_dispatch_symbolic_meta_outplace_fft_fft_xpu_int64
test_meta_xpu.py::TestMetaXPU::test_dispatch_symbolic_meta_outplace_fft_fft_xpu_int8
test_meta_xpu.py::TestMetaXPU::test_dispatch_symbolic_meta_outplace_fft_fft_xpu_uint8
test_meta_xpu.py::TestMetaXPU::test_dispatch_symbolic_meta_outplace_stft_xpu_float32
test_meta_xpu.py::TestMetaXPU::test_dispatch_symbolic_meta_outplace_stft_xpu_float64
test_meta_xpu.py::TestMetaXPU::test_meta_outplace_fft_fft_xpu_bool
test_meta_xpu.py::TestMetaXPU::test_meta_outplace_fft_fft_xpu_float32
test_meta_xpu.py::TestMetaXPU::test_meta_outplace_fft_fft_xpu_float64
test_meta_xpu.py::TestMetaXPU::test_meta_outplace_fft_fft_xpu_int16
test_meta_xpu.py::TestMetaXPU::test_meta_outplace_fft_fft_xpu_int32
test_meta_xpu.py::TestMetaXPU::test_meta_outplace_fft_fft_xpu_int64
test_meta_xpu.py::TestMetaXPU::test_meta_outplace_fft_fft_xpu_int8
test_meta_xpu.py::TestMetaXPU::test_meta_outplace_fft_fft_xpu_uint8
yaml/xpu_functions.yaml
Outdated
Comment on lines
750
to
755
| - _fft_c2c | ||
| - _fft_c2c.out | ||
| - _fft_c2r | ||
| - _fft_c2r.out | ||
| - _fft_r2c | ||
| - _fft_r2c.out |
There was a problem hiding this comment.
Please remove these lines. yaml/xpu_functions.yaml is replaced by yaml/native/native_functions.yaml.
Comment on lines
79
to
80
| desc.set_value(config_param::FWD_STRIDES, bwd_strides.data()); | ||
| desc.set_value(config_param::BWD_STRIDES, fwd_strides.data()); |
There was a problem hiding this comment.
FWD_STRIDES <- bwd_strides.data() and BWD_STRIDES <- fwd_strides.data() may cause confusion. Please rename fwd_strides and bwd_strides, or try other methods.
Comment on lines
369
to
373
| auto result = _fft_c2c_mkl( | ||
| self, dim, static_cast<int64_t>(fft_norm_mode::none), forward); | ||
| at::native::resize_output(out, result.sizes()); | ||
| return impl::_fft_apply_normalization_out( | ||
| out, result, normalization, result.sizes(), dim); | ||
| out.copy_(result); | ||
| return out; |
There was a problem hiding this comment.
The semantics before and after modification are not equivalent. Please note that the normalization parameter above is fft_norm_mode::none. Seems you want to apply auto result = _fft_c2c_mkl(self, dim, normalization, forward);, right?
Comment on lines
-478
to
+448
| return impl::_fft_apply_normalization_out( | ||
| out, result, normalization, result.sizes(), dim); | ||
| out.copy_(result); | ||
| return out; |
Comment on lines
543
to
545
| auto result = _fft_r2c_mkl( | ||
| self, dim, static_cast<int64_t>(fft_norm_mode::none), /*onesided=*/true); | ||
|
|
||
| if (onesided) { | ||
| return impl::_fft_apply_normalization_out( | ||
| out, result, normalization, self.sizes(), dim); | ||
| } | ||
|
|
||
| at::native::resize_output(out, self.sizes()); | ||
|
|
||
| auto last_dim = dim.back(); | ||
| auto last_dim_halfsize = result.sizes()[last_dim]; | ||
| auto out_slice = out.slice(last_dim, 0, last_dim_halfsize); | ||
|
|
||
| impl::_fft_apply_normalization_out( | ||
| out_slice, result, normalization, self.sizes(), dim); | ||
| at::native::_fft_fill_with_conjugate_symmetry_(out, dim); | ||
| at::native::resize_output(out, result.sizes()); | ||
| out.copy_(result); |
Comment on lines
60
to
61
| std::vector<int64_t> fwd_strides(istrides.cbegin(), istrides.cbegin() + signal_ndim + 1), | ||
| bwd_strides(ostrides.cbegin(), ostrides.cbegin() + signal_ndim + 1); |
There was a problem hiding this comment.
Just a reminder of columns.
|
@Wetitpig Please also update PR description, thanks. |
|
The |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There is a problem of redundant normalisations in the
_fft_(c2c|c2r|r2c)_mkl_outfunctions, which should be fixed by this PR. Some loops/conditional branches have also been simplified.