Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add nms op and batched_nms api #40962

Merged

Conversation

RichardWooSJTU
Copy link
Contributor

@RichardWooSJTU RichardWooSJTU commented Mar 25, 2022

PR types

New features

PR changes

OPs

Describe

add nms op in paddle/fluid/operators/detection and corresponding api batched_nms In python/paddle/vision/ops.py

English document preview:
doc_en

中文文档预览:
doc_zh

Tensor* output = context.Output<Tensor>("KeepBoxesIdxs");
int64_t* output_data = output->mutable_data<int64_t>(context.GetPlace());
auto threshold = context.template Attr<float>("iou_threshold");
VLOG(3) << "threshold= " << threshold;
Copy link
Contributor

Choose a reason for hiding this comment

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

这行删掉吧

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已删除


static const int64_t threadsPerBlock = sizeof(int64_t) * 8;

__host__ __device__ static inline int64_t CeilDivide(int64_t n, int64_t m) {
Copy link
Contributor

@heavengate heavengate Mar 28, 2022

Choose a reason for hiding this comment

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

"host device" -> HOSTDEVICE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已修改

heavengate
heavengate previously approved these changes Mar 29, 2022
"Boxes is a Tensor with shape [N, 4] "
"N is the number of boxes "
"in last dimension in format [x1, x2, y1, y2] "
"the relation shold be ``0 <= x1 < x2 && 0 <= y1 < y2``.");
Copy link
Contributor

Choose a reason for hiding this comment

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

shold -> should

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!


HOSTDEVICE static inline int64_t CeilDivide(int64_t n, int64_t m) {
return (n + m - 1) / m;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This common function can be put in nms_op.h, then remove this function in .cc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

template <typename T>
static __device__ inline bool CalculateIoU(const T* const box_1,
const T* const box_2,
const float threshold) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as "CeilDivide"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

else:
continue

return selected_indices
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove duplicated code, import from other file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

return out


def batched_nms(boxes, scores, category_idxs, categories, iou_threshold, top_k):
Copy link
Contributor

Choose a reason for hiding this comment

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

please also add unit testing for Dynamic to Static

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added test_batched_nms_dynamic_to_static function in python/paddle/fluid/tests/unittest/test_batched_nms.py

return out


def batched_nms(boxes, scores, category_idxs, categories, iou_threshold, top_k):
Copy link
Contributor

@XiaoguangHu01 XiaoguangHu01 Mar 30, 2022

Choose a reason for hiding this comment

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

nms和batched_nms是否是业界普遍的叫法?
batched_nms和nms这两个api是否能在对外接口层合并?比如,默认category_idxs=None,表示nms的情况

  1. 一般api不区分batched和非batched的形式,非batched认为是batched一种特殊形式,比如不会有batched_conv
  2. batched一般理解是输入数据第0维度表示样本的batch,但这里的含义并不相同

Copy link
Contributor Author

Choose a reason for hiding this comment

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

当时使用该名称时仅参考了pytorch的设计,经调研batched_nms仅在pytorch中使用。与指导人讨论后认为batched一次确实无法较好概括该API所实现的功能。目前已经将这两个API合并为同一个nms,使用参数进行区分。

heavengate
heavengate previously approved these changes Mar 31, 2022
… add_nms_op_and_batched_nms

merge to modify unittest CMakefile.txt to skip test_ops_nms
Copy link
Contributor

@TCChenlong TCChenlong left a comment

Choose a reason for hiding this comment

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

LGTM
TODO:Fix api docs

Copy link
Contributor

@XiaoguangHu01 XiaoguangHu01 left a comment

Choose a reason for hiding this comment

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

LG API

std::vector<uint64_t> mask_host(num_boxes * blocks_per_line);
memory::Copy(platform::CPUPlace(), mask_host.data(), context.GetPlace(),
mask_dev, num_boxes * blocks_per_line * sizeof(uint64_t),
context.cuda_device_context().stream());
Copy link
Contributor

Choose a reason for hiding this comment

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

GPU内容拷回CPU后,需要同步,不然后面用到的mask_host极有可能是脏数据。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已线下沟通 下个PR修改

@heavengate heavengate merged commit 7554f42 into PaddlePaddle:develop Apr 5, 2022
@RichardWooSJTU RichardWooSJTU deleted the add_nms_op_and_batched_nms branch April 6, 2022 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants