Support full convention in quantized pooling #13260

TaoLv · 2018-11-14T04:33:44Z

Description

Fix quantized pooling to support full convention
Enable quantized pooling in INT8 SqueezeNet example

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Feature1, tests, (and when applicable, API doc)
Feature2, tests, (and when applicable, API doc)

Comments

If this change is a backward incompatible change, why must this change be made.
Interesting edge cases to note here

TaoLv · 2018-11-14T13:17:56Z

This PR depends on #11047 to past the new unit test.

kalyc · 2018-11-16T20:47:23Z

@TaoLv thanks for your contribution - could you please take a look at the failed CI and re-trigger it?

kalyc · 2018-11-16T20:47:36Z

@mxnet-label-bot add [pr-work-in-progress]

TaoLv · 2018-11-17T03:53:27Z

@kalyc Code was re-based and passed the CI. It's ready for review now.

pengzhao-intel · 2018-11-18T01:19:17Z

@zheng-da @reminisce @szha @eric-haibin-lin @apeforest
This is an important enhancement for quantization flow of MKL-DNN in r1.4.
Please help take a review

kalyc · 2018-11-19T17:49:41Z

@mxnet-label-bot update [pr-awaiting-review]

apeforest

Thanks for your contribution.

example/quantization/imagenet_gen_qsym_mkldnn.py

src/operator/quantization/quantized_pooling.cc

apeforest · 2018-11-19T18:58:05Z

src/operator/quantization/quantized_pooling.cc

