Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cjolif
Copy link

@cjolif cjolif commented May 19, 2024

Fixes #126649

Copy link

pytorch-bot bot commented May 19, 2024

🔗 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.

✅ No Failures

As of commit 87351e4 with merge base aa6de76 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

Copy link

linux-foundation-easycla bot commented May 19, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: cjolif / name: Christophe Jolif (87351e4)

@pytorch-bot pytorch-bot bot added the release notes: mps Release notes category label May 19, 2024
@drisspg drisspg added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 20, 2024
@kulinseth
Copy link
Collaborator

Thanks for the PR @cjolif , can you add a test-case which this is fixing ?

@cjolif
Copy link
Author

cjolif commented May 21, 2024

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.

@cjolif
Copy link
Author

cjolif commented May 21, 2024

The test actually already exists:

def test_fft_round_trip(self, device, dtype):
. However I think because of onlyNativeDeviceTypes decorator it does not run on MPS.

@cjolif
Copy link
Author

cjolif commented May 21, 2024

ok @kulinseth I think I got it. I made sure the test now run on MPS. Let me know what you think?

@cjolif
Copy link
Author

cjolif commented May 23, 2024

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.

@cjolif
Copy link
Author

cjolif commented May 23, 2024

Actually #125328 seems to be breaking the test while my fix was not.

(.venv_main) ➜  pytorch git:(c2r_fft_mps_fix) ✗ pytest -s test/test_spectral_ops.py -k "test_fft_round_trip"
================================================================================================ test session starts =================================================================================================
platform darwin -- Python 3.10.14, pytest-8.2.1, pluggy-1.5.0
rootdir: /Users/cjolif/OSS/pytorch
configfile: pytest.ini
plugins: hypothesis-6.102.5
collected 542 items / 530 deselected / 12 selected
Running 12 items in this shard

test/test_spectral_ops.py .s.s..ss.sFs

====================================================================================================== FAILURES ======================================================================================================
_____________________________________________________________________________________ TestFFTMPS.test_fft_round_trip_mps_float32 _____________________________________________________________________________________
Traceback (most recent call last):
  File "/Users/cjolif/.pyenv/versions/3.10.14/lib/python3.10/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/Users/cjolif/.pyenv/versions/3.10.14/lib/python3.10/unittest/case.py", line 591, in run
    self._callTestMethod(testMethod)
  File "/Users/cjolif/.pyenv/versions/3.10.14/lib/python3.10/unittest/case.py", line 549, in _callTestMethod
    method()
  File "/Users/cjolif/Personnal/Playground/.venv_main/lib/python3.10/site-packages/torch/testing/_internal/common_utils.py", line 2756, in wrapper
    method(*args, **kwargs)
  File "/Users/cjolif/Personnal/Playground/.venv_main/lib/python3.10/site-packages/torch/testing/_internal/common_device_type.py", line 419, in instantiated_test
    result = test(self, **param_kwargs)
  File "/Users/cjolif/Personnal/Playground/.venv_main/lib/python3.10/site-packages/torch/testing/_internal/common_device_type.py", line 1028, in dep_fn
    return fn(slf, *args, **kwargs)
  File "/Users/cjolif/OSS/pytorch/test/test_spectral_ops.py", line 232, in test_fft_round_trip
    self.assertEqual(x, y, exact_dtype=(
  File "/Users/cjolif/Personnal/Playground/.venv_main/lib/python3.10/site-packages/torch/testing/_internal/common_utils.py", line 3639, in assertEqual
    raise error_metas.pop()[0].to_error(
AssertionError: Tensor-likes are not close!

Mismatched elements: 66 / 67 (98.5%)
Greatest absolute difference: 3.966374397277832 at index (20,) (up to 1e-05 allowed)
Greatest relative difference: 64.65916442871094 at index (4,) (up to 1.3e-06 allowed)

To execute this test, run the following from the base repo dir:
     python test/test_spectral_ops.py -k TestFFTMPS.test_fft_round_trip_mps_float32

I will look into it more and come back here.

@cjolif
Copy link
Author

cjolif commented May 23, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open source release notes: mps Release notes category topic: not user facing topic category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

torch.fft.irfft(torch.fft.rrft) is not identity on MPS
4 participants