[date-conversion-attention] Initial check-in of date-conversion-attention

[caisq]

• This PR checks in only the training scripts. Model inference in the browser will
  be checked in in a later PR.
• Unit tests are written for the data, model training and inference routines, although
  they are not hooked up with Travis right now. They are just run manually instead.

This change is 

[caisq]

Gentle ping @davidsoergel

[davidsoergel]

Reviewed 8 of 14 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @caisq and @davidsoergel)

---

package.json, line 1 at r1 (raw file):

{

I'm unclear why we need a top-level package.json or how it works. I thought all of the individual examples are independent, with their own package.jsons. Does this let you test all examples at once (and are the nested package.jsons honored too, to get the right dependencies for each one)?

---

test_util.js, line 24 at r1 (raw file):

function runTests(jasmine_util, spec_files) {
  // tslint:disable-next-line:no-require-imports
  const jasmineCtor = require('jasmine');

What does Ctor mean?

---

date-conversion-attention/date_format.js, line 17 at r1 (raw file):

 * =============================================================================
 */

Maybe add a comment here describing what this file is about. It's just for generating training data, right?

---

date-conversion-attention/date_format.js, line 35 at r1 (raw file):

// Use "\n" for padding for both input and output. It has to be at the
// beginning so that `mask_zero=True` can be used in the keras model.
export const INPUT_VOCAB = '\n0123456789/-, ' +

Add to the comment that the elements of the vocabulary are single characters, not words or anything else.

---

date-conversion-attention/date_format.js, line 70 at r1 (raw file):

}

export function dateTupleToMMSlashDDSlashYYYY(dateTuple) {

Maybe add a comment for each of these making the format easier to read, i.e. MM/DD/YYYY in this case

---

date-conversion-attention/date_format.js, line 127 at r1 (raw file):

  return `${dateTuple[0]}-${monthStr}-${dayStr}`;
}

DD.MM.YYYY and YYYYMMDD are also common, I think.

---

date-conversion-attention/date_format.js, line 146 at r1 (raw file):

}

export function encodeOutputDateStrings(dateStrings, oneHot = false) {

Since this is an example, I think a docstring would be particularly useful for this one (especially lacking TypeScript types).

---

date-conversion-attention/date_format.js, line 146 at r1 (raw file):

}

export function encodeOutputDateStrings(dateStrings, oneHot = false) {

I think it would be clearer to convert first to the integer form, and then convert that to one-hot as a separate step. Are you worried about the ~doubled memory requirement for that?

---

date-conversion-attention/date_format_test.js, line 133 at r1 (raw file):

    const values = encoded.dataSync();
    let strPrime = '';

strPrime sounds like it should mean more than it does. How about actual?

---

date-conversion-attention/model.js, line 53 at r1 (raw file):

attentin

typo

---

date-conversion-attention/model.js, line 57 at r1 (raw file):

Ouptut

typo

[caisq]

Thanks for the review, @davidsoergel !

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @davidsoergel)

---

package.json, line 1 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

I'm unclear why we need a top-level package.json or how it works. I thought all of the individual examples are independent, with their own package.jsons. Does this let you test all examples at once (and are the nested package.jsons honored too, to get the right dependencies for each one)?

The top-level package.json is needed to setup the Jasmine testing environment for all the example folders. It is for testing only. This follows the pattern in tfjs-models. The individual examples are still largely independent, with the exception of

1. this testing related change
2. the ../shared/ folder that Yannick introduced recently to host some common css files.

---

test_util.js, line 24 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

What does Ctor mean?

Constructor. This file is forked from tfjs-models. I renamed the variables in this file to make them more explicit and consistent.

---

date-conversion-attention/date_format.js, line 17 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

Maybe add a comment here describing what this file is about. It's just for generating training data, right?

This file is concerned with the data, their formats and conversions between them. It is use for training of the model in this PR, but a future PR will add code for inference and that will use this file too.

I added a file-level doc string to clarify this. Thanks for the suggestion.

---

date-conversion-attention/date_format.js, line 35 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

Add to the comment that the elements of the vocabulary are single characters, not words or anything else.

Done.

---

date-conversion-attention/date_format.js, line 70 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

Maybe add a comment for each of these making the format easier to read, i.e. MM/DD/YYYY in this case

Done.

---

date-conversion-attention/date_format.js, line 127 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

DD.MM.YYYY and YYYYMMDD are also common, I think.

I added these three new formats: DD.MM.YYYY, YYYY.MM.DD and YYYYMMDD. Thanks for the suggestion.

---

date-conversion-attention/date_format.js, line 146 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

Since this is an example, I think a docstring would be particularly useful for this one (especially lacking TypeScript types).

Done adding two doc strings.

---

date-conversion-attention/date_format.js, line 146 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

I think it would be clearer to convert first to the integer form, and then convert that to one-hot as a separate step. Are you worried about the ~doubled memory requirement for that?

Good question. The choice of using tf.buffer here has two reasons

1. It is straightforward and easy to explain
2. The inference step involves progressively setting more time steps in a tensor buffer (see the function runSeq2SeqInference in models.js). So using the same approach here is consistent with that and leaves us with one fewer thing to explain.

