Add stft op #1746

Lewuathe · 2019-05-07T12:06:43Z

Add tf.signal.stft op.

One TODO is passing fft length parameter because rfft does not support fft length parameter. We can pass fft length parameter after rfft supports it.

See: tensorflow/tfjs#1362

This change is

nsthorat

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

src/ops/signal_ops.ts, line 116 at r1 (raw file):

 */
function stft_(
    signal: Tensor1D, frameLength: number, frameStep: number,

so normally this would be fine to wait for a param (fft_length) but unfortunately adding that would be a breaking API before the windowFn since it's not the last argument

Is this something you would be interested in adding to rfft?

Lewuathe · 2019-05-08T00:14:04Z

Okay, I will try to extend rfft ops.

Lewuathe · 2019-05-18T12:38:31Z

Updated rfft to get the fftLength parameter as TensorFlow core does.
See: #1762

Lewuathe · 2019-06-18T10:34:59Z

@nsthorat I updated to support the given fftLength. Could you take a look when you get a chance?

nsthorat · 2019-06-18T15:09:42Z

Check out the details for failing tests:

Lewuathe · 2019-06-18T23:55:08Z

Oh, sorry. I'll take a look.

Lewuathe · 2019-06-19T01:19:26Z

Hmm, it keeps showing a weird error. That is exactly the same code used in the unit test. But it failed due to tf.signal.stft is not a function.

Step #6 - "test-snippets": ```js
Step #6 - "test-snippets": const input = tf.tensor1d([1, 1, 1, 1, 1])
Step #6 - "test-snippets": tf.signal.stft(input, 3, 1);
Step #6 - "test-snippets": ```
Step #6 - "test-snippets": 
Step #6 - "test-snippets": TypeError: tf.signal.stft is not a function

nsthorat · 2019-06-19T13:27:17Z

src/ops/signal_ops.ts

@@ -107,3 +142,4 @@ function cosineWindow(windowLength: number, a: number, b: number): Tensor1D {
 export const hannWindow = op({hannWindow_});
 export const hammingWindow = op({hammingWindow_});
 export const frame = op({frame_});
+export const sftf = op({stft_});


the spelling is off here :)

Oops. Thanks for catching.

nsthorat · 2019-06-19T13:27:43Z

Responded inline where the problem is.

nsthorat

Reviewed 1 of 2 files at r4.
Reviewable status: 0 of 1 approvals obtained (waiting on @Lewuathe)

src/ops/signal_ops.ts, line 105 at r4 (raw file):

 * ```js
 * const input = tf.tensor1d([1, 1, 1, 1, 1])
 * tf.signal.stft(input, 3, 1);

.print()

src/ops/signal_ops.ts, line 117 at r4 (raw file):

 */
function stft_(
    signal: Tensor1D, frameLength: number, frameStep: number,

in tensorflow this supports more than tensor1d, likely we'll have to do that here too can you support that?

src/ops/signal_ops_test.ts, line 130 at r4 (raw file):

describeWithFlags('stft', ALL_ENVS, () => {
  it('3 length with hann window', async () => {
    const input = tf.tensor1d([1, 1, 1, 1, 1]);

lets have different values here

src/ops/signal_ops_test.ts, line 131 at r4 (raw file):

  it('3 length with hann window', async () => {
    const input = tf.tensor1d([1, 1, 1, 1, 1]);
    const outputs = tf.signal.stft(input, 3, 1);

pull the 3, 1 into named constants (also for the rest of these)

src/ops/signal_ops_test.ts, line 133 at r4 (raw file):

    const outputs = tf.signal.stft(input, 3, 1);
    expect(outputs.length).toEqual(3);
    for (const output of outputs) {

in general you should prefer to just do the following

expectArraysClose(outputs[0].data(), ..);
expectArraysClose(outputs[1].data(), ..);

src/ops/signal_ops_test.ts, line 163 at r4 (raw file):

      expectArraysClose(
          await output.data(),
          [1.99999, 0.0, -1.059017, -0.76942056, 0.05901694, 0.18163569]);

are there situations where all of these aren't the same? can we have a test for that?

Lewuathe

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

src/ops/signal_ops.ts, line 117 at r4 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

in tensorflow this supports more than tensor1d, likely we'll have to do that here too can you support that?

I think we need to first work on supporting complex type by concat ops otherwise we do not return the tensor with the same shape as input. That's why current implementation returns the value as the array of tensor.

So I'm going to support complex type by concat operator first.

nsthorat · 2019-08-06T18:56:52Z

Stamped the complex64 change. Let's get this in ASAP if possible since we're going to move all the code in this repository to tensorflow/tfjs.

manrajgrover · 2019-08-07T09:03:28Z

@nsthorat But why? 😮

nsthorat · 2019-08-07T15:25:02Z

@manrajgrover having a monorepo will simplify a lot of the tooling we have, allowing us to share compiler settings, tslint settings, vscode settings, etc.

nsthorat

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

src/ops/signal_ops_test.ts, line 130 at r4 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

lets have different values here

can you make these different values? here and below

src/ops/signal_ops_test.ts, line 134 at r6 (raw file):

    const frameStep = 1;
    const output = tf.signal.stft(input, frameLength, frameStep);
    expect(output.shape[0]).toEqual(3);

expect the whole output shape to be something not just the first value (here and elsewhere)

src/ops/signal_ops_test.ts, line 143 at r6 (raw file):

  it('3 length with hann window (sequencial number)', async () => {
    const input = tf.tensor1d([1, 2, 3, 4, 5]);

if I run this in TensorFlow I get a different result:

a = tf.constant([1, 2, 3, 4, 5], shape=[5], dtype=tf.float32)
b = tf.signal.stft(a, 3, 1)

tf.Tensor(
[[ 2.+0.j  0.-2.j -2.+0.j]
 [ 3.+0.j  0.-3.j -3.+0.j]
 [ 4.+0.j  0.-4.j -4.+0.j]], shape=(3, 3), dtype=complex64)

Any idea why this is different?

src/ops/signal_ops_test.ts, line 251 at r6 (raw file):

    const fftLength = 5;
    const output = tf.signal.stft(input, frameLength, frameStep,
      fftLength, tf.signal.hammingWindow);

use (length) => tf.signal.hammingWindow(length)

here and elsewhere

Lewuathe

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

src/ops/signal_ops_test.ts, line 130 at r4 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

can you make these different values? here and below

Is the following case not sufficient?

'3 length with hann window (sequencial number)

Lewuathe

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

src/ops/signal_ops_test.ts, line 143 at r6 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

if I run this in TensorFlow I get a different result:

a = tf.constant([1, 2, 3, 4, 5], shape=[5], dtype=tf.float32)
b = tf.signal.stft(a, 3, 1)

tf.Tensor(
[[ 2.+0.j  0.-2.j -2.+0.j]
 [ 3.+0.j  0.-3.j -3.+0.j]
 [ 4.+0.j  0.-4.j -4.+0.j]], shape=(3, 3), dtype=complex64)

Any idea why this is different?

Hmm, let me check.

src/ops/signal_ops_test.ts, line 251 at r6 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

use (length) => tf.signal.hammingWindow(length)

here and elsewhere

ok

Lewuathe

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

src/ops/signal_ops_test.ts, line 143 at r6 (raw file):

Previously, Lewuathe (Kai Sasaki) wrote…

Hmm, let me check.

Looks like TensorFlow does not set the fftLength as fftFrame directly.

https://github.com/tensorflow/tensorflow/blob/7bb7116c77bf4498feeda85c2e608b8ccb281b43/tensorflow/python/ops/signal/spectral_ops.py#L78

Need to convert the same calculate as _enclosing_power_of_two does.

nsthorat

Last comment about Tensor1D then LGTM and I will merge this :)

Reviewed 1 of 2 files at r7.
Reviewable status: 0 of 1 approvals obtained (waiting on @Lewuathe)

src/ops/signal_ops.ts, line 117 at r4 (raw file):

Previously, Lewuathe (Kai Sasaki) wrote…

I think we need to first work on supporting complex type by concat ops otherwise we do not return the tensor with the same shape as input. That's why current implementation returns the value as the array of tensor.

So I'm going to support complex type by concat operator first.

Now that it's fixed, can we update this typing and add tests for non-tensor1d inputs?

src/ops/signal_ops_test.ts, line 143 at r6 (raw file):

Previously, Lewuathe (Kai Sasaki) wrote…

Looks like TensorFlow does not set the fftLength as fftFrame directly.

https://github.com/tensorflow/tensorflow/blob/7bb7116c77bf4498feeda85c2e608b8ccb281b43/tensorflow/python/ops/signal/spectral_ops.py#L78

Need to convert the same calculate as _enclosing_power_of_two does.

Awesome, this matches tensorflow now.

nsthorat · 2019-08-12T16:59:06Z

Also the test failure is a known flakiness issue that I fixed in #1877 so just update to master.

nsthorat · 2019-08-12T18:25:56Z

Actually, for expediency, I'll merge this now and can you send a follow up to the monorepo once tfjs-core has moved?

Lewuathe · 2019-08-13T00:14:29Z

@nsthorat Sure, I'll make a follow-up PR. Thanks!

Add stft op

b34c00d

nsthorat reviewed May 7, 2019

View reviewed changes

Lewuathe mentioned this pull request May 18, 2019

Specify fftLength in rfft op #1762

Merged

Lewuathe added 2 commits June 18, 2019 16:57

Merge branch 'master' into support-stft

c58756c

Support fftLength

45a9199

Lewuathe added 3 commits June 19, 2019 08:55

Merge branch 'master' into support-stft

9589b46

Fix lint

3d9895f

Fix the code snippet

5811662

nsthorat reviewed Jun 19, 2019

View reviewed changes

Lewuathe added 2 commits June 20, 2019 22:32

Fix typo

f532564

Merge branch 'master' into support-stft

a7a5d2f

nsthorat reviewed Jun 20, 2019

View reviewed changes

Lewuathe commented Jul 3, 2019

View reviewed changes

Lewuathe added 2 commits July 3, 2019 21:43

Post review followup

3187446

Merge branch 'master' into support-stft

421a7e7

Lewuathe mentioned this pull request Jul 4, 2019

Support complex type in concat op #1829

Merged

Fix test failure

93ca416

Lewuathe force-pushed the support-stft branch from 457a65f to 93ca416 Compare July 4, 2019 12:12

Lewuathe added 3 commits August 7, 2019 09:47

Merge branch 'master' into support-stft

541a060

Concat the output tensors

b01358d

Fix snippet error

403f5a3

nsthorat reviewed Aug 7, 2019

View reviewed changes

Merge branch 'master' into support-stft

2145962

Lewuathe commented Aug 8, 2019

View reviewed changes

Lewuathe added 2 commits August 8, 2019 12:14

wip

430d7db

Fix compatibility with TensorFlow

d90436f

googlebot added the cla: yes label Aug 8, 2019

Merge branch 'master' into support-stft

aeabeb9

nsthorat reviewed Aug 12, 2019

View reviewed changes

Merge branch 'master' into support-stft

417168d

nsthorat merged commit f135c5a into tensorflow:master Aug 12, 2019

miaobin mentioned this pull request Aug 27, 2019

[example] Implement Speech Recognition example. intel/webml-polyfill#924

Closed

Add stft op #1746

Add stft op #1746

Uh oh!

Conversation

Lewuathe commented May 7, 2019 • edited by dsmilkov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nsthorat left a comment

Choose a reason for hiding this comment

Uh oh!

Lewuathe commented May 8, 2019

Uh oh!

Lewuathe commented May 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lewuathe commented Jun 18, 2019

Uh oh!

nsthorat commented Jun 18, 2019

Uh oh!

Lewuathe commented Jun 18, 2019

Uh oh!

Lewuathe commented Jun 19, 2019

Uh oh!

nsthorat Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

Lewuathe Jun 20, 2019

Choose a reason for hiding this comment

Uh oh!

nsthorat commented Jun 19, 2019

Uh oh!

nsthorat left a comment

Choose a reason for hiding this comment

Uh oh!

Lewuathe left a comment

Choose a reason for hiding this comment

Uh oh!

nsthorat commented Aug 6, 2019

Uh oh!

manrajgrover commented Aug 7, 2019

Uh oh!

nsthorat commented Aug 7, 2019

Uh oh!

nsthorat left a comment

Choose a reason for hiding this comment

Uh oh!

Lewuathe left a comment

Choose a reason for hiding this comment

Uh oh!

Lewuathe left a comment

Choose a reason for hiding this comment

Uh oh!

Lewuathe left a comment

Choose a reason for hiding this comment

Uh oh!

nsthorat left a comment

Choose a reason for hiding this comment

Uh oh!

nsthorat commented Aug 12, 2019

Uh oh!

nsthorat commented Aug 12, 2019

Uh oh!

Lewuathe commented Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Lewuathe commented May 7, 2019 •

edited by dsmilkov

Loading

Lewuathe commented May 18, 2019 •

edited

Loading

Lewuathe commented Aug 13, 2019 •

edited

Loading