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

Add hannWindow and hammingWindow op #1671

Merged
merged 10 commits into from
Apr 23, 2019

Conversation

Lewuathe
Copy link
Contributor

@Lewuathe Lewuathe commented Apr 10, 2019

Add Hann window op and hamming window op since this window function is necessary to implement stft and isftf as well as hamming window.

See: tensorflow/tfjs#1362


This change is Reviewable

@Lewuathe Lewuathe changed the title Add hannWindow op Add hannWindow and hamming op Apr 14, 2019
@Lewuathe Lewuathe changed the title Add hannWindow and hamming op Add hannWindow and hammingWindow op Apr 14, 2019
@Lewuathe
Copy link
Contributor Author

@nsthorat @annxingyuan That's necessary for implementing stft, isftf (tensorflow/tfjs#1362). Could you take a look when you get a chance?

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 1 of 7 files at r1, 6 of 10 files at r2, 1 of 1 files at r3.
Reviewable status: 0 of 1 approvals obtained (waiting on @Lewuathe)


src/backends/cpu/backend_cpu.ts, line 3409 at r3 (raw file):

    const newValues = new Float32Array(windowLength);
    for (let i = 0; i < windowLength; ++i) {
      const cosArg = (2.0 * Math.PI * i) / (windowLength+even-1);

can you make sure you have clang-format installed and working properly here? there should be spaces in this statement :)


src/backends/cpu/backend_cpu.ts, line 3410 at r3 (raw file):

    for (let i = 0; i < windowLength; ++i) {
      const cosArg = (2.0 * Math.PI * i) / (windowLength+even-1);
      newValues[i] = 0.54 - 0.46 * Math.cos(cosArg);

where do these constants come from? can we make them actual constants?


src/ops/signal_ops.ts, line 3 at r3 (raw file):

/**
 * @license
 * Copyright 2019 Google Inc. All Rights Reserved.

LLC


src/ops/signal_ops.ts, line 36 at r3 (raw file):

 */
function hannWindow_(windowLength: number): Tensor {
  return ENGINE.runKernel(backend => backend.hannWindow(windowLength), {});

In TensorFlow it looks like these don't exist as kernels, they just happen in python: https://github.com/tensorflow/tensorflow/blob/r1.13/tensorflow/python/ops/signal/window_ops.py

I think we should just do that here, generate all the values on the CPU.In practice I bet the windows aren't huge so I would bet that doing this on the CPU would actually be faster than spinning up a whole GPU program. You can remove all the backend / kernel logic and just put that as vanilla JS right in this file :)

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 and @nsthorat)


src/backends/cpu/backend_cpu.ts, line 3410 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

where do these constants come from? can we make them actual constants?

I checked Wikipedia as a reference as TensorFlow core does.
https://en.wikipedia.org/wiki/Window_function#Hann_and_Hamming_windows

0.54 is actually used in TensorFlow core too.
https://github.com/tensorflow/tensorflow/blob/65f12d778825306c31fcff5aa0d770bac1d4751e/tensorflow/python/ops/signal/window_ops.py#L80-L81


src/ops/signal_ops.ts, line 3 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

LLC

Sorry, I don't understand what it means. I used the exact same license header as other files. What change do I need?


src/ops/signal_ops.ts, line 36 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

In TensorFlow it looks like these don't exist as kernels, they just happen in python: https://github.com/tensorflow/tensorflow/blob/r1.13/tensorflow/python/ops/signal/window_ops.py

I think we should just do that here, generate all the values on the CPU.In practice I bet the windows aren't huge so I would bet that doing this on the CPU would actually be faster than spinning up a whole GPU program. You can remove all the backend / kernel logic and just put that as vanilla JS right in this file :)

Okay, will do.

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.

Will merge after these, thanks for the PR!

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


src/ops/signal_ops.ts, line 3 at r3 (raw file):

Previously, Lewuathe (Kai Sasaki) wrote…

Sorry, I don't understand what it means. I used the exact same license header as other files. What change do I need?

oh sorry it's 2019 Google LLC now, the 2019 files should have that.


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

 * Generate a hamming window.
 *
 * See: https://en.wikipedia.org/wiki/Window_function#Hann_window

this is the link to hann :)


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

 * @doc {heading: 'Operations', subheading: 'Signal', namespace: 'signal'}
 */
function hammingWindow_(windowLength: number): Tensor {

return can be stricter, Tensor1D here and above

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 1 of 1 files at r5.
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.

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


src/ops/signal_ops.ts, line 62 at r5 (raw file):

    newValues[i] = a - b * Math.cos(cosArg);
  }
  return Tensor.make([windowLength], {values: newValues}) as Tensor1D;

okay I promise this is the last comment, you should use the public API here, just call the tensor1d(values, [windowLength]) function

@Lewuathe
Copy link
Contributor Author

Thanks. Updated.

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 1 of 1 files at r6.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @Lewuathe)

@nsthorat nsthorat merged commit f82628f into tensorflow:master Apr 23, 2019
@nsthorat
Copy link
Contributor

Thanks for the PR!

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