Skip to content

Enhance concat op to support empty input.#17015

Merged
jerrywgz merged 3 commits intoPaddlePaddle:developfrom
jerrywgz:enhance_concat
May 5, 2019
Merged

Enhance concat op to support empty input.#17015
jerrywgz merged 3 commits intoPaddlePaddle:developfrom
jerrywgz:enhance_concat

Conversation

@jerrywgz
Copy link
Contributor

@jerrywgz jerrywgz commented Apr 22, 2019

Enhance concat op to allow empty variable in inputs.
For example, In Faster-RCNN-FPN model, RoIs are distributed onto different levels and get roi features by roi_align. In such case, the number of rois in one level might be 0, then causes empty input and output in roi_align. As result, we should support empty variable when use concat op to concatenate roi features.

Copy link
Contributor

@qingqing01 qingqing01 left a comment

Choose a reason for hiding this comment

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

@chengduoZH Please also help to review. Thanks!

if (axis == 0 && ins.size() < 10) {
size_t output_offset = 0;
for (auto* in : ins) {
if (in->numel() == 0UL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better to check:

if (!in || in->numel() == 0UL) {
}

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

continue;
} else {
inputs.push_back(*ins[j]);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

if (ins[j] || in->numel() > 0) {
  inputs.push_back(*ins[j]);
} else {
  continue;
}

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, but here should be || or &&?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, should be &&

Copy link
Contributor

Choose a reason for hiding this comment

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

@jerrywgz please to fix code

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

@chengduoZH
Copy link
Contributor

@jerrywgz You should add unit test for this change.

@jerrywgz
Copy link
Contributor Author

@jerrywgz You should add unit test for this change.

Done

@qingqing01 qingqing01 changed the title Enhance_concat, test=develop Enhance concat op to support empty input. Apr 25, 2019
self.axis = 0

def test_check_grad(self):
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Why pass check_grad ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it will meet fetch error when there is a null grad

@chengduoZH
Copy link
Contributor

@jerrywgz Why the inputs have some empty argument?

@jerrywgz
Copy link
Contributor Author

@jerrywgz Why the inputs have some empty argument?

In FPN detection model, we need to divide rois into four parts and it may produce empty rois in one part. Concat op in caffe2 can support empty argument.
Or maybe we could add a warning when there's empty argument?

continue;
} else {
inputs.push_back(*ins[j]);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@jerrywgz please to fix code

@jerrywgz jerrywgz merged commit a72907b into PaddlePaddle:develop May 5, 2019
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.

3 participants