Skip to content

Fix fft when any of the input dimensions is not aligned #6118

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.

Sign up for GitHub

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

Jump to bottom
Merged
ezyang merged 7 commits into from
Apr 10, 2018
Merged

Fix fft when any of the input dimensions is not aligned #6118

ezyang merged 7 commits into from
Apr 10, 2018

Conversation

@ssnl
Copy link
Collaborator

@ssnl ssnl commented Mar 29, 2018 •
edited
Loading

Discovered this bug while looking into the test fail @colesbury saw. In C2C or C2R transform, when any of the input dimension is not aligned to complex type (stride % 2 != 0), the third party library calls give incorrect results. Added those checks and tests for this.

Added test for ifft+fft == identity. Empirically this only increased test time by 0.1s.

@ezyang

ssnl
ssnl commented Mar 29, 2018
View reviewed changes
test/test_torch.py Outdated

This comment was marked as off-topic.

Sign in to view

@onnxbot-worker-1 onnxbot-worker-1 mentioned this pull request Mar 29, 2018
Closed
ezyang
ezyang reviewed Mar 30, 2018
View reviewed changes
aten/src/ATen/native/mkl/SpectralOps.cpp Outdated

This comment was marked as off-topic.

Sign in to view

ezyang
ezyang previously requested changes Mar 30, 2018
View reviewed changes
aten/src/ATen/native/cuda/SpectralOps.cu Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

colesbury
colesbury reviewed Mar 30, 2018
View reviewed changes
aten/src/ATen/native/mkl/SpectralOps.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang
ezyang reviewed Apr 1, 2018
View reviewed changes
aten/src/ATen/native/cuda/SpectralOps.cu Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@ezyang ezyang mentioned this pull request Apr 1, 2018
Open
@ssnl
Copy link
Collaborator Author

ssnl commented Apr 2, 2018

Addressed @ezyang 's comments.

I still couldn't believe that this is my first experience with optional and my first version just compiled without error. :O

@ssnl ssnl force-pushed the mkl_fft_strides branch 5 times, most recently from 6d60b71 to 8c80373 Compare April 3, 2018 21:15
@ssnl ssnl force-pushed the mkl_fft_strides branch from 934fed8 to 62736c6 Compare April 4, 2018 22:13
@ssnl
Copy link
Collaborator Author

ssnl commented Apr 4, 2018

@pytorchbot retest this please

1 similar comment
@ssnl
Copy link
Collaborator Author

ssnl commented Apr 5, 2018

@pytorchbot retest this please

@ssnl ssnl force-pushed the mkl_fft_strides branch from 62736c6 to 878b8fb Compare April 5, 2018 15:05
@ezyang
Copy link
Contributor

ezyang commented Apr 5, 2018

@pytorchbot retest this please

@ssnl
Copy link
Collaborator Author

ssnl commented Apr 5, 2018

@pytorchbot retest this please

1 similar comment
@ezyang
Copy link
Contributor

ezyang commented Apr 5, 2018

@pytorchbot retest this please

ezyang
ezyang approved these changes Apr 10, 2018
View reviewed changes
aten/src/ATen/native/cuda/SpectralOps.cu
//
// input[x, y, z] = input[((x * inembed[1] + y) * inembed[2] + z)]
//
// Above is the simplified the formulate ignoring the batch dimension. In fact,

This comment was marked as off-topic.

Sign in to view

aten/src/ATen/native/SpectralOpsUtils.h
// X[m, \omega] = X[m, N - \omega]*. We return only the first floor(N / 2) + 1
// values by default (onesided=True). This is also the assumption in libraries
// including cuFFT and MKL.
// [ NOTE ] Fourier Transform Congjugate Symmetry

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@ezyang ezyang merged commit 930f181 into pytorch:master Apr 10, 2018
@ssnl ssnl mentioned this pull request Apr 10, 2018
Merged
@ssnl ssnl deleted the mkl_fft_strides branch April 10, 2018 17:19
ssnl added a commit to ssnl/pytorch that referenced this pull request Apr 10, 2018
@ssnl
fix nits and typos from pytorch#6118
78f154d
ssnl added a commit that referenced this pull request Apr 11, 2018
@ssnl
[fft] [3 of 3] Implements backward of fft ifft rfft irfft (#5537)
0dff2b5 
* change irfft signal_sizes arg to be the last

* add docs for fft, ifft, rfft, irfft; update doc for stft

* fix typo in window function docs

* improve gradcheck error message

* implement backward of fft, ifft, rfft, irfft

* add grad tests for fft, ifft, rfft, irfft

* fix nits and typos from #6118

* address comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ezyang ezyang ezyang approved these changes

@colesbury colesbury colesbury left review comments

@apaszke apaszke Awaiting requested review from apaszke

@gchanan gchanan Awaiting requested review from gchanan

@soumith soumith Awaiting requested review from soumith

@zdevito zdevito Awaiting requested review from zdevito

Assignees

No one assigned

Labels

open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ssnl @ezyang @colesbury