Add CoordFlip CPU operator #1894

jantonguirao · 2020-04-23T10:32:28Z

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

Why we need this PR?

It adds new feature, Coordinate flip, needed to complete MaskRCNN pipeline

What happened in this PR?

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

What solution was applied:
Added Coordinate Flip CPU operator
Affected modules and functionalities:
New operator
Key points relevant for the review:
The operator implementation
Validation and testing:
Python operator tests added
Documentation (including examples):
NA

JIRA TASK: [DALI-1392]

JanuszL · 2020-04-23T11:39:42Z

dali/operators/coord/coord_flip.cc

+          int64_t i = 0;
+          for (; i < in_size; i++, d++) {
+            if (d == ndim_) d = 0;
+            assert(in[i] >= 0.0f && in[i] <= 1.0f);


Maybe enforce, we can get this data from the ExternalSource, so it is a user error.

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

szalpal · 2020-04-24T09:40:09Z

dali/operators/coord/coord_flip.cc

+    .AddOptionalArg("horizontal", R"code(Perform flip along horizontal axis.)code", 1, true)
+    .AddOptionalArg("vertical", R"code(Perform flip along vertical axis.)code", 0, true)
+    .AddOptionalArg("depthwise", R"code(Perform flip along depthwise axis.)code", 0, true);


I think, that image flip is (unfortunately) defined the opposite. horizontal denotes flip along vertical axis. Maybe we should unify it?

Yes, at least per docs: horizontal (int, optional, default = 1) – Perform a horizontal flip.

On the other hand BbFlip does the same as this operator, we're already not consistent.

Dayum :( Let's discuss what to do with it

szalpal · 2020-04-24T09:42:11Z

dali/operators/coord/coord_flip.cc

+    bool horizontal_flip = spec_.GetArgument<int>("horizontal", &ws, sample_id);
+    bool vertical_flip = spec_.GetArgument<int>("vertical", &ws, sample_id);
+    bool depthwise_flip = spec_.GetArgument<int>("depthwise", &ws, sample_id);
+    std::array<bool, 3> flip_dim = {false, false, false};


std::array has no specialization for bool. Maybe we'd be better with std::vector<bool> here?

vector of bool should be killed with fire and purged from existence.

I just want 3 bools, no need for dynamic allocation

szalpal · 2020-04-24T09:47:20Z

dali/operators/coord/coord_flip.cc

+
+DALI_SCHEMA(CoordFlip)
+    .DocStr(
+        R"code(Transforms normalized coordinates (range [0.0, 1.0]) so that they map to the same place after


How about adding also a version for not normalized coordinates? We have tensor shape, so simple switch in API makes do

done with the new patch

szalpal · 2020-04-24T09:54:15Z

dali/operators/coord/coord_flip.h

+
+    DALI_ENFORCE(in_shape[0].size() == 2);
+    ndim_ = in_shape[0][1];
+    DALI_ENFORCE(ndim_ >= 1 && ndim_ <= 3, make_string("Unexpected number of dimensions ", ndim_));


It could work for 0-dim, right? Just return 1-input

what would be the meaning of a 0D coordinate?

This will never be 0. Hovever, it's going to be possible to have in.sample_dim() to be 0 - scalar 1D coordinate (not a vector) - in which case I think we should just assume it's X.

szalpal · 2020-04-24T09:55:18Z

dali/operators/coord/coord_flip.cc

+          int d = 0;
+          int64_t i = 0;
+          for (; i < in_size; i++, d++) {


That's a nitpick, but I'd appreciate a little bit more descriptive names ;) Like dim_idx and coor_idx

klecki · 2020-04-24T13:53:45Z

dali/operators/coord/coord_flip.cc

+  Possible values are:
+
+  ``x`` (horizontal position), ``y`` (vertical position), ``z`` (depthwise position),
+
+Note: If left empty, ``"xy"`` or ``"xyz"`` will be assumed, depending on the number of dimensions.


I'm afraid that this won't look very well in the docs (but we will need to check it.
How about using bullets here?

klecki · 2020-04-24T14:59:15Z

dali/operators/coord/coord_flip.cc

+
+void CoordFlipCPU::RunImpl(workspace_t<CPUBackend> &ws) {
+  const auto &input = ws.InputRef<CPUBackend>(0);
+  DALI_ENFORCE(input.type().id() == DALI_FLOAT, "Input is expected to be float");


Can you maybe move this enforce to Setup? If we decide to have some type/shape inference, if the input data is wrong (for example u8), we will report that we produce u8 and later report error during Run.

klecki · 2020-04-24T15:00:12Z

dali/operators/coord/coord_flip.cc

+  auto &thread_pool = ws.GetThreadPool();
+
+  if (layout_.empty()) {
+    layout_ = ndim_ == 2 ? "xy" : "xyz";


If the ndim_ is 1 you end up with wrong layout.

dali/operators/coord/coord_flip.cc

klecki · 2020-04-24T15:09:18Z

dali/operators/coord/coord_flip.cc

+            DALI_ENFORCE(in_val >= 0.0f && in_val <= 1.0f,
+              "Input expected to be within the range [0.0, 1.0]");


I know we already had this discussion, but isn't this a bit over-defensive? You're probably not reporting such errors from the GPU Op. As you are flipping along x=0.5 here (or whatever the axis), it will work regardless of the range.

klecki · 2020-04-24T15:18:34Z

dali/test/python/test_operator_coord_flip.py

+def test_operator_coord_flip():
+    for device in ['cpu']:
+        for batch_size in [1, 3]:
+            for layout, shape in [("xy", (10, 2)), ("xyz", (10, 3))]:
+                yield check_operator_coord_flip, device, batch_size, layout, shape


Can you also check 1 dim as it's supported.

I believe 0-dim also should be supported

klecki

Please verify scalar arguments and 1-dim case.

I also think that the implementation is a bit overprotective - I didn't spot Janusz's comment. I still think last time we decided on Garbage in - garbage out approach. Especially that it won't be exactly garbage.

JanuszL · 2020-04-24T21:09:34Z

I also think that the implementation is a bit overprotective - I didn't spot Janusz's comment.

I focused on how to do it, not if it is justified. You are right. Maybe we should not put such restrictions. We had some discussion with @mzient that bboxes or polygons may span beyond the image and some networks may still produce a valid result - if you see half a car in the image you can still tell where it ends.

klecki · 2020-04-27T08:47:32Z

I also think that the implementation is a bit overprotective - I didn't spot Janusz's comment.

I focused on how to do it, not if it is justified. You are right. Maybe we should not put such restrictions. We had some discussion with @mzient that bboxes or polygons may span beyond the image and some networks may still produce a valid result - if you see half a car in the image you can still tell where it ends.

Especially that it still would flip them correctly.

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

mzient · 2020-04-27T15:43:14Z

dali/operators/coord/coord_flip.cc

+Note: If left empty, ``"x"``, ``"xy"`` or ``"xyz"`` will be assumed, depending on the number of dimensions.
+)code",
+      TensorLayout{""})
+    .AddOptionalArg("horizontal", R"code(Flip horizontal dimension.)code", 1, true)


maybe they should be called flip_x, flip_y, flip_z ? Just a thought/question. Current ones are compatible with other operators, but since the order of coordinates is different here (XYZ vs (Z)YX as in (D)HWC layout), this compatibility may cause more problems than it solves.

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

JanuszL · 2020-04-27T16:52:04Z

dali/test/python/test_operator_coord_flip.py

+                    expected_out_coords[:, d] = 1.0 - in_coords[:, d]
+            np.testing.assert_allclose(out_coords[:, d], expected_out_coords[:, d])
+
+def test_operator_coord_flip():


How about testing center_* argument as well?

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

klecki · 2020-04-28T09:26:35Z

dali/operators/coord/coord_flip.cc

+    flip_dim[y_dim] = spec_.GetArgument<int>("flip_y", &ws, sample_id);
+    flip_dim[z_dim] = spec_.GetArgument<int>("flip_z", &ws, sample_id);


y_dim and z_dim can be -1 if they are not in layout (1D and 2D case). Same below.

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

jantonguirao · 2020-04-28T10:16:39Z

!build

dali-automaton · 2020-04-28T10:20:46Z

CI MESSAGE: [1285277]: BUILD STARTED

dali-automaton · 2020-04-28T15:36:29Z

CI MESSAGE: [1285277]: BUILD PASSED

JanuszL reviewed Apr 23, 2020

View reviewed changes

jantonguirao added 2 commits April 23, 2020 15:27

Add CoordFlip CPU operator

035bd81

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

Code review fixes

8fbd477

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

jantonguirao force-pushed the coord_flip branch from 626041b to 8fbd477 Compare April 24, 2020 07:23

JanuszL approved these changes Apr 24, 2020

View reviewed changes

szalpal reviewed Apr 24, 2020

View reviewed changes

klecki reviewed Apr 24, 2020

View reviewed changes

dali/operators/coord/coord_flip.cc Outdated Show resolved Hide resolved

klecki reviewed Apr 24, 2020

View reviewed changes

klecki requested changes Apr 24, 2020

View reviewed changes

Add flip center argument

c2482b3

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

jantonguirao force-pushed the coord_flip branch 2 times, most recently from a03bac2 to 9403852 Compare April 27, 2020 15:34

mzient reviewed Apr 27, 2020

View reviewed changes

jantonguirao force-pushed the coord_flip branch from 9403852 to e49eb99 Compare April 27, 2020 16:14

Code review fixes

4907582

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

jantonguirao force-pushed the coord_flip branch from e49eb99 to 4907582 Compare April 27, 2020 16:33

JanuszL reviewed Apr 27, 2020

View reviewed changes

Add tests for custom coord flip center

5347fc0

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

jantonguirao requested a review from klecki April 28, 2020 08:45

JanuszL self-requested a review April 28, 2020 09:04

JanuszL approved these changes Apr 28, 2020

View reviewed changes

klecki reviewed Apr 28, 2020

View reviewed changes

fixes

70de828

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

klecki approved these changes Apr 28, 2020

View reviewed changes

mzient approved these changes Apr 28, 2020

View reviewed changes

jantonguirao merged commit 4c1b3b3 into NVIDIA:master Apr 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CoordFlip CPU operator #1894

Add CoordFlip CPU operator #1894

jantonguirao commented Apr 23, 2020

JanuszL Apr 23, 2020

szalpal Apr 24, 2020

klecki Apr 24, 2020

szalpal Apr 24, 2020

szalpal Apr 24, 2020

klecki Apr 24, 2020

jantonguirao Apr 27, 2020

szalpal Apr 24, 2020

jantonguirao Apr 27, 2020

szalpal Apr 24, 2020

jantonguirao Apr 27, 2020

mzient Apr 27, 2020

szalpal Apr 24, 2020

klecki Apr 24, 2020

klecki Apr 24, 2020

jantonguirao Apr 27, 2020

klecki Apr 24, 2020

jantonguirao Apr 27, 2020

klecki Apr 24, 2020

jantonguirao Apr 28, 2020

klecki Apr 24, 2020

szalpal Apr 27, 2020

klecki left a comment •

edited

Loading

JanuszL commented Apr 24, 2020

klecki commented Apr 27, 2020

mzient Apr 27, 2020

jantonguirao Apr 28, 2020

JanuszL Apr 27, 2020

jantonguirao Apr 28, 2020

klecki Apr 28, 2020

jantonguirao commented Apr 28, 2020

dali-automaton commented Apr 28, 2020

dali-automaton commented Apr 28, 2020

		DALI_ENFORCE(in_val >= 0.0f && in_val <= 1.0f,
		"Input expected to be within the range [0.0, 1.0]");

		flip_dim[y_dim] = spec_.GetArgument<int>("flip_y", &ws, sample_id);
		flip_dim[z_dim] = spec_.GetArgument<int>("flip_z", &ws, sample_id);

Add CoordFlip CPU operator #1894

Add CoordFlip CPU operator #1894

Conversation

jantonguirao commented Apr 23, 2020

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

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

klecki left a comment • edited Loading

Choose a reason for hiding this comment

JanuszL commented Apr 24, 2020

klecki commented Apr 27, 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

jantonguirao commented Apr 28, 2020

dali-automaton commented Apr 28, 2020

dali-automaton commented Apr 28, 2020

klecki left a comment •

edited

Loading