Add LSTM ops and a simple LSTM demo #47

cghawthorne · 2017-08-15T00:05:50Z

Adds ports of tf.contrib.rnn.BasicLSTMCell and tf.contrib.rnn.MultiRNNCell.

This change is

dsmilkov · 2017-08-15T20:12:58Z

So cool to see this PR! We use Reviewable for this project which is similar to Gerrit and the internal Google tool. Let me know if you have any questions about Reviewable! Most comments are just style/API goodies. The only bigger question is about the API regarding the input data type of the math lstm methods.

Reviewed 13 of 13 files at r1.
Review status: all files reviewed at latest revision, 16 unresolved discussions.

demos/lstm/index.html, line 4 at r1 (raw file):

<head>
  <title>LSTM Demo</title>
  <script src="bundle.js"></script>

move this <script> tag at the end of the body after <div id="success">

We follow this practice as a precaution to avoid situations where the code uses elements in the dom that are not yet rendered.

demos/lstm/lstm.ts, line 67 at r1 (raw file):

      const weightedResult1D = math.reshape(
          weightedResult, [fullyConnectedBiases.shape[0]]) as Array1D;
      const logits = math.add(

we added support for broadcasting in #40. math.add(weightedResult, fullyConnectedBiases) should work

demos/lstm/lstm.ts, line 85 at r1 (raw file):

});

