-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Make sure c2r is actually configured for backward operation #126651
base: main
Are you sure you want to change the base?
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/126651
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit 87351e4 with merge base aa6de76 (): FLAKY - The following job failed but was likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
Thanks for the PR @cjolif , can you add a test-case which this is fixing ? |
Sure @kulinseth will do that. Was just trying to figure out a bit more how test PyTorch test system is built to put things at the right place. If there are some docs around this you can point me to it might help accelerate my discovery. |
The test actually already exists: pytorch/test/test_spectral_ops.py Line 179 in fed536d
|
ok @kulinseth I think I got it. I made sure the test now run on MPS. Let me know what you think? |
Meanwhile #125328 merged this fix (with additional fix). Still the roundtrip FFT test is not included in that fix. So I will rebase this PR and just add the roundtrip FFT test as it seems useful to avoid future regression and I don't see why it is not running on MPS. |
Actually #125328 seems to be breaking the test while my fix was not.
I will look into it more and come back here. |
Hmm actually I probably mixed something up. The test pass. So @kulinseth I think you can merge that one to add the roundtrip test on MPS that is skipped today. |
Fixes #126649