-
Notifications
You must be signed in to change notification settings - Fork 371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conv1d and batchnorm1d layers #209
base: master
Are you sure you want to change the base?
Conversation
12d4184
to
b85cda2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code compiles, runs as expected. numerical checks passed.
Minor modifications, additional code explanation needed.
Possible superfluous code in conv1d_utils.*
c_reference/src/conv1d.c
Outdated
|
||
const ConvLayers_LR_Params* tparams= (ConvLayers_LR_Params*) params; | ||
|
||
if (padding == -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the header files doesn't say much about what setting padding to -1 (or any other value) means
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some documentation for padding. I removed this -1 part altogether to avoid confusions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was the -1 for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally thought of keeping a case to calculate the padding automatically to maintain the time steps.
So if it was -1, the padding would be calculated such that input time steps would be equal to the output time steps. It was similar to the "same" convolution in tensorflow
But since it would be confusing for the user, I removed those lines
@@ -0,0 +1,272 @@ | |||
// Copyright (c) Microsoft Corporation. All rights reserved. | |||
// Licensed under the MIT license. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We need some description of the overall architecture, and clarification over the form of input being tested and the nature of output.
- can this code be extended for streaming data easily?
- The file name could also be more specific as to which model arch for keyword spotting tested here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the description and a note on what to cater to when porting the code to the microcontroller.
The code needs minor modifications for handling the streaming data. These changes are mostly variable assignments, for example fixing input streaming buffer it steps, feature length
I've changed the file to phoneme_det_cnn_rnn to signify the use of the combined arch
memcpy(rnn_out + ((out_index++)*RNN_O_F), temp_hiddenstate, rnn_hidden*sizeof(float)); | ||
} | ||
|
||
// Backward Pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forward and backward passes could be explained a bit in text here so the reader can follow the intent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved the bricking to a separate file altogether. This bricking module has been added to the src folder, so that it can be used as function for other applications as well.
I've added a detailed documentation and line comments explaining how the code works. I've even included notes on how to use the bi-directional option and have highlighted the constrains for the bi-directional application
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a quick pass. I'll look more thoroughly after these minor changes are updated.
Also can we have more expressive macro names than |
I've incorporated all the modifications as instructed. I'll work on unifying the test benches and will rectify the macro expressiveness . I'll make these changes and push the commit for the same shortly |
I've changed the macros to a more expressive name. I have also combined the test-bench files as mentioned above |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is riddled with typos, and it makes little sense to point them out one by one. Please spend some time self-reviewing the code and documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not making any optimization passes over this as of now. Otherwise it looks good. Some minor comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments, full review pending.
Sorry for the delay in the new commit. All the changes have been made. I have even added a couple of extra lines of comments for the error metrics for better clarity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments, otherwise looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. @harsha-simhadri This can be merged now.
No description provided.