---

date-conversion-attention/date_format_test.js, line 133 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

strPrime sounds like it should mean more than it does. How about actual?

Renamed it to decodedStr.

---

date-conversion-attention/model.js, line 53 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

attentin

typo

Done

---

date-conversion-attention/model.js, line 57 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

Ouptut

typo

Done.

[davidsoergel]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @caisq)

---

date-conversion-attention/date_format.js, line 146 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…

Good question. The choice of using tf.buffer here has two reasons

1. It is straightforward and easy to explain
2. The inference step involves progressively setting more time steps in a tensor buffer (see the function runSeq2SeqInference in models.js). So using the same approach here is consistent with that and leaves us with one fewer thing to explain.

I think the use of tf.buffer is orthogonal to my concern, which is that a) you have to test the value of oneHot in multiple places, so the two code paths are entangled, and b) the return value is effectively of a union type, i.e. you can only interpret the resulting tensor correctly in the context of extra information.

So the proposal is: limit this function to the oneHot==false case (which can still use tf.buffer) and then add a convertToOneHot() function, which can also use the tf.buffer approach to make it very clear what's going on. That function's docstring should point out that tf.oneHot() does the same thing.

---

date-conversion-attention/date_format.js, line 24 at r2 (raw file):

represent

nit: represents

---

date-conversion-attention/date_format.js, line 26 at r2 (raw file):

contains function

nit: contains a function

---

date-conversion-attention/date_format.js, line 181 at r2 (raw file):

paddin

nit: padding

---

date-conversion-attention/date_format.js, line 183 at r2 (raw file):

 * input date strings. The paddin value is zero.
 *
 * @param {string[]} dateStrings Input date strings. Must be one

Clarify that mixed formats are accepted, as opposed to one array with a bunch of dates in the same format. E.g., "Each element of the array must be a date string in one of the formats listed above."

---

date-conversion-attention/date_format.js, line 186 at r2 (raw file):

flota32

typo

---

date-conversion-attention/date_format.js, line 187 at r2 (raw file):

iput

typo

---

date-conversion-attention/date_format.js, line 215 at r2 (raw file):

flota32

typo

---

date-conversion-attention/date_format_test.js, line 133 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…

Renamed it to decodedStr.

OK that works-- propagate to the rest of the cases too? I.e. search for 'Prime' in this file and others.

[caisq]

Thanks again!

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

---

date-conversion-attention/date_format.js, line 146 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

I think the use of tf.buffer is orthogonal to my concern, which is that a) you have to test the value of oneHot in multiple places, so the two code paths are entangled, and b) the return value is effectively of a union type, i.e. you can only interpret the resulting tensor correctly in the context of extra information.

So the proposal is: limit this function to the oneHot==false case (which can still use tf.buffer) and then add a convertToOneHot() function, which can also use the tf.buffer approach to make it very clear what's going on. That function's docstring should point out that tf.oneHot() does the same thing.

OK. Good point. Done. Also updated the doc string accordingly.

---

date-conversion-attention/date_format.js, line 24 at r2 (raw file):

Previously, davidsoergel (David Soergel) wrote…

represent

nit: represents

'represent' is correct. The subject is 'functions' (plural).

---

date-conversion-attention/date_format.js, line 26 at r2 (raw file):

Previously, davidsoergel (David Soergel) wrote…

contains function

nit: contains a function

Changed it to '... contains functions that convert ...'

---

date-conversion-attention/date_format.js, line 181 at r2 (raw file):

Previously, davidsoergel (David Soergel) wrote…

paddin

nit: padding

Done.

---

date-conversion-attention/date_format.js, line 183 at r2 (raw file):

Previously, davidsoergel (David Soergel) wrote…

Clarify that mixed formats are accepted, as opposed to one array with a bunch of dates in the same format. E.g., "Each element of the array must be a date string in one of the formats listed above."

Good point. Done.

---

date-conversion-attention/date_format.js, line 186 at r2 (raw file):

Previously, davidsoergel (David Soergel) wrote…

flota32

typo

Done.

---

date-conversion-attention/date_format.js, line 187 at r2 (raw file):

Previously, davidsoergel (David Soergel) wrote…

iput

typo

Done.

---

date-conversion-attention/date_format.js, line 215 at r2 (raw file):

Previously, davidsoergel (David Soergel) wrote…

flota32

typo

Done.

[caisq]

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

---

date-conversion-attention/date_format_test.js, line 133 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

OK that works-- propagate to the rest of the cases too? I.e. search for 'Prime' in this file and others.

Done.

[caisq]

Note: Don't merge this PR until the next release of tfjs and tfjs-node happens, because it depends on a bug fix there.

[davidsoergel]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @caisq and @davidsoergel)

---

date-conversion-attention/date_format.js, line 146 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…

OK. Good point. Done. Also updated the doc string accordingly.

Great, so now you don't need the oneHot argument

---

date-conversion-attention/date_format.js, line 24 at r2 (raw file):

Previously, caisq (Shanqing Cai) wrote…

'represent' is correct. The subject is 'functions' (plural).

Oh indeed, sorry