function checkArrays(expected: number[], results: number[]) {

import {util} from '../deeplearnjs' and use util.arraysEqual(a, b)

demos/lstm/README.md, line 1 at r1 (raw file):

LSTM Demo

How about a title like "Learning digits of pi using an LSTM"

demos/lstm/README.md, line 6 at r1 (raw file):

BasicLSTMCells

s/BasicLSTMCells/BasicLSTMCells . Same for MultiRNNCell (wrap in ``)

demos/lstm/README.md, line 7 at r1 (raw file):

the

s/the/to

demos/lstm/README.md, line 15 at r1 (raw file):

.

s/.:/:

src/math/math.ts, line 1148 at r1 (raw file):

   * @param c Array of previous cell states.
   * @param h Array of previous cell outputs.
   */

add @return Tuple [nextCellStates, cellOutputs] Our doc generator thanks you!

src/math/math.ts, line 1150 at r1 (raw file):

   */
  multiRNNCell(lstmCells: LSTMCell[], data: Array1D, c: Array2D[],
      h: Array2D[]): Array2D[][] {

Since it's always a tuple of 2 you can type the returning result as:
: [Array2D[], Array2D[]] {

src/math/math.ts, line 1151 at r1 (raw file):

keep, track

FYI, you can just say this.scope(() => {..} if you don't use keep and track

src/math/math.ts, line 1158 at r1 (raw file):

this.reshape(output[1], [output[1].shape[1]]) as Array1D;

use input = output[1].as1D() here and elsewhere. Math.reshape() is going away - see other comment about reshape.

src/math/math.ts, line 1180 at r1 (raw file):

   * @param data The input to the cell.
   * @param c Previous cell state.
   * @param h Previous cell output.

add @return Tuple [nextState, output] to be used in the next iteration.

src/math/math.ts, line 1183 at r1 (raw file):

Array2D[]

if the result is always of length 2, you can type it more strictly:
: [Array2D, Array2D] {

Also, is the cell/hidden state Array2D because of batching? In that case, maybe change input data to Array2D as well, here and in MultiRNNCell. And throw an Error inside if batch size is > 1 for now, saying batch sizes > 1 are not yet supported.

src/math/math.ts, line 1189 at r1 (raw file):

this.reshape

use data.as3D(1, 1, data.shape[0]) here and elsewhere (as2D, as1D etc) where you call this.reshape(). You don't need Math.reshape, which does physical texture reshaping and is going away since we are moving our shaders to logical sampling. asXD() methods use NDArray.reshape(), which does logical reshape and is practically free.

src/math/math.ts, line 1201 at r1 (raw file):

  const weighted1D = this.reshape(weighted, [lstmBias.shape[0]]) as Array1D;
  const res1D = this.add(weighted1D, lstmBias);

We added broadcasting in #40 so this.add(weighted, lstmBias) should work now!

src/math/math.ts, line 1215 at r1 (raw file):

this.elementWiseMul

sorry API's is evolving while you are writing this demo :) we deprecated this method. Use math.multiply(a, b) (which supports broadcasting), or math.multiplyStrict(a, b)which makes sure a and b are of the same shape and doesn't do broadcasting.

Comments from Reviewable

nsthorat · 2017-08-16T00:29:43Z

I have only minor nits, Daniel covered all the non-trivial stuff! This is /so/ awesome, thank you for contributing to src/ and being the first major external contributor! :)

Reviewed 13 of 13 files at r1.
Review status: all files reviewed at latest revision, 24 unresolved discussions, some commit checks failed.

demos/lstm/index.html, line 1 at r1 (raw file):

<html>

Add the HTML license docstring (check out demos/mnist.html)

demos/lstm/lstm.ts, line 1 at r1 (raw file):

/* Copyright 2017 Google Inc. All Rights Reserved.

can we maybe call this lstm_inference.ts so that it's clear this doesn't do any training?

demos/lstm/lstm.ts, line 39 at r1 (raw file):

  const fullyConnectedWeights = vars['fully_connected/weights'] as Array2D;

  const results:number[] = [];

space before number[]

demos/lstm/lstm.ts, line 54 at r1 (raw file):

    let input = primerData;
    for (let i = 0; i <  expected.length; i++) {

remove extra space before expected

src/math/math.ts, line 1143 at r1 (raw file):

   * Computes the next states and outputs of a stack of LSTMCells.
   * Each cell output is used as input to the next cell.
   * Derived from tf.contrib.rn.MultiRNNCell.

can you say in the docstring here that this is only the forward mode?

src/math/math.ts, line 1163 at r1 (raw file):

      return newStates;
    });
    const newC:Array2D[] = [];

Add space before "Array2D[]" and below

src/math/math.ts, line 1173 at r1 (raw file):

  /**
   * Computes the next state and output of a BasicLSTMCell.

add to docstring saying this is only forward mode

src/math/math.ts, line 1198 at r1 (raw file):

      // tf.nn.bias_add(weighted, lstmBias)
      // There is no broadcast add, but we can assume a batch size of 1,

just a reminder to update this comment when you do the broadcast add :)

Comments from Reviewable

cghawthorne · 2017-08-16T18:37:08Z

Review status: 8 of 13 files reviewed at latest revision, 24 unresolved discussions.

demos/lstm/index.html, line 1 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

Add the HTML license docstring (check out demos/mnist.html)

Done.

demos/lstm/index.html, line 4 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

move this <script> tag at the end of the body after <div id="success">

We follow this practice as a precaution to avoid situations where the code uses elements in the dom that are not yet rendered.

Done.

demos/lstm/README.md, line 1 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

LSTM Demo

How about a title like "Learning digits of pi using an LSTM"

Done.

demos/lstm/README.md, line 6 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

BasicLSTMCells

s/BasicLSTMCells/BasicLSTMCells . Same for MultiRNNCell (wrap in ``)

Done.

demos/lstm/README.md, line 7 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

the

s/the/to

Done.

demos/lstm/README.md, line 15 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

.

s/.:/:

Done.

src/math/math.ts, line 1143 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

can you say in the docstring here that this is only the forward mode?

Done.

src/math/math.ts, line 1148 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

add @return Tuple [nextCellStates, cellOutputs] Our doc generator thanks you!

Done.

src/math/math.ts, line 1150 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

Since it's always a tuple of 2 you can type the returning result as:
: [Array2D[], Array2D[]] {

Done.

src/math/math.ts, line 1151 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

keep, track

FYI, you can just say this.scope(() => {..} if you don't use keep and track

Done.

src/math/math.ts, line 1158 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

this.reshape(output[1], [output[1].shape[1]]) as Array1D;

use input = output[1].as1D() here and elsewhere. Math.reshape() is going away - see other comment about reshape.

Done.

src/math/math.ts, line 1163 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

Add space before "Array2D[]" and below

Done.

src/math/math.ts, line 1173 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

add to docstring saying this is only forward mode

Done.

src/math/math.ts, line 1180 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

add @return Tuple [nextState, output] to be used in the next iteration.

Done.

src/math/math.ts, line 1183 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

Array2D[]

if the result is always of length 2, you can type it more strictly:
: [Array2D, Array2D] {

Also, is the cell/hidden state Array2D because of batching? In that case, maybe change input data to Array2D as well, here and in MultiRNNCell. And throw an Error inside if batch size is > 1 for now, saying batch sizes > 1 are not yet supported.

Done.

src/math/math.ts, line 1189 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

this.reshape

use data.as3D(1, 1, data.shape[0]) here and elsewhere (as2D, as1D etc) where you call this.reshape(). You don't need Math.reshape, which does physical texture reshaping and is going away since we are moving our shaders to logical sampling. asXD() methods use NDArray.reshape(), which does logical reshape and is practically free.

Done.

src/math/math.ts, line 1198 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

just a reminder to update this comment when you do the broadcast add :)

Done.

src/math/math.ts, line 1201 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

  const weighted1D = this.reshape(weighted, [lstmBias.shape[0]]) as Array1D;
  const res1D = this.add(weighted1D, lstmBias);
We added broadcasting in #40 so this.add(weighted, lstmBias) should work now!

Done.

src/math/math.ts, line 1215 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

this.elementWiseMul

sorry API's is evolving while you are writing this demo :) we deprecated this method. Use math.multiply(a, b) (which supports broadcasting), or math.multiplyStrict(a, b)which makes sure a and b are of the same shape and doesn't do broadcasting.

Done.

demos/lstm/lstm.ts, line 1 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

can we maybe call this lstm_inference.ts so that it's clear this doesn't do any training?

Done.

demos/lstm/lstm.ts, line 39 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

space before number[]

Done.

demos/lstm/lstm.ts, line 54 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

remove extra space before expected

Done.

demos/lstm/lstm.ts, line 67 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

we added support for broadcasting in #40. math.add(weightedResult, fullyConnectedBiases) should work

Done.

demos/lstm/lstm.ts, line 85 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

import {util} from '../deeplearnjs' and use util.arraysEqual(a, b)

Done.

Comments from Reviewable

cghawthorne · 2017-08-16T18:38:31Z

Thanks for the review! I think I've got everything fixed up based on your comments.

Review status: 8 of 13 files reviewed at latest revision, 24 unresolved discussions.

Comments from Reviewable

dsmilkov · 2017-08-16T18:52:31Z

Thank you for the amazing contribution! We've learned a lot along the process regarding our API, such as the need for broadcasting support, adding concat1D/2D(), removing/deprecating math.reshape() etc.

If you have any other feedback of how the API can be simplified, or what it needs, would love to hear it!

Review status: 8 of 13 files reviewed at latest revision, 8 unresolved discussions.

Comments from Reviewable

* BasicLSTMCell * cleanup * 2-layer lstm * add training script for pi digits * start * interface * docs * lint * lint * fixes * updates based on comments

dsmilkov requested review from dsmilkov and nsthorat August 15, 2017 00:30

cghawthorne added 11 commits August 16, 2017 10:50

BasicLSTMCell

f18532e

cleanup

493bc5b

2-layer lstm

cf11b9f

add training script for pi digits

0c9d50c

start

1bf4841

interface

1640f64

docs

20e5597

lint

f3657bc

lint

fe7296c

fixes

c881ee6

updates based on comments

0672931

cghawthorne force-pushed the lstm branch from 265635e to 0672931 Compare August 16, 2017 18:35

dsmilkov merged commit 4f0de84 into tensorflow:master Aug 16, 2017

cghawthorne deleted the lstm branch August 16, 2017 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add LSTM ops and a simple LSTM demo #47

Add LSTM ops and a simple LSTM demo #47

Uh oh!

cghawthorne commented Aug 15, 2017 •

edited by dsmilkov

Loading

Uh oh!

dsmilkov commented Aug 15, 2017

Uh oh!

nsthorat commented Aug 16, 2017

Uh oh!

cghawthorne commented Aug 16, 2017

Uh oh!

cghawthorne commented Aug 16, 2017

Uh oh!

dsmilkov commented Aug 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add LSTM ops and a simple LSTM demo #47

Add LSTM ops and a simple LSTM demo #47

Uh oh!

Conversation

cghawthorne commented Aug 15, 2017 • edited by dsmilkov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsmilkov commented Aug 15, 2017

Uh oh!

nsthorat commented Aug 16, 2017

Uh oh!

cghawthorne commented Aug 16, 2017

Uh oh!

cghawthorne commented Aug 16, 2017

Uh oh!

dsmilkov commented Aug 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cghawthorne commented Aug 15, 2017 •

edited by dsmilkov

Loading