Options update #137

freddyC · 2018-01-30T07:55:12Z

Description

I updated the training options to be a property of the neural network. You can now pass in any of the training options into the constructor and they will be recognized, set them to the network or pass them into the training and the property will get adjusted and trained appropriately.
I prepended underscores to neural network methods that are intended for private use
I did some general cleanup up with the new properties being on the class, and not passed into the methods.
I updated the tests to reflect changes. I also moved the logs test into their own file.
I included the training options in the saving of JSON so when the network is restored it's training options is restored as well. If a log method is chosen it will restore to simply console.log. The callback method is not restored. This interaction is mentioned in the readme.

Motivation and Context

#111 will be helped but I don't think this finishes it
finishes #120 for the neural-network class

The main focus is maintaining code. This shouldn't change functionality other then extending it where logical. It should help the library be simpler to implement as well as more robust and more maintainable in the future.

How Has This Been Tested?

I added tests to mirror the functional changes. I added in a test for passing things into the constructors. I added tests for the training objects JSONification.

Screenshots (if appropriate):

Types of changes

~~Bug fix (non-breaking change which fixes an issue)~~
~~New feature (non-breaking change which adds functionality)~~
~~Breaking change (fix or feature that would cause existing functionality to not work as expected)~~
Maintenance and simplification

Author's Checklist:

My code focuses on the main motivation and avoids scope creep.
My code passes current tests and adds new tests where possible.
My code is SOLID and DRY.
I have updated the documentation as needed.

Reviewer's Checklist:

I kept my comments to the author positive, specific, and productive.
I tested the code and didn't find any new problems.
I think the motivation is good for the project.
I think the code works to satisfies the motivation.

Made setActivation() accept a param to be more friendly for other users Created log test class Moved the options tests that were for training options into the training options tests made async training options tests. Updates some readme stuff

robertleeplummerjr

can you remove run?

Fantastic work man!

robertleeplummerjr · 2018-01-30T16:39:47Z

test/README.md


 ```
-npm test
+npm run test


I don't think you need run with newer npm.

robertleeplummerjr

perkyguy · 2018-01-30T17:10:20Z

src/neural-network.js

+   *       momentum: (number),
+   *       activation: ['sigmoid', 'relu', 'leaky-relu', 'tanh']
+   */
+  updateTrainingOptions(opts) {


This way of doing this seems a little cumbersome, especially if a new option wants to get added in. Exceptions to the standard (items contained in this.trainOpts are a little obfuscated as well. Maybe you could change this to something more like

Object.keys(NeuralNetwork.trainDefaults).forEach(opt => this.trainOpts[opt] = opts[opt] || this.trainOpts[opt]); this._setLogMethod(opts.log || this.trainOpts.log); this.activation = opts.activation || this.activation;

This makes sense and is cleaner. I am a little less familiar with the new JS magic. The one thing I like about having the longer explicit if .... do this method is for functionality changes that may arise, then you don't have to re-parse to add functionality, but that is a weak argument. Mostly the things that I am less familiar with are harder to read and I try and make it easy to read, doesn't mean it's the best lol.

the problem with the big if (...) {} if (...) {} was that if you want to add in a new option, you have to remember to add it into this list. All of the functions were doing the same thing, except for two exceptions (should those have even been in trainingOpts?), so why not handle the same things the same?

perkyguy · 2018-01-30T17:17:28Z

src/neural-network.js

+   *  Gets JSON of trainOpts object
+   *    NOTE: Activation is stored directly on JSON object and not in the training options
+   */
+  _getTrainOptsJSON() {


Same comment as the one for updateTrainingOptions, we could change this to something like

return Object.keys(NeuralNetwork.trainDefaults) .reduce((opts, opt) => { if (this.trainOpts[opt]) opts[opt] = this.trainOpts[opt]; return opts; }, {});```

The only place that gets weird is with the callbacks. I have it set up right now where a callback doesn't get stored and log just stores true or false. When restored the log method will be set as console.log as long as we handle those this method seems much better.

this is only for log?

iirc the callback isn't stored but if log is a truthy value we store it as true, then when restoring it we restore it to the method console.log

