[ROCM] enable fft tests #60313
[ROCM] enable fft tests #60313
Conversation
💊 CI failures summary and remediationsAs of commit 9aa3713 (more details on the Dr. CI page and at hud.pytorch.org/pr/60313): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 Preview docs built from this PR This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
Codecov Report
@@ Coverage Diff @@
## master #60313 +/- ##
==========================================
- Coverage 76.23% 76.23% -0.01%
==========================================
Files 2059 2059
Lines 204892 204892
==========================================
- Hits 156201 156200 -1
- Misses 48691 48692 +1 |
@malfet rocFFT supports complex input. What that check is doing is seeing if the input will invoke hipfftExecC2R or hipfftExecZ2D and if it does apply a certain modification to the input. |
Co-authored-by: Nikita Shulga <nikita.shulga@gmail.com>
|
@malfet I added your suggestions and everything seems to pass. |
|
@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| # generate Hermitian symmetric input using rfftn | ||
| rfft_output = np.fft.rfftn(np_input_real) | ||
|
|
||
| return torch.from_numpy(rfft_output) |
There was a problem hiding this comment.
@micmelesse from_numpy will always return a CPU tensor, right? So this never runs any tests on rocm.
There was a problem hiding this comment.
@peterbell10 I didn't notice that in the documentation. Hopefully, the op forces it to GPU. Either way, I will create a pr today to explicitly force the tensor to GPU.
There was a problem hiding this comment.
The ops do not force inputs to the correct device or dtype, they just call the corresponding torch function.
There was a problem hiding this comment.
This was was fixed in #61073
Summary: This PR fixes a bug in #60313. Where the tensors generated by _generate_valid_rocfft_input are on the cpu instead of the gpu. This was due to using numpy to generate tensors and converting it to pytorch using torch.from_numpy. This leads to the generated tensors staying on the cpu. We now generate the tensors using pytorch itself which carries over the device type of the input tensors to the generated tensor. Pull Request resolved: #61073 Reviewed By: H-Huang Differential Revision: D29668418 Pulled By: malfet fbshipit-source-id: ce2025c26d079c15603a89b9bf7878f48d73155e
|
@peterbell10 Are you the owner of these fft unit tests? I'd like to request that we consider making the "workaround" in this PR be actually considered as a proper fix for the inputs to these unit tests. The original unit test inputs were actually testing undefined behaviour even according to cufft documentation, as mentioned here: https://docs.nvidia.com/cuda/cufft/index.html#fft-types Given that, wouldn't it be an improvement to the unit tests to provide inputs that have defined behaviour instead of relying on undefined behaviour that could change down the line? If needed, we can go over the technical details in a call. |
7 participants
This PR enables fft tests on ROCM. It contains a function that generates a valid input for fft tests that call hipfftExecC2R or hipfftExecZ2D. With this helper function we are able to fix a number of fft tests. This brings a close to the series of fft PRs enabling fft tests on ROCM.