RandomBBoxCrop to optionally output the indices of the bounding boxes that passed the centroid filter #2374

jantonguirao · 2020-10-19T15:58:31Z

Signed-off-by: Joaquin Anton janton@nvidia.com

Why we need this PR?

It adds new feature needed to effectively handle segmentation masks together with bounding boxes

What happened in this PR?

Fill relevant points, put NA otherwise. Replace anything inside []

What solution was applied:
Added an optional output to RandomBBoxCrop with the indices of the filtered bounding boxes, so that it can be used to select the relevant masks associated with those (in a different operator)
Affected modules and functionalities:
RandomBBoxCrop
Key points relevant for the review:
All
Validation and testing:
Existing tests
Documentation (including examples):
Docstr updated

JIRA TASK: [DALI-1633]

… that passed the centroid filter Signed-off-by: Joaquin Anton <janton@nvidia.com>

JanuszL · 2020-10-19T16:16:26Z

dali/operators/image/crop/bbox_crop.cc

+      const auto &labels_in = ws.Input<CPUBackend>(1);
+      auto &labels_out = ws.Output<CPUBackend>(next_out_idx++);
+      labels_out.Resize({static_cast<Index>(prospective_crop.bbox_indices.size()), 1});
+      int64_t i = 0;


JanuszL · 2020-10-19T16:18:06Z

Can you add a test to check if the 4th output is provided and is a subset of id returned by the coco reader?

Edit
I see that the indexes are relative. You should check at least that they are matching the filtered bboxes in the test.

Signed-off-by: Joaquin Anton <janton@nvidia.com>

mzient · 2020-10-20T09:40:19Z

dali/operators/image/crop/bbox_crop.cc

@@ -323,7 +332,13 @@ The values are:
 .. note::
  If left empty, depending on the number of dimensions ``"WH"`` or ``"WHD"`` will be assumed.
 )code",
-        TensorLayout{""});
+        TensorLayout{""})


Suggested change

TensorLayout{""})

TensorLayout{})

mzient · 2020-10-20T09:42:07Z

dali/operators/image/crop/bbox_crop.cc

