-
Notifications
You must be signed in to change notification settings - Fork 1
Maxpool #16
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
base: tdnn-pool
Are you sure you want to change the base?
Maxpool #16
Conversation
src/cudamatrix/cu-kernels.cu
Outdated
| for (int32_cuda q = 0; q < num_col_blocks; q++) { | ||
| dst[index] = fmax( | ||
| src[index_src + p * src_stride * d.rows + q * d.cols], | ||
| dst[index]); |
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.
Don't you need to initialize dst[index]?
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.
but dst is one of the input.
I am not sure how should initialize it?
I just imitate the function "_add_mat_blocks" in this file(line 722).
| src[index_src + p * src_stride * d.cols + q * d.rows], | ||
| dst[index]); | ||
| } | ||
| } |
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.
Why do you need alpha in these functions?
You can modify functions by removing alpha.
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.
OK, I would delete all the alpha.
I was not sure if we need this parameter and since the default value is one, it seems has no impact.
src/cudamatrix/cu-kernels.cu
Outdated
| dst_index = i + j * dst_dim.stride, | ||
| src_index = src_i + src_j * src_dim.stride; | ||
| if (i < dst_dim.cols && j < dst_dim.rows) | ||
| dst[dst_index] += alpha * src[src_index]; |
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.
You need to modify this line.
src/cudamatrix/cu-kernels.cu
Outdated
| void cudaF_max_mat_blocks(dim3 Gr, dim3 Bl, float alpha, const float* src, | ||
| int32_cuda num_row_blocks, int32_cuda num_col_blocks, | ||
| float* dst, MatrixDim d, int src_stride, | ||
| int A_trans) { |
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.
no need for alpha
src/cudamatrix/cu-matrix.h
Outdated
| void AddMatBlocks(Real alpha, const CuMatrixBase<Real> &A, | ||
| MatrixTransposeType trans = kNoTrans); | ||
|
|
||
| void MaxMatBlocks(Real alpha, const CuMatrixBase<Real> &A, |
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.
Add comments containing function and parameter definition
…ion-component)(without cuda functions)
src/cudamatrix/cu-kernels.cu
Outdated
| // loop over all the elements in each pool to find the maximum one, | ||
| // and record its index. | ||
|
|
||
| for (int32_cuda t = 0; t < pool_t_size_; t++) { |
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 think your assumption in this function is that stride in all dimensions are 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.
Do you mean I should set all the stride as 1?
the stride is represented by pool_{t,h,f}_step. I would add some description.
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.
pool_{t,h,f}_step is the stride between blocks, but as far as I found out the stride within block is 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.
Do you mean it is possible that I need to get the maximum of part of the block rather than the whole block?
Like getting 5 from block [1,6,5,7,1] with within_block_stride as 2.
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 mean the rows of block can be something like [1 5 9]!
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 see what you mean, I would fix this! Thank you!
By the way, is this kind of stride only apply for rows? would the column also be like [1 5 9]?
src/cudamatrix/cu-matrix.h
Outdated
| /// function set all the values in &out_deriv whose index is not in | ||
| /// vector(not corresponding to maximum value in each pool of &in_value) | ||
| /// as zero, and keeps those correponding to maximum value as the *in_deriv. | ||
| void MaxMatBlocks(const CuMatrixBase<Real> &A, CuVectorBase<Real> &index_max_, |
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.
Please add description for all variables and explain how you select pooling block w.r.t parameters (You can add formula)
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 have added the description of variables, but these descriptions are almost the same as in the file nnet-convolution-component.h(line 400), I am not sure if we should write it again.
| MaxPoolingOverBlock component was firstly used in ConvNet for selecting an | ||
| representative activation in an area. It inspired Maxout nonlinearity. | ||
| Each output element of this component is the maximum of a block of | ||
| input elements where the block has a 2.5D dimension (pool_t_size_, |
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.
remove 2.5D dimension and just explain block!
remove 'for selecting an representative activation in an area'
|
|
||
| This component is designed to be used after a ConvolutionComponent | ||
| so that the input matrix is propagated from a 2d-convolutional layer. | ||
| This component implements 2.5d-maxpooling which performs |
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.
2.5d is meaningless! You can say it is 3D-maxpooling and explain how the 3d matrix is stored in 2 dim!
|
@qbetterk , please add test code for your function in cu-matrix.*/ |
|
@qbetterk Did you add test code for max function in matrix class? |
|
Hi Pegah,
I did write some test code,but I am not sure I should put it matrix.cc file or write a new cu-matrix-test.cc file like others (cu-math-test.cc).
And I am not in Maryland this week. So, sorry I cannot upload them so far. I would return on this Thursday and would upload the code as soon as Possible.
Best,
Kun
…Sent from my iPhone
On May 15, 2018, at 12:48, pegahgh ***@***.***> wrote:
@qbetterk Did you add test code for max function in matrix class?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Sure, we can discuss about it when you are back! |
No description provided.