Add SelectMasks operator #2381

jantonguirao · 2020-10-20T17:32:55Z

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

Why we need this PR?

It adds new feature needed to be able to select mask polygons associated with the relevant bounding boxes in image segmentation use cases.

What happened in this PR?

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

What solution was applied:
Added a segmentation.SelectMasks operator that selects a subset of masks based on a list of indices
Affected modules and functionalities:
New operator
Key points relevant for the review:
All
Validation and testing:
Tests added
Documentation (including examples):
Docstr

JIRA TASK: [DALI-1680]

JanuszL · 2020-10-21T20:08:27Z

dali/operators/segmentation/select_masks.cc

+DALI_SCHEMA(segmentation__SelectMasks)
+    .DocStr(R"(Selects a subset of mask by their mask ids.
+
+The operator expects ``mask_meta`` and ``mask_coords``, containing segmentation mask.


I would add an info about mask_ids as well.

JanuszL · 2020-10-21T20:09:07Z

dali/operators/segmentation/select_masks.cc

+
+Example 2: Selecting two out of the three mask, reindexing the mask ids to follow the order of the input mask ids::
+
+``mask_ids`` = [2, 0]


Add leading tabs in this example.

JanuszL · 2020-10-21T20:11:01Z

dali/operators/segmentation/select_masks.cc

+
+Example 2: Selecting two out of the three mask, reindexing the mask ids to follow the order of the input mask ids::
+
+``mask_ids`` = [2, 0]


Do we want to allow such inversion, what is the use case?

The thing is that the mask_ids are actually indices of the bounding boxes in the batch.

Let's say we had three bounding boxes [A, B, C] with corresponding masks ids [0, 1, 2].
Now random bbox crop filtered out B and we are left with [A, B] and an output describing the indices of the bounding boxes that stayed: [0, 2].

What we want now is select the masks that refer to those bounding boxes that stayed, so we filter mask_ids [0, 2]. However, the bounding boxes now are two, so we have indices [0, 1]. To be able to map a mask to a bounding box the mask id should have the proper index, so we do this mapping: mask_id remains 0, but mask_id is now 1 because that's the index of the bounding box in the cropped image.

I understand the reason for reindexing, but I was asking about the [0, 2] case. Do we expect operators to change the order of bboxes?

I think the order doesn't matter much. I mean, there is no special reason to reorder them, it is just that we are processing them as they appear in the mask_ids input.

JanuszL · 2020-10-21T20:12:23Z

dali/operators/segmentation/select_masks.cc

+
+  const auto &in_mask_coords = ws.template InputRef<CPUBackend>(2);
+  auto in_mask_coords_shape = in_mask_coords.shape();
+  DALI_ENFORCE(in_mask_coords.type().id() == DALI_FLOAT,


Why not support ints as well here?

JanuszL · 2020-10-21T20:13:51Z

dali/operators/segmentation/select_masks.cc

+                             in_mask_meta_shape[i][1], " elements"));
+  }
+
+  int coord_ndim = in_mask_coords_shape[0][1];


What if in_mask_coords_shape[0] is zero lenght?

JanuszL · 2020-10-21T20:14:26Z

dali/operators/segmentation/select_masks.cc

+
+  const auto &in_mask_ids_view = view<const int32_t, 1>(in_mask_ids);
+  const auto &in_mask_meta_view = view<const int32_t, 2>(in_mask_meta);
+  const auto &in_mask_coords_view = view<const float, 2>(in_mask_coords);


JanuszL · 2020-10-21T20:16:42Z

dali/test/python/test_dali_cpu_only.py

+    def get_data_source(*args, **kwargs):
+        from test_segmentation_utils import make_batch_select_masks
+        return lambda: make_batch_select_masks(*args, **kwargs)
+    pipe = Pipeline(batch_size=batch_size, num_threads=4, device_id=0, seed=1234)


Suggested change

pipe = Pipeline(batch_size=batch_size, num_threads=4, device_id=0, seed=1234)

pipe = Pipeline(batch_size=batch_size, num_threads=4, device_id=None, seed=1234)

JanuszL · 2020-10-21T20:18:58Z

dali/test/python/test_dali_cpu_only.py

@@ -659,4 +659,23 @@ def test_combine_transforms_cpu():
    for _ in range(3):
        pipe.run()

+def test_segmentation_select_masks():
+    def get_data_source(*args, **kwargs):
+        from test_segmentation_utils import make_batch_select_masks


I think this can be a global import.

JanuszL · 2020-10-21T20:27:16Z

dali/operators/segmentation/select_masks.cc

