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

Add RROIAlign #16017

Merged
merged 8 commits into from
Aug 30, 2019
Merged

Add RROIAlign #16017

merged 8 commits into from
Aug 30, 2019

Conversation

ElaineBao
Copy link
Contributor

@ElaineBao ElaineBao commented Aug 27, 2019

Description

Add RROIAlign operator and test case.
Different from ROI Align, RROI Align uses rotated rois, which is suitable for text detection.
Reference paper: Ma, Jianqi, et al. "Arbitrary-Oriented Scene Text Detection via Rotation Proposals."
IEEE Transactions on Multimedia, 2018.

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)
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • RROIAlign
  • test case for RROIAlign

Comments

  • Only forward pass of RROIAlign is implemented.

@pengzhao-intel
Copy link
Contributor

@wkcn could you help to take a review?
FYI, the backward will be submitted later.

Copy link
Contributor

@pengzhao-intel pengzhao-intel left a comment

Choose a reason for hiding this comment

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

Could you provide some performance data?

src/operator/rroi_align.cc Outdated Show resolved Hide resolved
src/operator/rroi_align.cc Outdated Show resolved Hide resolved
@ElaineBao
Copy link
Contributor Author

Could you provide some performance data?

Could you give a hint on what kind of performance data can be provided?

@pengzhao-intel
Copy link
Contributor

Could you provide some performance data?

Could you give a hint on what kind of performance data can be provided?

A simple way is to compare the performance with and without your omp pragma and see how much speedup from your parallelization.

@pengzhao-intel
Copy link
Contributor

btw, I think you still need to register CUDA version and error out with the hint like "No implementation"

@wkcn wkcn added the pr-awaiting-review PR is waiting for code review label Aug 27, 2019
@ElaineBao
Copy link
Contributor Author

Could you provide some performance data?

Could you give a hint on what kind of performance data can be provided?

A simple way is to compare the performance with and without your omp pragma and see how much speedup from your parallelization.

Hi, @pengzhao-intel, the performance w/o omp is as follows:

omp option performance
with omp 0.0586 s
without omp 0.4895 s
speedup 8.35X

Settings for the performance data:
clx-8260L, N=64, C=512, H=16, W=16, num_rois=1000, iterations=1000

src/operator/contrib/rroi_align.cc Outdated Show resolved Hide resolved
src/operator/contrib/rroi_align-inl.h Outdated Show resolved Hide resolved
src/operator/contrib/rroi_align.cc Outdated Show resolved Hide resolved
src/operator/contrib/rroi_align.cc Outdated Show resolved Hide resolved
Copy link
Member

@wkcn wkcn left a comment

Choose a reason for hiding this comment

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

Great work! Thank you! LGTM : )

@pengzhao-intel
Copy link
Contributor

@ciyongch could you take a look as well?

Copy link
Contributor

@pengzhao-intel pengzhao-intel 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.

@wkcn wkcn added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-review PR is waiting for code review labels Aug 29, 2019
@pengzhao-intel pengzhao-intel merged commit 36455b2 into apache:master Aug 30, 2019
@ElaineBao ElaineBao deleted the rroialign branch April 14, 2020 00:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Operator pr-awaiting-merge Review and CI is complete. Ready to Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants