fix max-pool & full-connected layer problem when using nnpack#398
Conversation
|
Unfortunately I get totally different results from FC layer with NNPack than with tinydnn backend. And using maxpool crashes my application inside a fc layer (does not matter if tinydnn or nnpack backend) that comes after a maxpool. Conv layer (nnpack) after a nnpack maxpool does not crash. |
|
@Kamairo could you open a ticket for this specific issue? Report backtrace and so on. Thx! |
|
@edgarriba the conflict in maxpool_op_nnpack.h is solved |
|
@azsane cool! just fix style typo that I pointed out in my review and we'll be good to merge. Also notice https://github.com/tiny-dnn/tiny-dnn/pull/416/files#diff-26d451c6b2b93a9e871ebcc919d1488bR98 |
|
@edgarriba I didn't find review in 398...so I just modified some variables like #416 and the thing should be noticed is that conv has changed to nnp_convolution_output? |
| float* output_ptr = reinterpret_cast<float*>(&out_data[0]); | ||
| const float* kernel_ptr = W.data(); | ||
| const float* input_ptr = in_data[0].data(); | ||
| float* output_ptr =out_data[0].data(); |
| pthreadpool_destroy(threadpool); | ||
|
|
||
| // TODO: find a proper way to do this | ||
| output_ptr =out_data[0].data(); |
| throw nn_error("NNPACK Max-Pool requires a pool size == 2."); | ||
| } | ||
|
|
||
| */ |
There was a problem hiding this comment.
@azsane ok, if we are pretty sure of this just remove it and keep your comment
|
|
||
| const auto status = | ||
| nnp_max_pooling_output( | ||
| 1, |
There was a problem hiding this comment.
@azsane as I see from the documentation this one represents the batch_size.
Could you add any comment or todo here in order to handle a bigger batch size in a future?
| #ifdef CNN_USE_NNPACK | ||
| TEST(fully_connected, forward_nnp) | ||
| { | ||
| nnp_initialize(); |
There was a problem hiding this comment.
@azsane do you think that we could call this during the layer instantiation?
| TEST(max_pool, forward_stride_nnp_not_2x2) { | ||
| nnp_initialize(); | ||
| max_pooling_layer<identity> l(4, 4, 1, 3, 1, | ||
| core::backend_t::nnpack); |
There was a problem hiding this comment.
typo error: one space missing
|
@azsane it's fine. I'll fix typos myself. Please could you check why nnp_convolution_inference breaks the tests? |
|
@edgarriba I just ran the conv test with latest master branch,and all passed. |
code modified pass the test for max-pool & full-connected layer