+      int mask_id = meta.selected_masks[k];
+      auto it = meta.masks_meta.find(mask_id);
+      if (it == meta.masks_meta.end()) {
+        DALI_FAIL(make_string("Selected mask_id ", mask_id,


Shouldn't that FAIL be moved to L136?
I don't think that is possible here as:

for (auto mask_id : meta.selected_masks) { if (meta.masks_meta.find(mask_id) != meta.masks_meta.end()) { DALI_WARN(make_string("mask_id ", mask_id, " is duplicated. Ignoring...")); continue; } meta.masks_meta[mask_id].mask_id = mask_id; meta.masks_meta[mask_id].new_mask_id = reindex_masks_ ? idx++ : mask_id; }

So instead of range for, you have 0 to nselected for, but sill mask_ids are the same.

JanuszL · 2020-10-21T20:29:37Z

dali/test/python/test_operator_segmentation_select_masks.py

+                out_coords = out_masks_coords[out_coord_start:out_coord_end+1]
+                assert (expected_out_coords == out_coords).all()
+
+def test_select_masks():


Can you add a negative case to triger any enforce you have in your setup?

JanuszL · 2020-10-21T20:30:04Z

dali/operators/segmentation/select_masks.h

+  void RunImpl(workspace_t<CPUBackend> &ws) override;
+
+  struct MaskMeta {
+    int mask_id = -1;


I don't think that field is usefull.

JanuszL · 2020-10-21T20:30:19Z

dali/operators/segmentation/select_masks.cc

+        DALI_WARN(make_string("mask_id ", mask_id, " is duplicated. Ignoring..."));
+        continue;
+      }
+      meta.masks_meta[mask_id].mask_id = mask_id;


I don't think you need mask_id field.

mzient · 2020-10-22T10:57:35Z

dali/operators/segmentation/select_masks.cc

+        R"code(Each row describes a single mask with three values, ``mask_idx, start_idx, end_idx``,
+where ``start_idx`` and ``end_idx`` refer to indices in ``mask_coords``.)code")
+    .InputDox(2, "mask_coords", "2D TensorList of float or int",
+              R"code(Each row contains a single ``x, y`` coordinate.)code")


Suggested change

R"code(Each row contains a single ``x, y`` coordinate.)code")

R"code(Each row contains a single set of coordinates, kike ``x, y`` or ``x, y, z``.code")

...and, of course, we should support more than 2D points.

mzient · 2020-10-22T11:00:30Z

dali/operators/segmentation/select_masks.cc

+        R"code(If set to True, new mask ids are reassigned to each of the selected mask, so that they
+follow zero-based index of the mask ids in the input.)code",


Suggested change

R"code(If set to True, new mask ids are reassigned to each of the selected mask, so that they

follow zero-based index of the mask ids in the input.)code",

R"(If set to True, the output mask ids are replaced with the indices at which they appeared in ``mask_ids`` input.)",

mzient · 2020-10-22T11:03:58Z

dali/operators/segmentation/select_masks.cc

