Handles User Object Validation #142 #148

freddyC · 2018-02-03T08:49:44Z

Added in validation for training options and tests as well.

Description

Creates validation for when the user sets the training options.

Motivation and Context

issue
This should help users who run into problems to see things they may have done wrong.

How Has This Been Tested?

I created a slew of tests that go over the expected supported functionality

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.

perkyguy · 2018-02-03T15:34:36Z

src/neural-network.js

    };
  }

+  static get trainingValidations() {


Do we need a get for this? Why not just define these in the validation method itself?

sorry I didn't see your comments but yeah I got rid of the getter/setter. Originally when they set this to false I was printing out a message showing what it would do, but I thought that overkill.

perkyguy · 2018-02-03T15:40:12Z

src/neural-network.js

+      iterations: (val) => { return typeof val === 'number' && val > 0; },
+      errorThresh: (val) => { return typeof val === 'number' && val > 0 && val < 1; },
+      log: (val) => { return typeof val === 'function' || typeof val === 'boolean'; },
+      logPeriod: (val) => { return typeof val === 'number' && val > 0; },


logPeriod only matters if you also defined log as well. I think these functions don't have to be a one-to-one mapping for the training options and can instead explicitly check the training options they are testing. This way, they all would be of the form (opts) => { ... }.

I am not sure I completely understand your point. I could couple them so that we only check things that are dependent on each other and I could set them to check the value of the already set property. I chose not to because I wanted to keep each property decoupled. I think that will make it easier to extend (if you change the function of one property you don't have to worry about additional side effects) and I also didn't want to check every property each time I validate, just the properties that are changed. Does that make sense? I am open to changing but I don't yet understand the benefit.

perkyguy · 2018-02-03T15:44:32Z

src/neural-network.js

    this.trainOpts = {};
    this._updateTrainingOptions(Object.assign({}, this.constructor.trainDefaults, options));

+    this._isStrict = true;


If we are going to have multiple validation options, this should be contained within an object that contains the options and this as well. If we are not going to have multiple validation options, this should be named to something more descriptive, e.g. this._strictTrainingOptionValidation or something

totally agree the name didn't fit. I ended up changing it before I saw this comment.

perkyguy · 2018-02-03T15:44:59Z

src/neural-network.js

+   *      When true: setting a training option outside expected range will throw an error
+   *      When false: setting a training option outside expected range will log out and continue
+   */
+  get validateTrainingOptions() { return this._isStrict; }


Return an object? the name suggests there are multiple

I don't see how that name suggests an object is returned. I did change it from being a getter/setter. Originally I was printing out the a log saying what was happening when you set it to false but I thought that was a bit overkill especially once someone understands already how it works.

perkyguy · 2018-02-03T15:45:26Z

src/neural-network.js

+   *      When false: setting a training option outside expected range will log out and continue
+   */
+  get validateTrainingOptions() { return this._isStrict; }
+  set validateTrainingOptions(s) { this._isStrict = s; }


pass in an object?

yeah it was a setter/getter but I got rid of that (since there aren't any intentional side effects)

perkyguy · 2018-02-03T15:53:30Z

test/base/bitwise.js

               {input: [1, 1], output: [0]}];
    testBitwiseAsync(xor, 'xor', done);
-  }).timeout(10000);
+  }).timeout(15000);


Why the change?

perkyguy · 2018-02-03T15:53:39Z

test/base/bitwise.js

              {input: [1, 1], output: [1]}];
    testBitwiseAsync(or, 'or', done);
-  }).timeout(10000);
+  }).timeout(15000);


Why the change?

perkyguy · 2018-02-03T15:53:46Z

test/base/bitwise.js

               {input: [1, 1], output: [1]}];
    testBitwiseAsync(and, 'and', done);
-  }).timeout(10000);
+  }).timeout(15000);


Why the change?

perkyguy · 2018-02-03T15:55:26Z

test/base/trainopts.js

+    assert.throws(() => { net._updateTrainingOptions({ log: 'no strings' }) });
+    assert.throws(() => { net._updateTrainingOptions({ log: 4 }) });
+    assert.doesNotThrow(() => { net._updateTrainingOptions({ log: false }) });
+    assert.doesNotThrow(() => { net._updateTrainingOptions({ log: () => {} }) });


is this a "valid" log function?

