RafLit commented on a change in pull request #20855:
URL: https://github.com/apache/incubator-mxnet/pull/20855#discussion_r820607123
##########
File path: src/operator/contrib/adaptive_avg_pooling.cc
##########
@@ -27,11 +27,13 @@
#include "../operator_common.h"
#include "adaptive_avg_pooling-inl.h"
#if MXNET_USE_MKLDNN == 1
+#include "../nn/mkldnn/mkldnn_base-inl.h"
#include "../nn/mkldnn/mkldnn_pooling-inl.h"
#endif // MXNET_USE_MKLDNN
#define START_IND(a, b, c) static_cast<int>(std::floor(static_cast<float>(a *
c) / b))
#define END_IND(a, b, c) static_cast<int>(std::ceil(static_cast<float>((a + 1)
* c) / b))
+#define DIV_ROUND_UP(a, b) ((a + (b - 1)) / b)
Review comment:
Macro parameters should be wrapped into brackets (b cannot be an
expression currently)
##########
File path: src/operator/nn/pooling-inl.h
##########
@@ -166,6 +196,10 @@ struct hash<mxnet::op::PoolingParam> {
ret = dmlc::HashCombine(ret, val.count_include_pad);
int val_layout = val.layout.has_value() ? val.layout.value() : -1;
ret = dmlc::HashCombine(ret, val_layout);
+ mxnet::Tuple<int> val_out_size = val.output_size.has_value()
Review comment:
shouldn't IsAdaptivePooling be used here?
##########
File path: src/operator/contrib/adaptive_avg_pooling.cc
##########
@@ -219,10 +221,10 @@ bool SupportMKLDNNAveragePooling(const NDArray &in_data,
const int OH = out_data.shape()[2];
const int OW = out_data.shape()[3];
- const int strides_H = floor((IH << 1) / OH) - floor(IH / OH);
- const int strides_W = floor((IW << 1) / OW) - floor(IW / OW);
- const int kernel_H = ceil((IH << 1) / OH) - floor(IH / OH);
- const int kernel_W = ceil((IW << 1) / OW) - floor(IW / OW);
+ const int strides_H = ((IH << 1) / OH) - (IH / OH);
+ const int strides_W = ((IW << 1) / OW) - (IW / OW);
+ const int kernel_H = DIV_ROUND_UP((IH << 1) / OH, 1) - (IH / OH);
+ const int kernel_W = DIV_ROUND_UP((IW << 1) / OW, 1) - (IW / OW);
const int pad_l_top = (strides_H * (OH - 1) + kernel_H - IH) / 2;
const int pad_l_left = (strides_W * (OW - 1) + kernel_W - IW) / 2;
Review comment:
this calculations can seperated into a function as they're repeated in
UseAdaptivePaddingKernel
##########
File path: src/operator/contrib/adaptive_avg_pooling.cc
##########
@@ -27,11 +27,13 @@
#include "../operator_common.h"
#include "adaptive_avg_pooling-inl.h"
#if MXNET_USE_MKLDNN == 1
+#include "../nn/mkldnn/mkldnn_base-inl.h"
#include "../nn/mkldnn/mkldnn_pooling-inl.h"
#endif // MXNET_USE_MKLDNN
#define START_IND(a, b, c) static_cast<int>(std::floor(static_cast<float>(a *
c) / b))
#define END_IND(a, b, c) static_cast<int>(std::ceil(static_cast<float>((a + 1)
* c) / b))
+#define DIV_ROUND_UP(a, b) ((a + (b - 1)) / b)
Review comment:
this can be deleted as it's already defined in
src/operator/nn/mkldnn/mkldnn_pooling-inl.h
##########
File path: tests/python/mkl/test_mkldnn.py
##########
@@ -422,6 +422,35 @@ def check_pooling_training(stype):
check_pooling_training(stype)
+def test_adaptive_pooling():
+ def test_case(num_filter, output_size, stype, shape):
+ data_tmp = mx.nd.random.uniform(shape=shape)
+ data = mx.sym.var('data', stype=stype)
+ in_channels = shape[1]
+
+ data = mx.sym.Convolution(data=data, kernel=(3, 3), pad=(1,1),
num_filter=num_filter)
+ data = mx.sym.contrib.AdaptiveAvgPooling2D(data=data,
output_size=output_size)
+
+ weight_tmp = np.random.normal(-0.1, 0.1, size=(num_filter,
in_channels, 3, 3))
+ bias_tmp = np.random.normal(0.1, 0.1, size=(num_filter,))
+
+ in_location = [mx.nd.array(data_tmp).tostype(stype),
mx.nd.array(weight_tmp).tostype(stype),
+ mx.nd.array(bias_tmp).tostype(stype)]
+
+ check_numeric_gradient(data, in_location, numeric_eps=1e-2, rtol=0.16,
atol=1e-4)
+
+ num_filters = [4, 8, 16]
+ output_sizes = [4, 5, 8, 16]
+ stypes = ['row_sparse', 'default']
+ shapes = [(3, 3, 8, 8), (3, 3, 20, 20), (3, 3, 32, 32)]
Review comment:
could you add some tests for different numbers of channels?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
