completed lstm_cell #70

mei1127 · 2024-02-28T07:56:53Z

@huningxin @fdwr @shiyi9801 @BruceDai PTAL, thanks!

BruceDai · 2024-02-28T08:01:24Z

src/lstm_cell.js

+ * @param {MLLstmCellOptions} options
+ * return {Tensor}
+ */
+


Please remove this blank line.

BruceDai · 2024-02-28T08:04:03Z

src/lstm_cell.js

+  validateLstmCellParams(...arguments);
+  const zero = new Scalar(0);
+  const inputSize = input.shape[1];
+  const starts = layout === 'iofg' ? {i: 0, o: hiddenSize, f: 2* hiddenSize, g: 3*hiddenSize} :


Suggested change

const starts = layout === 'iofg' ? {i: 0, o: hiddenSize, f: 2* hiddenSize, g: 3*hiddenSize} :

const starts = layout === 'iofg' ? {i: 0, o: hiddenSize, f: 2 * hiddenSize, g: 3 * hiddenSize} :

{i: 0, f: hiddenSize, g: 2 * hiddenSize, o: 3 * hiddenSize};

BruceDai · 2024-02-28T08:06:32Z

test/lstm_cell_test.js

+import * as utils from './utils.js';
+
+describe('test lstmCell', function() {
+  it.only('lstmCell lstmCell activations=[relu, relu, relu]', function() {


Please use it() method.

BruceDai · 2024-02-28T08:08:29Z

test/lstm_cell_test.js

+    const hiddenSize = 2;
+    const input = new Tensor([batchSize, inputSize], [1, 2, 2, 1]);
+    const weight = new Tensor([4 * hiddenSize, inputSize],
+        new Float32Array([


Prefer keep 2 space indentations.

Prefer keep 2 space indentations.

👍 Yeah, although I much prefer 4-space indents over 2-space indents to more clearly/accurately/quickly see the overall hierarchy of blocks, even more important is consistency within a codebase, and since the rest of the files are 2-space, then I'd do so here too (a mix of both 4-space and 2-space makes it harder to follow the levels too :'-( ).

There are lots of possible ways to do it, but picking one and sticking with it is better than switching between 3 different kinds of indentation. e.g.

Linear flow (uses more vertical space, but it's highly scannable for the eye to quickly locate code, efficient to glean overall block structure, consistently symmetric in constructs, and easily diffable)

const hiddenState = new Tensor( // -> [batchSize, hiddenSize], // 0 new Float32Array(batchSize * hiddenSize).fill(0), // 1 ); // <- const cellState = new Tensor( // -> [batchSize, hiddenSize], // 0 new Float32Array(batchSize * hiddenSize).fill(0), // 1 ); // <- const activations = [ // -> relu, // 0 relu, // 1 relu, // 2 ]; // <- const outputs = lstmCell( input, weight, recurrentWeight, hiddenState, cellState, hiddenSize, {bias, recurrentBias, peepholeWeight, activations} );

Compact zig-zag ragged wrap with variable indent for nested lines (slower to scan and spot identifiers, and terrible for diffing, but shaves vertical space):

const hiddenState = new Tensor([batchSize, hiddenSize], new Float32Array(batchSize * hiddenSize).fill(0)); const cellState = new Tensor([batchSize, hiddenSize], new Float32Array(batchSize * hiddenSize).fill(0)); const activations = [relu, relu, relu]; const outputs = lstmCell(input, weight, recurrentWeight, hiddenState, cellState, hiddenSize, {bias, recurrentBias, peepholeWeight, activations});

Compact zig-zag ragged wrap with uniform indentation for nested lines (slower to scan, bad for diffing albeit it a little less than #‌1, and shaves less vertical space):

const hiddenState = new Tensor( [batchSize, hiddenSize], new Float32Array(batchSize * hiddenSize).fill(0)); const cellState = new Tensor( [batchSize, hiddenSize], new Float32Array(batchSize * hiddenSize).fill(0)); const activations = [ relu, relu, relu]; const outputs = lstmCell( input, weight, recurrentWeight, hiddenState, cellState, hiddenSize, {bias, recurrentBias, peepholeWeight, activations});

BruceDai · 2024-02-28T08:09:58Z

Thanks @mei1127.
LGTM with some nits, please do some modifications.

fdwr

Minor comments. Thanks Mei.

fdwr · 2024-03-02T08:29:31Z

src/lstm_cell.js

+  const starts = layout === 'iofg' ? {i: 0, o: hiddenSize, f: 2 * hiddenSize, g: 3 *hiddenSize} :
+  {i: 0, f: hiddenSize, g: 2 * hiddenSize, o: 3 * hiddenSize};


Would be easier for future readers to read if indentation was aligned consistently:

Suggested change

const starts = layout === 'iofg' ? {i: 0, o: hiddenSize, f: 2 * hiddenSize, g: 3 *hiddenSize} :

{i: 0, f: hiddenSize, g: 2 * hiddenSize, o: 3 * hiddenSize};

const starts = layout === 'iofg' ? {i: 0, o: hiddenSize, f: 2 * hiddenSize, g: 3 *hiddenSize} :

{i: 0, f: hiddenSize, g: 2 * hiddenSize, o: 3 * hiddenSize};

fdwr · 2024-03-02T08:29:58Z

test/lstm_cell_test.js

+    const hiddenSize = 2;
+    const input = new Tensor([batchSize, inputSize], [1, 2, 2, 1]);
+    const weight = new Tensor([4 * hiddenSize, inputSize],
+        new Float32Array([


Prefer keep 2 space indentations.

👍 Yeah, although I much prefer 4-space indents over 2-space indents to more clearly/accurately/quickly see the overall hierarchy of blocks, even more important is consistency within a codebase, and since the rest of the files are 2-space, then I'd do so here too (a mix of both 4-space and 2-space makes it harder to follow the levels too :'-( ).

There are lots of possible ways to do it, but picking one and sticking with it is better than switching between 3 different kinds of indentation. e.g.

Linear flow (uses more vertical space, but it's highly scannable for the eye to quickly locate code, efficient to glean overall block structure, consistently symmetric in constructs, and easily diffable)

const hiddenState = new Tensor( // -> [batchSize, hiddenSize], // 0 new Float32Array(batchSize * hiddenSize).fill(0), // 1 ); // <- const cellState = new Tensor( // -> [batchSize, hiddenSize], // 0 new Float32Array(batchSize * hiddenSize).fill(0), // 1 ); // <- const activations = [ // -> relu, // 0 relu, // 1 relu, // 2 ]; // <- const outputs = lstmCell( input, weight, recurrentWeight, hiddenState, cellState, hiddenSize, {bias, recurrentBias, peepholeWeight, activations} );

Compact zig-zag ragged wrap with variable indent for nested lines (slower to scan and spot identifiers, and terrible for diffing, but shaves vertical space):

const hiddenState = new Tensor([batchSize, hiddenSize], new Float32Array(batchSize * hiddenSize).fill(0)); const cellState = new Tensor([batchSize, hiddenSize], new Float32Array(batchSize * hiddenSize).fill(0)); const activations = [relu, relu, relu]; const outputs = lstmCell(input, weight, recurrentWeight, hiddenState, cellState, hiddenSize, {bias, recurrentBias, peepholeWeight, activations});

Compact zig-zag ragged wrap with uniform indentation for nested lines (slower to scan, bad for diffing albeit it a little less than #‌1, and shaves less vertical space):

const hiddenState = new Tensor( [batchSize, hiddenSize], new Float32Array(batchSize * hiddenSize).fill(0)); const cellState = new Tensor( [batchSize, hiddenSize], new Float32Array(batchSize * hiddenSize).fill(0)); const activations = [ relu, relu, relu]; const outputs = lstmCell( input, weight, recurrentWeight, hiddenState, cellState, hiddenSize, {bias, recurrentBias, peepholeWeight, activations});

fdwr · 2024-03-02T08:31:32Z

src/lstm_cell.js

+  // output hidden state (ht)
+  const ht = mul(o, activation2(ct));
+
+  return [ht, ct];


Full names are nice, like return [hiddenState, cellState]. Plus you already spell these names hiddenState and cellStates elsewhere below, which would be more consistent.

Maybe hiddenStateNew could be better for understand and differentiate from the previous state？

fdwr · 2024-03-02T09:07:36Z

src/lib/validate-input.js

+    throw new Error(`The hiddenState (rank ${hiddenState.rank}) is not a 2-D tensor.`);
+  }
+  if (hiddenState.shape[0] !== batchSize || hiddenState.shape[1] !== hiddenSize) {
+    throw new Error(`The shape of hiddenState


The error message is split.

complete lstm_cell

abfceb9

BruceDai reviewed Feb 28, 2024

View reviewed changes

revised lstm_cell.js and lstm_cell_test.js

9a7e0a4

fdwr reviewed Mar 2, 2024

View reviewed changes

huningxin merged commit 9a7e0a4 into webmachinelearning:main Apr 16, 2024

mei1127 mentioned this pull request Apr 16, 2024

Address two remaining comments in the previous LstmCell PR #74

Merged

BruceDai mentioned this pull request Aug 15, 2024

WPT tests tracker / 2 for remaining ops webmachinelearning/webnn#338

Closed

	const starts = layout === 'iofg' ? {i: 0, o: hiddenSize, f: 2* hiddenSize, g: 3*hiddenSize} :
	const starts = layout === 'iofg' ? {i: 0, o: hiddenSize, f: 2 * hiddenSize, g: 3 * hiddenSize} :
	{i: 0, f: hiddenSize, g: 2 * hiddenSize, o: 3 * hiddenSize};

completed lstm_cell #70

completed lstm_cell #70

Uh oh!

Conversation

mei1127 commented Feb 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fdwr Mar 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Linear flow (uses more vertical space, but it's highly scannable for the eye to quickly locate code, efficient to glean overall block structure, consistently symmetric in constructs, and easily diffable)

Compact zig-zag ragged wrap with variable indent for nested lines (slower to scan and spot identifiers, and terrible for diffing, but shaves vertical space):

Compact zig-zag ragged wrap with uniform indentation for nested lines (slower to scan, bad for diffing albeit it a little less than #‌1, and shaves less vertical space):

Uh oh!

BruceDai commented Feb 28, 2024

Uh oh!

fdwr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fdwr Mar 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Linear flow (uses more vertical space, but it's highly scannable for the eye to quickly locate code, efficient to glean overall block structure, consistently symmetric in constructs, and easily diffable)

Compact zig-zag ragged wrap with variable indent for nested lines (slower to scan and spot identifiers, and terrible for diffing, but shaves vertical space):

Compact zig-zag ragged wrap with uniform indentation for nested lines (slower to scan, bad for diffing albeit it a little less than #‌1, and shaves less vertical space):

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fdwr Mar 2, 2024 •

edited

Loading

fdwr Mar 2, 2024 •

edited

Loading