+  for (int i = 0; i < nsamples; i++) {
+    auto sh = in_mask_meta_shape.tensor_shape_span(i);
+    DALI_ENFORCE(3 == sh[1],
+                 make_string("``mask_meta`` is expected to contain 3 element rows, containing "


Suggested change

make_string("``mask_meta`` is expected to contain 3 element rows, containing "

make_string("``mask_meta`` is expected to contain 2D tensors with 3 columns: "

mzient · 2020-10-22T11:05:19Z

dali/operators/segmentation/select_masks.cc

+      DALI_ENFORCE(
+        mask.start_coord >= 0 && mask.end_coord < input_total_ncoords,
+        make_string(
+            "Coordinate index range for mask id ", mask_id, " [", mask.start_coord, ", ",


Suggested change

"Coordinate index range for mask id ", mask_id, " [", mask.start_coord, ", ",

"Vertex index range for mask id ", mask_id, " [", mask.start_coord, ", ",

mzient · 2020-10-22T12:42:08Z

dali/operators/segmentation/select_masks.cc

+              R"code(Vertex data, of arbitrary dimensionality
+(the number of dimensions should be the same for every vertex).)code")


"...data of arbitrary dimensionality..." sounds a bit ambiguous - the vertices or the tensor itself?

There's no way to supply vertices of different dimensionality within one sample, because it would not be a tensor

Suggested change

R"code(Vertex data, of arbitrary dimensionality

(the number of dimensions should be the same for every vertex).)code")

R"code(Vertex data stored in interleaved format::

[[x0, y0], [x1, y1], ... , [xn, yn]]

The operator accepts vertices with arbitrary number of coordinates.)code")

mzient · 2020-10-22T15:41:39Z

dali/operators/segmentation/select_masks.cc

+  for (int i = 1; i < nsamples; i++) {
+    auto sh = in_vertices_shape.tensor_shape_span(i);
+    DALI_ENFORCE(vertex_ndim == sh[1],
+      make_string("All vertices are expected to have the same dimensionality. Got ",
+                  vertex_ndim, "D and ", sh[1], "D in the same batch"));
+  }


Why do we care? It seems to us that it doesn't make sense, but it is an artificial constraint and someone might want to abuse this argument for some purposes and we shouldn't disallow it.

JanuszL · 2020-10-23T14:38:12Z

dali/operators/segmentation/select_masks.cc

+    .NumOutput(2)
+    .InputDox(0, "mask_ids", "1D TensorList of int",
+              R"code(List of identifiers of the masks to be selected.)code")
+    .InputDox(1, "polygons", "2D TensorList of int",


I started wondering if we should extend this to 3D as well.

3D is supported. Here 2D refers to the shape of the data:

[[x0, y0, (z0, ...)], ..., [xN, yN, (zN, ...)]

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

jantonguirao · 2020-10-30T06:53:24Z

!build

dali-automaton · 2020-10-30T07:03:38Z

CI MESSAGE: [1748057]: BUILD STARTED

dali-automaton · 2020-10-30T08:49:13Z

CI MESSAGE: [1748057]: BUILD PASSED

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

mzient · 2020-10-30T14:10:14Z

dali/test/python/test_segmentation_utils.py

@@ -0,0 +1,52 @@
+# Copyright (c) 2020, NVIDIA CORPORATION. All rights reserved.


Nitpick:
The filename sounds like it's testing segmentation utils. If these are utilities for testing segmentation, then the correct name would be segmentation_test_utils.py.

mzient · 2020-10-30T14:11:47Z

dali/operators/segmentation/select_masks.cc

+      if (polygons.find(mask_id) != polygons.end()) {
+        DALI_WARN(make_string("mask_id ", mask_id, " is duplicated. Ignoring..."));
+        continue;
+      }


Why? I bet this will bite us.

Disallowed duplicates completely now

mzient · 2020-10-30T15:13:00Z

dali/operators/segmentation/select_masks.cc

+      poly.end_vertex = poly_data[2];
+
+      DALI_ENFORCE(
+        poly.start_vertex >= 0 && poly.end_vertex <= in_nvertices,


Missing check for poly.start_vertex <= poly.end_vertex.

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

mzient · 2020-10-30T15:34:11Z

dali/test/python/test_pipeline_segmentation.py

+        bbox_x = fn.slice(in_bboxes, 0, 1, axes=[1])
+        bbox_y = fn.slice(in_bboxes, 1, 1, axes=[1])
+        bbox_X = fn.slice(in_bboxes, 2, 1, axes=[1])
+        bbox_Y = fn.slice(in_bboxes, 3, 1, axes=[1])
+        in_bboxes_abs = fn.cat(bbox_x * w, bbox_y * h, bbox_X * w, bbox_Y * h, axis=1)


Suggested change

bbox_x = fn.slice(in_bboxes, 0, 1, axes=[1])

bbox_y = fn.slice(in_bboxes, 1, 1, axes=[1])

bbox_X = fn.slice(in_bboxes, 2, 1, axes=[1])

bbox_Y = fn.slice(in_bboxes, 3, 1, axes=[1])

in_bboxes_abs = fn.cat(bbox_x * w, bbox_y * h, bbox_X * w, bbox_Y * h, axis=1)

to_abs = fn.transforms.scale(scale=fn.cat(w, h, w, h, axis=0))

in_bboxes_abs = fn.coord_transform(in_bboxes, MT=to_abs)

Despite the matrix-vector multiplication it should be cheaper just because there are much fewer operators.

as we agreed offline, it's more straight forward the way it is now

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

jantonguirao · 2020-10-30T16:43:38Z

!build

dali-automaton · 2020-10-30T16:48:25Z

CI MESSAGE: [1749311]: BUILD STARTED

dali-automaton · 2020-10-30T18:25:34Z

CI MESSAGE: [1749311]: BUILD FAILED

dali-automaton · 2020-10-30T19:04:39Z

CI MESSAGE: [1749311]: BUILD PASSED

jantonguirao force-pushed the select_masks_op branch from 618cd67 to 73e0970 Compare October 21, 2020 15:21

jantonguirao changed the title ~~[WIP][DO NOT REVIEW YET] SelectMasks operator~~ Add SelectMasks operator Oct 21, 2020

jantonguirao requested review from mzient and a team October 21, 2020 15:48

JanuszL reviewed Oct 21, 2020

View reviewed changes

jantonguirao force-pushed the select_masks_op branch 3 times, most recently from 156589b to 322c3e9 Compare October 22, 2020 09:45

mzient reviewed Oct 22, 2020

View reviewed changes

jantonguirao force-pushed the select_masks_op branch 7 times, most recently from 0660071 to 9cece8a Compare October 22, 2020 12:49

mzient reviewed Oct 22, 2020

View reviewed changes

jantonguirao force-pushed the select_masks_op branch from 507aa3f to 25ad5fb Compare October 23, 2020 07:34

JanuszL reviewed Oct 23, 2020

View reviewed changes

JanuszL approved these changes Oct 23, 2020

View reviewed changes

jantonguirao force-pushed the select_masks_op branch from 25ad5fb to 6b841ad Compare October 26, 2020 10:33

jantonguirao added 6 commits October 30, 2020 07:52

Add SelectMasks operator (CPU)

2cdee9a

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

no cpu test

d574386

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

Code review fixes

d7766ac

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

Renaming masks to polygons and coords to vertices

8ac07fe

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

[start_vertex, end_vertex] to [start_vertex, end_vertex) semantics

485fe61

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

More code review fixes

93dbf3c

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

jantonguirao force-pushed the select_masks_op branch from 6b841ad to 93dbf3c Compare October 30, 2020 06:52

Add reinterpret_view<>

93bf6a7

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

mzient reviewed Oct 30, 2020

View reviewed changes

jantonguirao added 2 commits October 30, 2020 16:20

Add segmentation tests

ae84bf8

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

Code review fixes

50d9b8c

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

mzient reviewed Oct 30, 2020

View reviewed changes

mzient approved these changes Oct 30, 2020

View reviewed changes

Code review fixes

9e6d1ad

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

jantonguirao force-pushed the select_masks_op branch from a5d78be to 9e6d1ad Compare October 30, 2020 16:42

jantonguirao merged commit 65f6def into NVIDIA:master Oct 30, 2020


		Example 2: Selecting two out of the three mask, reindexing the mask ids to follow the order of the input mask ids::

		``mask_ids`` = [2, 0]

	pipe = Pipeline(batch_size=batch_size, num_threads=4, device_id=0, seed=1234)
	pipe = Pipeline(batch_size=batch_size, num_threads=4, device_id=None, seed=1234)

	R"code(Each row contains a single ``x, y`` coordinate.)code")
	R"code(Each row contains a single set of coordinates, kike ``x, y`` or ``x, y, z``.code")

		R"code(If set to True, new mask ids are reassigned to each of the selected mask, so that they
		follow zero-based index of the mask ids in the input.)code",

	R"code(If set to True, new mask ids are reassigned to each of the selected mask, so that they
	follow zero-based index of the mask ids in the input.)code",
	R"(If set to True, the output mask ids are replaced with the indices at which they appeared in ``mask_ids`` input.)",

	make_string("``mask_meta`` is expected to contain 3 element rows, containing "
	make_string("``mask_meta`` is expected to contain 2D tensors with 3 columns: "

	"Coordinate index range for mask id ", mask_id, " [", mask.start_coord, ", ",
	"Vertex index range for mask id ", mask_id, " [", mask.start_coord, ", ",

		R"code(Vertex data, of arbitrary dimensionality
		(the number of dimensions should be the same for every vertex).)code")

-              R"code(Vertex data, of arbitrary dimensionality
-(the number of dimensions should be the same for every vertex).)code")
+              R"code(Vertex data stored in interleaved format::
+              [[x0, y0], [x1, y1], ... , [xn, yn]]
+The operator accepts vertices with arbitrary number of coordinates.)code")

		@@ -0,0 +1,52 @@
		# Copyright (c) 2020, NVIDIA CORPORATION. All rights reserved.

Add SelectMasks operator #2381

Add SelectMasks operator #2381

Conversation

jantonguirao commented Oct 20, 2020 • edited Loading

Why we need this PR?

What happened in this PR?

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

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

mzient Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jantonguirao commented Oct 30, 2020

dali-automaton commented Oct 30, 2020

dali-automaton commented Oct 30, 2020

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

mzient Oct 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jantonguirao commented Oct 30, 2020

dali-automaton commented Oct 30, 2020

dali-automaton commented Oct 30, 2020

dali-automaton commented Oct 30, 2020

jantonguirao commented Oct 20, 2020 •

edited

Loading

mzient Oct 22, 2020 •

edited

Loading

mzient Oct 22, 2020 •

edited

Loading

mzient Oct 30, 2020 •

edited

Loading