Refactor networks #7

0x33467 · 2016-12-09T14:13:20Z

A better implementation of the neural nets that allows to add new activation functions easily

now fits mljs guideline and easy to add new activation functions

targos · 2016-12-09T14:19:26Z

Is it a fix for #6 ?
/cc @zafarali

0x33467 · 2016-12-09T14:22:00Z

yes, I didn't implement the functions that receives the alpha parameter but it should be easy to do.

targos · 2016-12-09T14:28:39Z

don't worry about the travis failures. nodejs.org has issues at the moment: nodejs/build#562

zafarali · 2016-12-09T14:42:02Z

Looks good!

zafarali · 2016-12-10T00:10:07Z

It seems that all layers get the same activation with this commit?

Also wouldn't it be better to allow activation function parameters to be passed as an object

activation_params = {
 alpha: 0.005,
 gamma: 0.04
}

and then the activation functions can pick out whatever parameters necessary?

0x33467 · 2016-12-10T00:30:44Z

ok, I will change the activation params. I you can pass the parameters as an array or object but now the current parametric activation functions takes only 1 argument as the Wikipedia functions that you propose...

targos · 2016-12-10T12:47:23Z

src/ActivationFunctions.js

@@ -0,0 +1,88 @@
+'use strict';


please use a lower case name for this file (it doesn't export a class)

targos · 2016-12-10T12:51:41Z

src/Utils.js

+
+var Matrix = require('ml-matrix');
+
+class Utils {


Can you make it a usual export of functions instead of a class ?
Then the filename can be lowercase

targos · 2016-12-10T12:52:26Z

src/Utils.js

+     * @param {Matrix} B
+     * @return {Matrix} A
+     */
+    static matrixSum(A, B) {


Isn't it the same as A.add(B) ? Same question for the other methods

in the current version of matrix I didn't find those methods.

They are currently not documented but they exist.
See here for a list of all supported operators and here for general Math methods.

targos · 2016-12-10T12:53:22Z

src/index.js

@@ -1,3 +1,3 @@
 'use strict';

-module.exports = require('./feedforwardNeuralNetwork');
+module.exports = require('./FeedForwardNeuralNetwork');


instead of re-exporting FeedForwardNeuralNetwork from here, you can remove this file and change the main field in package.json

targos · 2016-12-10T12:59:20Z

src/Layer.js

+     * @return {Matrix} the new delta values for the next layer.
+     */
+    backpropagation(delta, a) {
+        this.dW = a.transpose().mmul(delta);


You can win some time and memory by using a Matrix view for the transpose here and in other places. Just replace a.transpose() with a.transposeView().
This is a relatively new feature. You can see how it works here: https://github.com/mljs/matrix/blob/master/src/views/transpose.js
And the list of currently implemented views: https://mljs.github.io/matrix/#Matrix#transposeView

also small changes on filenames and export methods.

0x33467 · 2016-12-12T14:07:39Z

I'm getting a timeout error, should I do a shorter test?

targos · 2016-12-12T14:10:46Z

If you know it will take less than say 10secs, you can call this.timeout(10000) in the corresponding test: http://mochajs.org/#test-level
Otherwise if it's too long or unpredictable, yes, you should try to make it shorter.

0x33467 · 2016-12-12T19:36:01Z

ok, those changes are now ok? @targos

targos · 2016-12-14T13:43:26Z

Thanks, I'll finish the review tomorrow

maasencioh · 2016-12-16T16:57:51Z

src/FeedForwardNeuralNetwork.js

+
+var Layer = require('./Layer');
+var OutputLayer = require('./OutputLayer');
+var Utils = require('./utils');


Maybe use const for this cases?

maasencioh · 2016-12-16T17:00:38Z

src/FeedForwardNeuralNetwork.js

+     * @param {number} [options.activationParam=1] - if the selected activation function needs a parameter.
+     */
+    constructor(options) {
+        if (options === undefined) options = {};


options = options || {} it's just nicer

maasencioh · 2016-12-16T17:01:35Z

src/FeedForwardNeuralNetwork.js

+            this.activation = options.activation === undefined ? 'tanh' : options.activation;
+            this.activationParam = options.activationParam === undefined ? 1 : options.activationParam;
+            if (!(this.activation in Object.keys(ACTIVATION_FUNCTIONS))) {
+                //console.warn("Setting default activation function: 'tanh'");


this old code commented should be removed or at least comment why it's commented like this

0x33467 · 2016-12-16T17:21:41Z

Ok, I added your changes @maasencioh

targos

I don't see any outstanding issue. If @maasencioh is OK with it, you can merge. Do not forget to make a major version bump if you release it, because the API has changed.

maasencioh · 2016-12-16T20:13:39Z

For me you can merge, thanks!

0x33467 added 12 commits December 7, 2016 16:20

forward refactor working.

2737d85

derivatives are correctly calculated

1a0d222

neural network's weight updates correctly

599882c

neural network working nice with multiple layers

e3866d3

each class on his own file

be53c09

first test pass

78a342e

refactor working with all tests

0fe00e0

refactor(all): better implementation

6bc5661

now fits mljs guideline and easy to add new activation functions

feat(ActivationFunctions): new activation functions

4c24121

feat(ActivationFunctions): new activation functions

6ae5448

docs(all): updated docs for the new implementation

3c9a6a7

fix doc

1aef284

changing let for var

1d80ed6

0x33467 added 2 commits December 9, 2016 16:11

fix output layer derivative

43e7daa

feat(FeedForwardNeuralNetwork): parametric activation functions added.

ec89a4e

I forgot linter check :(

ae9520a

targos requested changes Dec 10, 2016

View reviewed changes

refactor(all): use Matrix operations

900da50

also small changes on filenames and export methods.

test: add timeout for big case test

ffcbccf

refactor: change utils filename

9cb1542

maasencioh approved these changes Dec 16, 2016

View reviewed changes

refactor(FeedForwardNeuralNetwork): small changes suggested added

ea61272

targos approved these changes Dec 16, 2016

View reviewed changes

0x33467 merged commit aad95d8 into master Dec 16, 2016

zafarali mentioned this pull request Dec 19, 2016

Add RELU and LeakyRELU activation functions #5

Closed

targos deleted the refactorNetworks branch December 19, 2016 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor networks #7

Refactor networks #7

0x33467 commented Dec 9, 2016

targos commented Dec 9, 2016

0x33467 commented Dec 9, 2016

targos commented Dec 9, 2016

zafarali commented Dec 9, 2016

zafarali commented Dec 10, 2016 •

edited

Loading

0x33467 commented Dec 10, 2016

targos Dec 10, 2016

targos Dec 10, 2016

targos Dec 10, 2016

0x33467 Dec 10, 2016

targos Dec 10, 2016

targos Dec 10, 2016

targos Dec 10, 2016

0x33467 commented Dec 12, 2016

targos commented Dec 12, 2016 •

edited

Loading

0x33467 commented Dec 12, 2016 •

edited

Loading

targos commented Dec 14, 2016

maasencioh Dec 16, 2016

maasencioh Dec 16, 2016

maasencioh Dec 16, 2016

0x33467 commented Dec 16, 2016

targos left a comment

maasencioh commented Dec 16, 2016

Refactor networks #7

Refactor networks #7

Conversation

0x33467 commented Dec 9, 2016

targos commented Dec 9, 2016

0x33467 commented Dec 9, 2016

targos commented Dec 9, 2016

zafarali commented Dec 9, 2016

zafarali commented Dec 10, 2016 • edited Loading

0x33467 commented Dec 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x33467 commented Dec 12, 2016

targos commented Dec 12, 2016 • edited Loading

0x33467 commented Dec 12, 2016 • edited Loading

targos commented Dec 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x33467 commented Dec 16, 2016

targos left a comment

Choose a reason for hiding this comment

maasencioh commented Dec 16, 2016

zafarali commented Dec 10, 2016 •

edited

Loading

targos commented Dec 12, 2016 •

edited

Loading

0x33467 commented Dec 12, 2016 •

edited

Loading