Add Time Step Recurrent Neural Network #193
Conversation
…can baseline for 2.0 and provide some support for community's requests.
|
This will also provide an answer for #138 |
|
This will also provide an answer for #109 |
ensure that rnn.js uses defaults of constructor
src/recurrent/matrix/equation.js
Outdated
| left: m, | ||
| product: product, | ||
| forwardFn: copy, | ||
| // backpropagationFn: () => { |
There was a problem hiding this comment.
Do you want to leave in the commented out code?
There was a problem hiding this comment.
I did not actually mean to add this method, removing.
| let outputSize = this.outputSize; | ||
| let lastHiddenSize = this.hiddenSizes[this.hiddenSizes.length - 1]; | ||
|
|
||
| //whd |
There was a problem hiding this comment.
What does whd and bd mean? Is this just a reference to other code values?
There was a problem hiding this comment.
I did mean to keep this, it reflects the same/similar function from the RNN class. This code will be deprecate in v2, so I think changing it would either require it changed in both locations or to just leave it alone. This was originally in recurrent.js, which was used as a foundation to eventually get to v2. I think the less work the better, but for the right reasons, so I think leave it.
| * @param {Number[]} input | ||
| * @returns {number} | ||
| */ | ||
| runInput(input) { |
There was a problem hiding this comment.
Where this method is not meant to be used by the library user, perhaps an _ before this method name would be beneficial? (it's semicolons but I think helps clarity)
There was a problem hiding this comment.
I'd like to leave it as it is for the time being, this code will be deprecated soon, and this is just used for baselining the new GPU stuff, changing it means we are going to maintain it and we've been very vocal that this the recurrent classes were beta. Also the deprecated argument.
| } | ||
| const outputs = []; | ||
|
|
||
| if (this.inputSize === 1) { |
There was a problem hiding this comment.
Could we change this from:
if (...) {
for (<loop A> ) {
for (<loop B>) {
// code block A
}
}
} else {
for (<loop A> ) {
for (<loop B>) {
// code block B
}
}
}
to:
for (<loop A> ) {
for (<loop B>) {
if (...) {
// code block A
} else {
// code block B
}
}
}
Man that took way longer then it should have to abstract it lol. Again it's semicolans but I think it helps clarity (and it looks like the only line that changes is: const error = output.weights[i] - next; vs const error = output.weights[i] - next[i]; unless I am missing something.
There was a problem hiding this comment.
I'm not going to waist time changing this because it is going to be deprecated.
| return errorSum; | ||
| } | ||
|
|
||
| runBackpropagate() { |
There was a problem hiding this comment.
Again an _ I think helps clarity
There was a problem hiding this comment.
perhaps, but deprecated.
| this.bindEquation(); | ||
| } | ||
| let lastOutput; | ||
| if (this.inputSize === 1) { |
There was a problem hiding this comment.
Again we could combine these loops into 1 that has the logic in it for if (this.InputSize === 1) { to just contain the runInput([input[i]]); or runInput(input[i]);
Again Semicolons.
| * | ||
| * @returns {Function} | ||
| */ | ||
| toFunction() { |
There was a problem hiding this comment.
Thank you for including this!! I love that it's stubbed out and throws, so it is obviously clear if ever used what went wrong, but when we are ready to implement it we already have it stubbed out and just need to finish it.
| } | ||
| } | ||
|
|
||
| RNNTimeStep.defaults = { |
There was a problem hiding this comment.
Did we get this represented in the documentation anywhere? (probably did but I haven't gotten that far yet).
There was a problem hiding this comment.
Will document that now.
| @@ -110,7 +110,7 @@ describe('lstm', () => { | |||
| assert(lastOutput.length > 0); | |||
| assert.equal(dataFormatter.toCharacters(net.toFunction()()).join(''), lastOutput); | |||
| }); | |||
| it.only('can include the DataFormatter', () => { | |||
test/recurrent/rnn-time-step.js
Outdated
| /* NOTE: TimeStep here is deprecated though being committed as something new, it is the first feature we want using | ||
| recurrent.js because it is simple one of the simplest recurrent neural networks and serves as a baseline to completing | ||
| the GPU architecture. This test is written so as to create the baseline we can measure against. | ||
| We get this working, we have a baseline, we finish recurrent.js, we change the world. |
There was a problem hiding this comment.
You are too optimistic here, can you word it a bit more gloomily?
There was a problem hiding this comment.
Could your give me an example of what you are looking for?
|
Oh, but also great job on this! I don't think I actually said it but I did think it. You have done some awesome stuff here so sorry to be the rain on you're parade.
I also think I might be gloomy because I started reading this book recently and makes me sad. |
|
Since the items were suggests and the actual net worked and closely resemble the existing recurrent neural network and will be removed after GPU implementations from the |
|
|
Add basic implementation of time step recurrent neural network so we can baseline for 2.0 and provide some support for community's requests.
This will answer #192
Description
Motivation and Context
issue
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Author's Checklist:
Reviewer's Checklist: