Skip to content

Enable aten::fft_c2r #1488

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 8, 2025
Merged

Enable aten::fft_c2r #1488

merged 2 commits into from
Apr 8, 2025

Conversation

CuiYifeng
Copy link
Contributor

@CuiYifeng CuiYifeng commented Mar 19, 2025

Enable aten::fft_c2r with Hermitian Symmetry fixing for DFTI_CONJUGATE_EVEN_STORAGE.

Copy link
Contributor

@dvrogozh dvrogozh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is #1469 which was submitted earlier by @Wetitpig which has also fft_r2c implemented it seems. Which PR should actually be reviewed?

@CuiYifeng
Copy link
Contributor Author

CuiYifeng commented Mar 27, 2025

There is #1469 which was submitted earlier by @Wetitpig which has also fft_r2c implemented it seems. Which PR should actually be reviewed?

Please review this PR, which contains a fixing for Hermitian Symmetry. There are some failed cases with my old branches.
I will create another PR for aten::fft_r2c recently.
Glad to see the fix of normalization in #1469.

@Wetitpig
Copy link

There is #1469 which was submitted earlier by @Wetitpig which has also fft_r2c implemented it seems. Which PR should actually be reviewed?

Please review this PR, which contains a fixing for Hermitian Symmetry. There are some failed cases with my old branches. I will create another PR for aten::fft_r2c recently. Glad to see the fix of normalization in #1469.

Would you mind specifically mentioning which edge cases they failed? This version of HermitSymm looks like using much more other pytorch methods, which may hinder performance.
Thank you

@CuiYifeng
Copy link
Contributor Author

CuiYifeng commented Mar 27, 2025

There is #1469 which was submitted earlier by @Wetitpig which has also fft_r2c implemented it seems. Which PR should actually be reviewed?

Please review this PR, which contains a fixing for Hermitian Symmetry. There are some failed cases with my old branches. I will create another PR for aten::fft_r2c recently. Glad to see the fix of normalization in #1469.

Would you mind specifically mentioning which edge cases they failed? This version of HermitSymm looks like using much more other pytorch methods, which may hinder performance. Thank you

@Wetitpig For example, test_reference_1d_fft_irfft_xpu_complex64 and test_reference_nd_fft_irfftn_xpu_complex64. You can get the total failed list with pytest -sv test/xpu/test_spectral_ops_xpu.py.
I understand your concern about the performance of HermitSymm. Functionality first, HermitSymm with small ATen Ops plan to be optimized in another PR.

@CuiYifeng CuiYifeng mentioned this pull request Apr 7, 2025
@CuiYifeng CuiYifeng requested a review from xytintel April 8, 2025 01:46
@xytintel xytintel added this pull request to the merge queue Apr 8, 2025
Merged via the queue into main with commit 7bb07d7 Apr 8, 2025
19 of 20 checks passed
@xytintel xytintel deleted the yifeng/fft_c2r branch April 8, 2025 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants