-
Notifications
You must be signed in to change notification settings - Fork 943
Conversation
@nsthorat @annxingyuan That's necessary for implementing stft, isftf (tensorflow/tfjs#1362). Could you take a look when you get a chance? |
There was a problem hiding this 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 :)
There was a problem hiding this 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.
277d91c
to
bea237f
Compare
There was a problem hiding this 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
There was a problem hiding this 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:complete! 1 of 1 approvals obtained
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
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
Thanks. Updated. |
There was a problem hiding this 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:complete! 1 of 1 approvals obtained (waiting on @Lewuathe)
Thanks for the PR! |
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