yep, it would be user defined. (logging allows the user to either set true and it will console.log or set their own method

I don't understand why the log value can be user defined, that's the point of callback. Buuuuuut, I was a bit too hasty with this and the other "Valid callback" comment. This is fine with me.

No worries. The main idea is so that if the user has their own logging system (i.e. it writes to file or whatever) then they can keep the log to do just that, without having to use up their callback for that.

Meh, I don't think it would "use up" their callback. But this is a moot point, we can continue discussion outside of this PR.

perkyguy · 2018-02-03T15:55:48Z

test/base/trainopts.js

+    assert.throws(() => { net._updateTrainingOptions({ callback: 4 }) });
+    assert.throws(() => { net._updateTrainingOptions({ callback: false }) });
+    assert.doesNotThrow(() => { net._updateTrainingOptions({ callback: null }) });
+    assert.doesNotThrow(() => { net._updateTrainingOptions({ callback: () => {} }) });


Is this a "valid" callback?

yep (it just does nothing with the passed in object, I am not enforcing the params)

freddyC · 2018-02-04T00:39:11Z

@perkyguy I think this is ready for another look-see

perkyguy

Questions more than anything

perkyguy · 2018-02-05T21:28:56Z

src/neural-network.js

    this.trainOpts = {};
    this._updateTrainingOptions(Object.assign({}, this.constructor.trainDefaults, options));

+    this.invalidTrainOptsShouldThrow = true;


why put this on the instance and not just always require valid options? Brain won't train correctly without these set?

perkyguy · 2018-02-05T21:30:24Z

src/neural-network.js

+      callbackPeriod: (val) => { return typeof val === 'number' && val > 0; },
+      timeout: (val) => { return typeof val === 'number' && val > 0 }
+    };
+    Object.keys(options).forEach(key => {


Only reason I would go over all the validation keys rather than the passed in objects, was to ensure validity of all options?

robertleeplummerjr · 2018-02-18T03:52:31Z

@perkyguy you waiting for anything else here?

perkyguy · 2018-02-18T04:45:25Z

I think I'm good with all of this. Fix the merge conflicts though, and make sure you build prior to merging.

Added in validation for training options and tests as well. Updates the readme Updates the readme again and uses a property for `validatesTrainOpts` instead of a getter/setter Updated the things I updated the `isStrict` property's name to be longer but more descriptive I reverted the timeout for async bitwise tests I moved the `validateTrainingOptions` method to be static Updated readme and got names a little better. Updates the things

Updates to iterate over the whole object each time.

Rebased against develop ran the make command double checked tests

robertleeplummerjr · 2018-02-18T14:25:01Z

nice

freddyC requested review from perkyguy and robertleeplummerjr February 3, 2018 08:56

perkyguy requested changes Feb 3, 2018

View reviewed changes

freddyC force-pushed the valid-options branch from 877adf1 to b40f305 Compare February 4, 2018 06:50

perkyguy reviewed Feb 5, 2018

View reviewed changes

freddyC force-pushed the valid-options branch from b40f305 to 5c2d010 Compare February 10, 2018 20:26

perkyguy self-requested a review February 14, 2018 16:49

perkyguy approved these changes Feb 18, 2018

View reviewed changes

freddyCfit added 3 commits February 17, 2018 23:08

Updated to always throw an error.

b8a5831

Updates to iterate over the whole object each time.

Getting ready to merrrrrrrge

23af33d

Rebased against develop ran the make command double checked tests

freddyC force-pushed the valid-options branch from e3c3fc9 to 23af33d Compare February 18, 2018 06:12

freddyC merged commit 2a0093e into develop Feb 18, 2018

freddyC deleted the valid-options branch February 18, 2018 06:15

Handles User Object Validation #142 #148

Handles User Object Validation #142 #148

Conversation

freddyC commented Feb 3, 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?

Types of changes

Author's Checklist:

Reviewer's Checklist:

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!

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

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freddyC commented Feb 4, 2018

Uh oh!

perkyguy 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!

robertleeplummerjr commented Feb 18, 2018

Uh oh!

perkyguy commented Feb 18, 2018

Uh oh!

robertleeplummerjr commented Feb 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

freddyC commented Feb 3, 2018 •

edited

Loading

freddyC Feb 3, 2018 •

edited

Loading

freddyC Feb 3, 2018 •

edited

Loading