Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Support full convention in quantized pooling #13260

Merged
merged 9 commits into from
Nov 23, 2018

Conversation

TaoLv
Copy link
Member

@TaoLv TaoLv commented Nov 14, 2018

Description

  1. Fix quantized pooling to support full convention
  2. 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
Copy link
Member Author

TaoLv commented Nov 14, 2018

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

@kalyc
Copy link
Contributor

kalyc commented Nov 16, 2018

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

@kalyc
Copy link
Contributor

kalyc commented Nov 16, 2018

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

@marcoabreu marcoabreu added the pr-work-in-progress PR is still work in progress label Nov 16, 2018
@TaoLv TaoLv changed the title [WIP] Support full convention in quantized pooling Support full convention in quantized pooling Nov 17, 2018
@TaoLv
Copy link
Member Author

TaoLv commented Nov 17, 2018

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

@pengzhao-intel
Copy link
Contributor

@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
Copy link
Contributor

kalyc commented Nov 19, 2018

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

@marcoabreu 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
Copy link
Contributor

@apeforest apeforest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution.

example/quantization/imagenet_gen_qsym_mkldnn.py Outdated Show resolved Hide resolved
src/operator/quantization/quantized_pooling.cc Outdated Show resolved Hide resolved
src/operator/quantization/quantized_pooling.cc Outdated Show resolved Hide resolved
(dshape[3] + 2 * param.pad[1] - param.kernel[1]) /
param.stride[1];
} else {
oshape[2] = 1 + static_cast<int>(std::ceil(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

@@ -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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@yuxihu yuxihu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Contributor

@apeforest apeforest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@TaoLv
Copy link
Member Author

TaoLv commented Nov 22, 2018

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

@reminisce reminisce merged commit cd0ce3b into apache:master Nov 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants