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

Implement IRFFT as a CPU kernel #10127

Merged
merged 2 commits into from
May 24, 2017
Merged

Implement IRFFT as a CPU kernel #10127

merged 2 commits into from
May 24, 2017

Conversation

Czxck001
Copy link
Contributor

Mentioned in issue #386 and #9029. TF is now supporting FFT operators. However, there still remains IRFFT as a CPU kernel unimplemented. In this PR I made up the missing part of FFTCPU class, and get the CPU kernel for IRFFT work.
I remove the limitation of if test.is_gpu_available(cuda_only=True): in fft_ops_test.py, and passed all test cases by bazel test --config=opt -k //tensorflow/python/kernel_tests:fft_ops_test.

The code is linted by clang-format 3.8 using google code style.

@tensorflow-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@Czxck001
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

Copy link
Member

@rryan rryan left a comment

Choose a reason for hiding this comment

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

Just a style nit. Assuming the tests pass, LGTM. Thanks!

Eigen::DSizes<Eigen::DenseIndex, FFTRank + 1> startIndices;
auto sizes = input.dimensions();

// Calculate the shape of full-fft temporary tensor
Copy link
Member

Choose a reason for hiding this comment

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

style nit: please end sentences in comments with periods (here and below)
https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_Grammar

@rryan
Copy link
Member

rryan commented May 23, 2017

@tensorflow-jenkins test this please.

@maciekcc
Copy link

Jenkins test this please.

@Czxck001
Copy link
Contributor Author

Czxck001 commented May 24, 2017


99% tests passed, 1 tests failed out of 281

Total Test time (real) = 738.47 sec

The following tests FAILED:
	231 - C:/tf_jenkins/home/workspace/tensorflow-pr-win-cmake-py/tensorflow/python/training/saver_test.py (Failed)
Errors while running CTest

c:\tf_jenkins\home\workspace\tensorflow-pr-win-cmake-py\cmake_build>exit 8 
Build step 'Execute Windows batch command' marked build as failure
[Set GitHub commit status (universal)] ERROR on repos [] (sha:7a9126c) with context:tensorflow-pr-win-cmake-py
Unable to get pull request builder trigger!!
Setting status of 538e0ce284abadcbfd58ed8e0aa04715020d5add to FAILURE with url https://ci.tensorflow.org/job/tensorflow-pr-win-cmake-py/2493/ and message: 'FAILURE
 '
Using context: Windows Cmake Tests
Finished: FAILURE

It seems the failure of Windows Cmake Tests is unrelated to this PR.

@maciekcc maciekcc merged commit 2081920 into tensorflow:master May 24, 2017
@Czxck001 Czxck001 deleted the cpuirfft branch May 24, 2017 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants