Baseball models, client, and server #66

nkreeger · 2018-05-04T21:08:10Z

This is rather large - so getting this started now.

This change is

dsmilkov · 2018-05-05T15:14:58Z

Took a first pass. The 3 major comments I have:

Move from TS to JS to make it more accessible
Add readme that explains what this demo does, how it works and how to build/develop/test
Not blocking: Remove Vue and use plain JS for the UI to simplify the project and make it more accessible

Reviewed 19 of 24 files at r1, 5 of 5 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

.gitignore, line 1 at r2 (raw file):

node_modules

Since we've standardized all the examples, we have a global README.md that talks how to build and run any of the examples, in addition on an example-specific readme that is much simpler. In this case, since this example is more complex (with server/client architecture), we should definitely have a readme with instructions on how this example is structured, what technologies it uses and how to build and test.

.gitignore, line 7 at r2 (raw file):

*.tgz
*.pyc
*_data.json

Store the downloaded data in baseball/dist/ where all generated files go. This way you don't have to git-ignore the json explicitly here.

baseball/package.json, line 1 at r2 (raw file):

rename the folder to baseball-node to clarify that it is a node demo. This will align with other examples where we use -core and -layers suffix

baseball/package.json, line 3 at r2 (raw file):

{
  "name": "tfjs-examples-baseball",
  "version": "0.1.0",

might be good to add "private": "true" - an additional signal we won't publish this to npm.

baseball/package.json, line 5 at r2 (raw file):

  "version": "0.1.0",
  "description": "Deep learning models for classifying baseball metrics",
  "main": "index.js",

do we need this "main" property? I think you can remove it

baseball/package.json, line 9 at r2 (raw file):

    "build": "tsc",
    "build-client": "webpack",
    "link-local": "yalc link",

Are we pretty close to publishing tfjs-node to npm? Asking since if you can publish to npm before we check in this demo, then we don't have to come back and update this demo. Also we will dogfood the npm and simplify the build process (no need to link-local)

baseball/package.json, line 26 at r2 (raw file):

    "clang-format": "^1.2.3",
    "containers.js": "^0.0.6",
    "css-loader": "^0.28.11",

replace all "^..." with "~..." to be a little more strict with the versioning. This helps reduce breakages in the fast moving npm world

baseball/package.json, line 43 at r2 (raw file):

    "yalc": "^1.0.0-pre.22"
  },
  "dependencies": {}

move tfjs, tfjs-node and other things that are used at run-time from devDependencies down here

baseball/tsconfig.json, line 2 at r2 (raw file):

{
  "compilerOptions": {

All of the other tfjs-examples are javascript to make them more accessible. This is based on a consistent feedback we've gotten from the community. Typescript people don't mind JS, JS people mind TS. Hopefully it won't be too hard to remove all ts annotations and turn the files into modern (es7) js.

baseball/webpack.config.js, line 1 at r2 (raw file):

var path = require('path')

add license

baseball/webpack.config.js, line 14 at r2 (raw file):

    rules: [
      {
        test: /\.vue$/,

Again based on community feedback. Not in this PR, but definitely worth filing a bug: To make the demo simpler and more accessible and to keep the focus on the library itself, we should remove vue and just use a plain JS for the UI.

baseball/src/abstract-pitch-model.ts, line 26 at r2 (raw file):

 * Abstract base class for defining Pitch ML models.
 */
export abstract class PitchModel {

noticed it's an abstract class without any abstract methods, whick makes it a bit hard to identify what the job of the subclass is.

baseball/src/pitch-data.ts, line 19 at r2 (raw file):

import * as tf from '@tensorflow/tfjs';
import {sizeFromShape} from '@tensorflow/tfjs-core/dist/util';

Use the public API tf.util.sizeFromShape instead, since this import approach is error prone to tfjs-core moving files around.

baseball/src/utils.ts, line 22 at r2 (raw file):

 */
// tslint:disable-next-line:no-any
export function shuffle(array: any[]): any[] {

use tf.util.shuffle

baseball/src/train/train-pitch-type-model.ts, line 24 at r2 (raw file):

bindTensorFlowBackend();

Is this used only for testing? In general, what's the purpose of the src/train/ folder? Perhaps we can remove it now that the demo is stable.

Comments from Reviewable

dsmilkov · 2018-05-05T15:19:02Z

Review status: all files reviewed at latest revision, 15 unresolved discussions.

baseball/webpack.config.js, line 18 at r2 (raw file):

        options: {
          loaders: {
            // Since sass-loader (weirdly) has SCSS as its default parse mode,

is this needed, since we use plain css in the example?

Comments from Reviewable

nkreeger · 2018-05-05T19:17:55Z

Review status: 0 of 25 files reviewed at latest revision, 16 unresolved discussions.

.gitignore, line 7 at r2 (raw file):