+                  (dshape[3] + 2 * param.pad[1] - param.kernel[1]) /
+                      param.stride[1];
+    } else {
+      oshape[2] = 1 + static_cast<int>(std::ceil(


It might be good to write a function to do such calculation, with a flag pool_enum to distinguish the way of calculating the shape. kValid: using floor, otherwise, using ceil.

You are probably right. But this code snippet was copied from FP32 pooling infer shape here: https://github.com/apache/incubator-mxnet/blob/master/src/operator/nn/pooling.cc#L163

I try not to change the code of FP32 pooling in this PR and keep the code align between quantized pooling and FP32 pooling. So I would keep it as is and refactor them in another PR in the future. What do you think?

apeforest · 2018-11-19T18:59:08Z

tests/python/quantization/test_quantization.py

@@ -214,7 +214,7 @@ def check_quantized_conv(data_shape, kernel, num_filter, pad, stride, no_bias, q

 @with_seed()
 def test_quantized_pooling():
-    def check_quantized_pooling(data_shape, kernel, pool_type, pad, stride, global_pool, qdtype):
+    def check_quantized_pooling(data_shape, kernel, pool_type, pad, stride, global_pool, qdtype, convention):


I suggest we keep this test to test the default behavior without passing in convention. Add another test to pass in convention as an extra argument.

Having another check function like check_quantized_pooling_with_convention may need much redundant code here. Do you think we can give the argument convention a default value here?

It's okay to only keep this test method. But As @yuxihu mentioned, you may want to test the backward compatibility of the operator without passing this argument.

…to fix-quantized-pooling

yuxihu · 2018-11-20T17:44:47Z

tests/python/quantization/test_quantization.py

@@ -214,7 +214,7 @@ def check_quantized_conv(data_shape, kernel, num_filter, pad, stride, no_bias, q

 @with_seed()
 def test_quantized_pooling():
-    def check_quantized_pooling(data_shape, kernel, pool_type, pad, stride, global_pool, qdtype):
+    def check_quantized_pooling(data_shape, kernel, pool_type, pad, stride, global_pool, qdtype, convention):


What is the default convention before this PR? By adding a new argument, you are testing "valid" and "full". Has the default case been covered with your change?

The default convention for mxnet pooling is "valid".
https://github.com/apache/incubator-mxnet/blob/master/src/operator/nn/pooling-inl.h#L74

I will set the default value for argument "convention" to "valid" here and revert the change for L264-L267. So the behavior of these 4 test cases will be as before.

yuxihu · 2018-11-20T17:46:04Z

tests/python/quantization/test_quantization.py

@@ -244,7 +245,8 @@ def check_quantized_pooling(data_shape, kernel, pool_type, pad, stride, global_p
        quantized_pooling = mx.sym.contrib.quantized_pooling(data=qdata, min_data=min_data,
                                                                max_data=max_data, kernel=kernel,


May be also good to fix the alignment from L246-L249.

yuxihu · 2018-11-20T17:49:12Z

tests/python/quantization/test_quantization.py

@@ -244,7 +245,8 @@ def check_quantized_pooling(data_shape, kernel, pool_type, pad, stride, global_p
        quantized_pooling = mx.sym.contrib.quantized_pooling(data=qdata, min_data=min_data,
                                                                max_data=max_data, kernel=kernel,
                                                                pad=pad, stride=stride, pool_type=pool_type,
-                                                                global_pool=global_pool)
+                                                                global_pool=global_pool,
+                                                                pooling_convention=convention)


same question here: what was the default value for pooling_convention? Make sure the current case is covered.

Should be "valid". https://mxnet.incubator.apache.org/api/python/ndarray/contrib.html#mxnet.ndarray.contrib.quantized_pooling

…to fix-quantized-pooling

yuxihu

LGTM.

apeforest

LGTM

…to fix-quantized-pooling

TaoLv · 2018-11-22T07:47:29Z

@marcoabreu I don't have any idea about the CI failure. Could you help to take a look? Thanks.

TaoLv requested review from anirudh2290 and szha as code owners November 14, 2018 04:33

TaoLv added 3 commits November 14, 2018 21:14

fix quantized pooling and enable it in INT8 SqueezeNet

9aebcec

add test

d2c8787

fix test

5198063

TaoLv force-pushed the fix-quantized-pooling branch from 5191444 to 5198063 Compare November 14, 2018 13:16

marcoabreu added the pr-work-in-progress PR is still work in progress label Nov 16, 2018

Merge remote-tracking branch 'origin/master' into fix-quantized-pooling

867dbfa

TaoLv changed the title ~~[WIP] Support full convention in quantized pooling~~ Support full convention in quantized pooling Nov 17, 2018

marcoabreu added pr-awaiting-review PR is waiting for code review and removed pr-work-in-progress PR is still work in progress labels Nov 19, 2018

apeforest suggested changes Nov 19, 2018

View reviewed changes

TaoLv added 2 commits November 20, 2018 09:18

address review comments

fccd6c2

Merge branch 'master' of https://github.com/apache/incubator-mxnet in…

16899fb

…to fix-quantized-pooling

yuxihu reviewed Nov 20, 2018

View reviewed changes

yuxihu suggested changes Nov 20, 2018

View reviewed changes

TaoLv added 2 commits November 21, 2018 14:53

refine the test for quantized pooling

fb10846

Merge branch 'master' of https://github.com/apache/incubator-mxnet in…

4c673e6

…to fix-quantized-pooling

yuxihu approved these changes Nov 21, 2018

View reviewed changes

apeforest approved these changes Nov 22, 2018

View reviewed changes

Merge branch 'master' of https://github.com/apache/incubator-mxnet in…

2abd701

…to fix-quantized-pooling

reminisce merged commit cd0ce3b into apache:master Nov 23, 2018

pengzhao-intel mentioned this pull request Dec 17, 2018

update with release notes for 1.4.0 release #13657

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support full convention in quantized pooling #13260

Support full convention in quantized pooling #13260

TaoLv commented Nov 14, 2018

TaoLv commented Nov 14, 2018

kalyc commented Nov 16, 2018

kalyc commented Nov 16, 2018

TaoLv commented Nov 17, 2018

pengzhao-intel commented Nov 18, 2018

kalyc commented Nov 19, 2018

apeforest left a comment

apeforest Nov 19, 2018

TaoLv Nov 20, 2018

apeforest Nov 19, 2018

TaoLv Nov 20, 2018

apeforest Nov 21, 2018

TaoLv Nov 21, 2018

yuxihu Nov 20, 2018

TaoLv Nov 21, 2018

yuxihu Nov 20, 2018

TaoLv Nov 21, 2018

yuxihu Nov 20, 2018

TaoLv Nov 21, 2018

yuxihu left a comment

apeforest left a comment

TaoLv commented Nov 22, 2018

		@@ -244,7 +245,8 @@ def check_quantized_pooling(data_shape, kernel, pool_type, pad, stride, global_p
		quantized_pooling = mx.sym.contrib.quantized_pooling(data=qdata, min_data=min_data,
		max_data=max_data, kernel=kernel,

Support full convention in quantized pooling #13260

Support full convention in quantized pooling #13260

Conversation

TaoLv commented Nov 14, 2018

Description

Checklist

Essentials

Changes

Comments

TaoLv commented Nov 14, 2018

kalyc commented Nov 16, 2018

kalyc commented Nov 16, 2018

TaoLv commented Nov 17, 2018

pengzhao-intel commented Nov 18, 2018

kalyc commented Nov 19, 2018

apeforest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuxihu left a comment

Choose a reason for hiding this comment

apeforest left a comment

Choose a reason for hiding this comment

TaoLv commented Nov 22, 2018