+        TensorLayout{""})
+    .AddOptionalArg<bool>(
+        "output_bbox_indices",
+        R"code(If set to True, an extra output ``bbox_indices`` will be returned, containing


Suggested change

R"code(If set to True, an extra output ``bbox_indices`` will be returned, containing

R"code(If set to True, an extra output will be returned, containing

I don't we have named outputs.

mzient · 2020-10-20T09:53:01Z

dali/operators/image/crop/bbox_crop.cc

@@ -529,20 +546,21 @@ class RandomBBoxCropImpl : public OpImplBase<CPUBackend> {
    bool success = false;
    Box<ndim, float> crop{};
    std::vector<Box<ndim, float>> boxes;
-    std::vector<int> labels;
+    SmallVector<int, 128> bbox_indices;


I think that's a bit too big an object - perhaps it should be a regular vector instead or at the very least we should be much more conservative wrt static size (8-16, not 128).

mzient · 2020-10-20T09:53:34Z

dali/operators/image/crop/bbox_crop.cc

-                    bool has_labels)
-        : success(success), crop(crop) {
-      assert(boxes_data.size() == labels_data.size() || !has_labels);
+                    SmallVector<int, 128> indices = {})


Passing a small vector by value is not a good idea.

Signed-off-by: Joaquin Anton <janton@nvidia.com>

mzient · 2020-10-20T17:38:22Z

dali/operators/image/crop/bbox_crop.cc

+    for (int sample_idx = 0; sample_idx < num_samples; sample_idx++) {
+      const auto &prospective_crop = sample_data_[sample_idx].prospective_crop;
+      auto extent = prospective_crop.crop.extent();
+      auto anchor_out_view = view<float>(anchor_out[sample_idx]);


Get a view to whole anchor/shape TLV outside the loop?

mzient · 2020-10-20T17:39:42Z

dali/operators/image/crop/bbox_crop.cc

+    auto &bbox_out = ws.template OutputRef<CPUBackend>(2);
+    bbox_out.Resize(bbox_out_shape);
+    for (int sample_idx = 0; sample_idx < num_samples; sample_idx++) {
+      auto bbox_out_view = view<float>(bbox_out[sample_idx]);


Likewise - get a view outside of the loop?

mzient · 2020-10-20T17:40:04Z

dali/operators/image/crop/bbox_crop.cc

+        auto labels_in_view = view<const int>(labels_in[sample_idx]);
+        auto labels_out_view = view<int>(labels_out[sample_idx]);


...and again.

mzient · 2020-10-20T17:40:21Z

dali/operators/image/crop/bbox_crop.cc

+      }
+      bbox_indices_out.Resize(bbox_indices_out_shape);
+      for (int sample_idx = 0; sample_idx < num_samples; sample_idx++) {
+        auto bbox_indices_out_view = view<int>(bbox_indices_out[sample_idx]);


...and once more.

mzient · 2020-10-20T18:09:14Z

dali/operators/image/crop/bbox_crop.cc

+    for (int sample_idx = 0; sample_idx < num_samples; sample_idx++) {
+      auto &data = sample_data_[sample_idx];
+      tp.AddWork([&, sample_idx](int thread_id) {
+        auto in_boxes_view = view<const float>(in_boxes[sample_idx]);


Get a view to entire batch outside of the loop.

Signed-off-by: Joaquin Anton <janton@nvidia.com>

jantonguirao · 2020-10-20T19:30:29Z

!build

dali-automaton · 2020-10-20T19:36:21Z

CI MESSAGE: [1717854]: BUILD STARTED

dali-automaton · 2020-10-20T21:06:52Z

CI MESSAGE: [1717854]: BUILD FAILED

jantonguirao · 2020-10-21T08:53:02Z

!build

dali-automaton · 2020-10-21T08:55:35Z

CI MESSAGE: [1719715]: BUILD STARTED

Signed-off-by: Joaquin Anton <janton@nvidia.com>

jantonguirao · 2020-10-21T09:31:40Z

!build

dali-automaton · 2020-10-21T09:35:54Z

CI MESSAGE: [1719811]: BUILD STARTED

dali-automaton · 2020-10-21T11:53:18Z

CI MESSAGE: [1719811]: BUILD PASSED

RandomBBoxCrop to optionally output the indices of the bounding boxes…

bd80b94

… that passed the centroid filter Signed-off-by: Joaquin Anton <janton@nvidia.com>

JanuszL reviewed Oct 19, 2020

View reviewed changes

jantonguirao added 2 commits October 19, 2020 18:37

Add tests

e95ec83

Signed-off-by: Joaquin Anton <janton@nvidia.com>

Code review fixes

c53efbf

Signed-off-by: Joaquin Anton <janton@nvidia.com>

JanuszL approved these changes Oct 19, 2020

View reviewed changes

mzient reviewed Oct 20, 2020

View reviewed changes

jantonguirao force-pushed the randombboxcrop_indices branch from 47ff204 to 47f768a Compare October 20, 2020 16:54

Code review fixes

0a46f46

Signed-off-by: Joaquin Anton <janton@nvidia.com>

jantonguirao force-pushed the randombboxcrop_indices branch from 47f768a to 0a46f46 Compare October 20, 2020 16:54

mzient reviewed Oct 20, 2020

View reviewed changes

jantonguirao force-pushed the randombboxcrop_indices branch from e4e0c8d to 3686518 Compare October 20, 2020 18:17

More code review fixes

763cd87

Signed-off-by: Joaquin Anton <janton@nvidia.com>

jantonguirao force-pushed the randombboxcrop_indices branch from 3686518 to 763cd87 Compare October 20, 2020 19:28

mzient approved these changes Oct 21, 2020

View reviewed changes

jantonguirao force-pushed the randombboxcrop_indices branch from 293d361 to 19518d5 Compare October 21, 2020 08:52

Do not change labels dimensionality in RandomBBoxCrop

3ab7730

Signed-off-by: Joaquin Anton <janton@nvidia.com>

jantonguirao force-pushed the randombboxcrop_indices branch from 19518d5 to 3ab7730 Compare October 21, 2020 09:09

jantonguirao merged commit 2df1b24 into NVIDIA:master Oct 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RandomBBoxCrop to optionally output the indices of the bounding boxes that passed the centroid filter #2374

RandomBBoxCrop to optionally output the indices of the bounding boxes that passed the centroid filter #2374

jantonguirao commented Oct 19, 2020

JanuszL Oct 19, 2020 •

edited

Loading

JanuszL commented Oct 19, 2020 •

edited

Loading

mzient Oct 20, 2020

mzient Oct 20, 2020

mzient Oct 20, 2020

mzient Oct 20, 2020

mzient Oct 20, 2020

mzient Oct 20, 2020

mzient Oct 20, 2020

mzient Oct 20, 2020

mzient Oct 20, 2020

jantonguirao commented Oct 20, 2020

dali-automaton commented Oct 20, 2020

dali-automaton commented Oct 20, 2020

jantonguirao commented Oct 21, 2020

dali-automaton commented Oct 21, 2020

jantonguirao commented Oct 21, 2020

dali-automaton commented Oct 21, 2020

dali-automaton commented Oct 21, 2020

	R"code(If set to True, an extra output ``bbox_indices`` will be returned, containing
	R"code(If set to True, an extra output will be returned, containing

		auto labels_in_view = view<const int>(labels_in[sample_idx]);
		auto labels_out_view = view<int>(labels_out[sample_idx]);

RandomBBoxCrop to optionally output the indices of the bounding boxes that passed the centroid filter #2374

RandomBBoxCrop to optionally output the indices of the bounding boxes that passed the centroid filter #2374

Conversation

jantonguirao commented Oct 19, 2020

Why we need this PR?

What happened in this PR?

JanuszL Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

JanuszL commented Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jantonguirao commented Oct 20, 2020

dali-automaton commented Oct 20, 2020

dali-automaton commented Oct 20, 2020

jantonguirao commented Oct 21, 2020

dali-automaton commented Oct 21, 2020

jantonguirao commented Oct 21, 2020

dali-automaton commented Oct 21, 2020

dali-automaton commented Oct 21, 2020

JanuszL Oct 19, 2020 •

edited

Loading

JanuszL commented Oct 19, 2020 •

edited

Loading