Skip to content
This repository has been archived by the owner on Aug 15, 2019. It is now read-only.

Add rfft op #1351

Merged
merged 6 commits into from
Nov 1, 2018
Merged

Add rfft op #1351

merged 6 commits into from
Nov 1, 2018

Conversation

Lewuathe
Copy link
Contributor

@Lewuathe Lewuathe commented Oct 26, 2018

Description

FEATURE

Add rfft op. Please see tensorflow/tfjs#782


For repository owners only:

Please remember to apply all applicable tags to your pull request.
Tags: FEATURE, BREAKING, BUG, PERF, DEV, DOC, SECURITY

For more info see: https://github.com/tensorflow/tfjs/blob/master/DEVELOPMENT.md


This change is Reviewable

Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @Lewuathe)


src/ops/spectral_ops.ts, line 120 at r1 (raw file):

    const complexInput = complex(input, zeros).as2D(batch, innerDimensionSize);

    const ret = ENV.engine.runKernel(

Actually we need to add a kernel for rfft because TensorFlow has a C API for this. We also can make this much more efficient because we're just throwing out half of the values when we could just run the shaders over half instead of doing the computation and throwing it out.

Does that make sense? Let me know if you need guidance there.

If you want, we can do this in a follow up since this is already better than what we have now. Totally happy to do that! :)


src/ops/spectral_ops.ts, line 121 at r1 (raw file):

    const ret = ENV.engine.runKernel(
      backend => backend.fft(complexInput), {input});

this should be {complexInput}


src/ops/spectral_ops.ts, line 126 at r1 (raw file):

    const half = Math.floor(innerDimensionSize / 2) + 1;
    const realValues = real(ret);
    const imagValues = imag(ret)

missing a semicolon which is making the linter fail :)


src/ops/spectral_ops_test.ts, line 235 at r1 (raw file):

});

describeWithFlags('2D RFFT', WEBGL_ENVS, () => {

can you add one more unit test for an odd inner dimension?

Copy link
Contributor Author

@Lewuathe Lewuathe left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @Lewuathe)


src/ops/spectral_ops.ts, line 120 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

Actually we need to add a kernel for rfft because TensorFlow has a C API for this. We also can make this much more efficient because we're just throwing out half of the values when we could just run the shaders over half instead of doing the computation and throwing it out.

Does that make sense? Let me know if you need guidance there.

If you want, we can do this in a follow up since this is already better than what we have now. Totally happy to do that! :)

That makes sense. If you already have an idea of the implementation, it would be great. Could you do that as the followup?
Thank you so much!

Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2.
Reviewable status: 0 of 1 approvals obtained

Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

By the way -- one high level question, I noticed your code doesn't follow our clang-format, do you use vscode? We have the repo set up such that if you use vscode from the root it will automatically format to our style.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @Lewuathe)


src/ops/spectral_ops.ts, line 132 at r2 (raw file):

      [half, innerDimensionSize - half], imagValues.shape.length-1);

    const outputShape = input.shape;

you need to call input.shape.slice() here otherwise you are mutating the shape of the input (that's why the unit test fails)

Copy link
Contributor Author

@Lewuathe Lewuathe left a comment

Choose a reason for hiding this comment

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

I used VSCode and installed clang-format extension. I thought it was applied automatically.
I will apply clang-format explicitly.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained

Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

I think the clang-format extension is installed improperly (this formatting isn't the Google style).

If you open vscode to the root of the repo, it should work out of the box. You need both clang-format and tslint. I'll merge this and make the formatting changes myself.

Thanks Kai! Do you think you could take on the imaginary part now?

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@nsthorat nsthorat merged commit fb86033 into tensorflow:master Nov 1, 2018
Copy link
Contributor Author

@Lewuathe Lewuathe left a comment

Choose a reason for hiding this comment

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

Do you think you could take on the imaginary part now?

Sorry, what do you mean by that? rfft for imaginary part?

Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@nsthorat
Copy link
Contributor

nsthorat commented Nov 2, 2018

Sorry I meant this: https://www.tensorflow.org/api_docs/python/tf/spectral/irfft

The real-valued inverse FFT.

@Lewuathe
Copy link
Contributor Author

Lewuathe commented Nov 2, 2018

I see. Sure I can.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants