make ROIAlign support position-sensitive pooling #13088

shesung · 2018-11-02T11:21:47Z

Description

Modify the RoIAlign operator to support position-sensitive roi pooling.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

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:https://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

Comments

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

pengzhao-intel · 2018-11-02T12:06:10Z

Thanks for the improvements.

Could you add the test case for the new feature?

ankkhedia · 2018-11-02T17:16:56Z

@shesung Thanks for your contribution!
Request you to look into CI failures and add tests for new feature.
Also, could you please update PR description with what this PR actually do

@mxnet-label-bot [pr-awaiting-review, operator]

shesung · 2018-11-02T19:21:39Z

Test case is added and CI failures is fixed.

pengzhao-intel · 2018-11-03T02:04:49Z

src/operator/contrib/roi_align.cc

@@ -213,14 +214,21 @@ void ROIAlignForward(
 num_threads(engine::OpenMP::Get()->GetRecommendedOMPThreadCount())


From our local test, the better performance will be gained if we move this OMP into n_rois loop, L160.
Could you help change this and test the performance with your cases? Thanks.

Time complexity of the new case should be the same with the original one. I did the changes as you suggested and test the runtime for both cases. On my server with two E5-2683 v3, this change did bring obvious performance boost.

pengzhao-intel

@szha to review the API and algorithm change

lupesko · 2018-11-20T01:09:18Z

@shesung thanks for the contribution!
Pulling in some more folks with expertise in this area: @anirudh2290 @apeforest @samskalicky @azai91

vandanavk · 2018-11-26T22:13:28Z

LGTM.

@anirudh2290 @apeforest @samskalicky @azai91 any comments on this PR?

roywei · 2018-12-11T02:15:46Z

@shesung thanks for the contribution, could you trigger CI again?

pengzhao-intel · 2018-12-20T01:45:30Z

@shesung could you rebase the code which will trigger the new CI?
It's almost the last step to complete the PR :)
@anirudh2290

shesung · 2018-12-26T03:22:01Z

@pengzhao-intel @roywei Some error occurred in jenkins. Do you know how to fix it?
http:https://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/mxnet-validation%2Fcentos-gpu/detail/PR-13088/3/pipeline

pengzhao-intel · 2018-12-26T03:46:04Z

Seems the problem of connection. Please try to retrigger the CI by adding a space/blank line in the code.

pengzhao-intel · 2018-12-26T04:06:26Z

@wkcn do you mind take a review for this PR?

shesung · 2018-12-26T06:41:46Z

There seem to be some bugs in test_operator_gpu.test_poisson_generator which lead to failure of CI.

wkcn · 2018-12-26T08:15:47Z

Thanks for your contribution！
The code works well, and I think it will be better to add a CHECK ‘input_channel % ( pooled_width*pooled_height)==0’ when position_sensitive is true.

wkcn · 2018-12-26T08:28:55Z

tests/python/unittest/test_operator.py

@@ -17,6 +17,7 @@

 # pylint: skip-file
 from __future__ import print_function
+from __future__ import division


The types of variables are specific as float or int.
But it is useful for the consistency of division between py2 and py3.

roywei · 2018-12-26T19:52:44Z

Hi, @shesung could you try git rebase to the latest master? Some tests may be flaky, usually rebase to latest master or push an empty comit will solve it.
Running this test locally does not show any error

MXNET_TEST_SEED=1616106831  nosetests  --logging-level=DEBUG  --verbose -s tests/python/gpu/test_operator_gpu.py:test_poisson_generator
[INFO] Setting module np/mx/python random seeds, use MXNET_MODULE_SEED=1447546482 to reproduce.
[WARNING] *** test-level seed set: all "@with_seed()" tests run deterministically ***
test_operator_gpu.test_poisson_generator ... [INFO] Setting test np/mx/python random seeds, use MXNET_TEST_SEED=1616106831 to reproduce.
ok

----------------------------------------------------------------------
Ran 1 test in 5.083s

OK

wkcn · 2018-12-27T02:39:32Z

src/operator/contrib/roi_align.cc

- Shape4(bshape[0], dshape[1], param.pooled_size[0], param.pooled_size[1]));
+ if (param.position_sensitive) {
+ CHECK_EQ(dshape[1] % (param.pooled_size[0]*param.pooled_size[1]), 0) <<
+ "Input channels shuold be divided by pooled_size[0]*pooled_size[1]";


Fix typo：shuold -> should

when position_sensitive is true.

wkcn

Fix typo：shuold -> should

when position_sensitive is true.

Thanks.

wkcn

LGTM.

Thanks for your contribution!

pengzhao-intel · 2019-01-02T11:52:28Z

@TaoLv could you help take a review and merge PR?

TaoLv

Minor comment. The rest looks good to me. Please re-trigger the CI before merging since the last run is about 6 days ago.

src/operator/contrib/roi_align.cc

TaoLv

LGTM

TaoLv · 2019-01-04T16:19:32Z

@shesung Thank you for the contribution. I think it's good to merge now.

* make ROIAlign support position-sensitive pooling * add unittest for RoIAlign op * fix ccplint error * fix python3 compability for unittest * change OMP for better performance * delete blank line to trigger CI * add shape check when position_sensitive is true * fix the typo * typo: shuold -> should * remove private() clause in omp statement

shesung requested a review from anirudh2290 as a code owner November 2, 2018 11:21

marcoabreu added Operator pr-awaiting-review PR is waiting for code review labels Nov 2, 2018

pengzhao-intel reviewed Nov 3, 2018

View reviewed changes

pengzhao-intel approved these changes Nov 7, 2018

View reviewed changes

shesung force-pushed the master branch from 76f9990 to bfff94c Compare December 25, 2018 03:49

wkcn reviewed Dec 26, 2018

View reviewed changes

shesung force-pushed the master branch from 8731e73 to 7340f14 Compare December 27, 2018 02:26

wkcn approved these changes Dec 27, 2018

View reviewed changes

wkcn reviewed Dec 27, 2018

View reviewed changes

wkcn suggested changes Dec 27, 2018

View reviewed changes

wkcn approved these changes Dec 27, 2018

View reviewed changes

TaoLv approved these changes Jan 2, 2019

View reviewed changes

src/operator/contrib/roi_align.cc Outdated Show resolved Hide resolved

shesung added 3 commits January 4, 2019 21:37

make ROIAlign support position-sensitive pooling

aa0715d

add unittest for RoIAlign op

22e6caa

fix ccplint error

a10ac16

shesung added 7 commits January 4, 2019 21:37

fix python3 compability for unittest

5c65872

change OMP for better performance

7301951

delete blank line to trigger CI

713ba58

add shape check when position_sensitive is true

2436541

fix the typo

f36f562

typo: shuold -> should

bb50fef

remove private() clause in omp statement

6a38b03

shesung force-pushed the master branch from baab747 to 6a38b03 Compare January 4, 2019 13:46

TaoLv reviewed Jan 4, 2019

View reviewed changes

TaoLv merged commit 16cb135 into apache:master Jan 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make ROIAlign support position-sensitive pooling #13088

make ROIAlign support position-sensitive pooling #13088

shesung commented Nov 2, 2018 •

edited

Loading

pengzhao-intel commented Nov 2, 2018

ankkhedia commented Nov 2, 2018

shesung commented Nov 2, 2018

pengzhao-intel Nov 3, 2018

shesung Nov 5, 2018

pengzhao-intel left a comment

lupesko commented Nov 20, 2018

vandanavk commented Nov 26, 2018

roywei commented Dec 11, 2018

pengzhao-intel commented Dec 20, 2018

shesung commented Dec 26, 2018 •

edited

Loading

pengzhao-intel commented Dec 26, 2018 •

edited

Loading

pengzhao-intel commented Dec 26, 2018

shesung commented Dec 26, 2018

wkcn commented Dec 26, 2018 •

edited

Loading

wkcn Dec 26, 2018 •

edited

Loading

roywei commented Dec 26, 2018 •

edited

Loading

wkcn Dec 27, 2018

wkcn left a comment •

edited

Loading

wkcn left a comment

pengzhao-intel commented Jan 2, 2019

TaoLv left a comment •

edited

Loading

TaoLv left a comment

TaoLv commented Jan 4, 2019

		@@ -213,14 +214,21 @@ void ROIAlignForward(
		num_threads(engine::OpenMP::Get()->GetRecommendedOMPThreadCount())

make ROIAlign support position-sensitive pooling #13088

make ROIAlign support position-sensitive pooling #13088

Conversation

shesung commented Nov 2, 2018 • edited Loading

Description

Checklist

Essentials

Comments

pengzhao-intel commented Nov 2, 2018

ankkhedia commented Nov 2, 2018

shesung commented Nov 2, 2018

pengzhao-intel Nov 3, 2018

Choose a reason for hiding this comment

shesung Nov 5, 2018

Choose a reason for hiding this comment

pengzhao-intel left a comment

Choose a reason for hiding this comment

lupesko commented Nov 20, 2018

vandanavk commented Nov 26, 2018

roywei commented Dec 11, 2018

pengzhao-intel commented Dec 20, 2018

shesung commented Dec 26, 2018 • edited Loading

pengzhao-intel commented Dec 26, 2018 • edited Loading

pengzhao-intel commented Dec 26, 2018

shesung commented Dec 26, 2018

wkcn commented Dec 26, 2018 • edited Loading

wkcn Dec 26, 2018 • edited Loading

Choose a reason for hiding this comment

roywei commented Dec 26, 2018 • edited Loading

wkcn Dec 27, 2018

Choose a reason for hiding this comment

wkcn left a comment • edited Loading

Choose a reason for hiding this comment

wkcn left a comment

Choose a reason for hiding this comment

pengzhao-intel commented Jan 2, 2019

TaoLv left a comment • edited Loading

Choose a reason for hiding this comment

TaoLv left a comment

Choose a reason for hiding this comment

TaoLv commented Jan 4, 2019

shesung commented Nov 2, 2018 •

edited

Loading

shesung commented Dec 26, 2018 •

edited

Loading

pengzhao-intel commented Dec 26, 2018 •

edited

Loading

wkcn commented Dec 26, 2018 •

edited

Loading

wkcn Dec 26, 2018 •

edited

Loading

roywei commented Dec 26, 2018 •

edited

Loading

wkcn left a comment •

edited

Loading

TaoLv left a comment •

edited

Loading