perkyguy · 2018-01-30T17:46:30Z

src/neural-network.js

-    if (!hiddenSizes) {
-      sizes.push(Math.max(3, Math.floor(inputSize / 2)));
+  _verifyIsInitialized(data) {
+    if (this.sizes) return;


should this verify that the net was initialized correctly? Or just that at some point the sizes got set?

My idea was to just verify the size was set but it would probably be a good place to ensure the network was initialized and give the user feedback when they try to run without training.

perkyguy · 2018-01-30T17:54:55Z

src/neural-network.js

-    let hiddenSizes = this.hiddenSizes;
-    if (!hiddenSizes) {
-      sizes.push(Math.max(3, Math.floor(inputSize / 2)));
+  _verifyIsInitialized(data) {


This name should probably change, as it doesn't just verify that the initialization has occurred and will actually do something if needed. Not a big deal, as it's intended as an internal-only function, but still, might be better to refactor it a little to something more descriptive of it's actual function

perkyguy · 2018-01-30T17:56:18Z

src/neural-network.js

-      let sizes = this._getSizesFromData(data);
-      this.initialize(sizes);
-    }
+    this._verifyIsInitialized(data);


This is why I don't like this name...but I commented at the function definition.

yeah good point

perkyguy · 2018-01-30T17:57:00Z

src/neural-network.js


-    while ( status.iterations < options.iterations && status.error > options.errorThresh) {
-      this._checkTrainingTick(data, status, options);
+    while (status.iterations < this.trainOpts.iterations && status.error > this.trainOpts.errorThresh && Date.now() < endTime) {


This PR is supposed to add in timeouts?

Yeah I completely forgot to add that in the pr, sorry. I probably should have broken that out into it's own pr.

perkyguy · 2018-01-30T18:09:21Z

src/neural-network.js

-
-          if (status.error < options.errorThresh) {
+          this._trainingTick(data, status);
+          if (status.error < this.trainOpts.errorThresh || Date.now() > endTime) {


adding in timeouts?

Also, why is this after the tick now?

perkyguy · 2018-01-30T18:14:32Z

src/neural-network.js

+  train(data, options = {}) {
+    this.updateTrainingOptions(options);
+    data = this._formatData(data);
+    const endTime = Date.now() + this.trainOpts.timeout;


adding timeout?

yeah, so they can train for a set time interval as if want.

perkyguy · 2018-01-30T18:17:19Z

This was kind of a large PR, with a little bit of scope creep

freddyC · 2018-01-30T19:15:54Z

yeah sorry I'll try and keep things more concise

freddyCfit added 4 commits January 29, 2018 21:54

Possible suggestions for clarity

91d00a4

Updates

e9f1401

Made setActivation() accept a param to be more friendly for other users Created log test class Moved the options tests that were for training options into the training options tests made async training options tests. Updates some readme stuff

Updating some things

27755d7

Updates the json test as well as doing a make.

0d1a110

robertleeplummerjr requested changes Jan 30, 2018

View reviewed changes

test/README.md Outdated

```

npm test

npm run test

Copy link

Contributor

robertleeplummerjr Jan 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need run with newer npm.

Readme updates

f1a213b

robertleeplummerjr approved these changes Jan 30, 2018

View reviewed changes

freddyC merged commit 7cb31e0 into develop Jan 30, 2018

freddyC deleted the options-update branch January 30, 2018 17:48

freddyC mentioned this pull request Jan 30, 2018

Private Functions in NeuralNetwork #120

Closed

35 tasks

perkyguy reviewed Jan 30, 2018

View reviewed changes

perkyguy mentioned this pull request Jan 30, 2018

Making trainingOption updater/getter less rigid #141

Merged

11 tasks

Options update #137

Options update #137

Uh oh!

Conversation

freddyC commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Author's Checklist:

Reviewer's Checklist:

Uh oh!

robertleeplummerjr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertleeplummerjr left a comment

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!

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!

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!

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!

perkyguy commented Jan 30, 2018

Uh oh!

freddyC commented Jan 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

freddyC commented Jan 30, 2018 •

edited

Loading