Erase CPU operator #1609

jantonguirao · 2019-12-24T10:04:57Z

Why we need this PR?

Pick one, remove the rest

It adds a new feature: Erase CPU operator, allowing to erase regions of the input, assigning a constant value

What happened in this PR?

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

What solution was applied:
Created an Erase CPU kernel
Created an Erase CPU operator
The Erase operator API allows the user to specify one or more regions with an anchor and a shape. The coordinates in both anchor and shape are typically a subset of the input dimensions. The user can specify the dimensions to be used.
Example:
layout="HWC" axis_names="WH" anchor=(10, 20) shape=(20, 30)
specifies a region going from x=10 to x=30 and from y=20 to y=50
Affected modules and functionalities:
Erase CPU kernel and operator
Key points relevant for the review:
The kernel and operator implementation
Validation and testing:
Python tests for the operator and C++ tests for the kernel
Documentation (including examples):
Doc string contains detailed usage description, including example
Jupyter notebook example

JIRA TASK: [DALI-1206]

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

klecki · 2020-01-02T15:07:46Z

!build

dali-automaton · 2020-01-02T15:10:18Z

CI MESSAGE: [1054794]: BUILD STARTED

dali-automaton · 2020-01-02T15:12:08Z

CI MESSAGE: [1054794]: BUILD FAILED

klecki

The kernel and operator look ok, small problem in argument handling, most issues with docs and argument handling - I'm ok with merging this and later reworking.

As for the example it would be good to repeat and explain the layout of multiple ROIs and maybe show how they are treated when they are out of bounds.

There seem to be empty cells at the end as well.

klecki · 2020-01-02T12:05:15Z

dali/operators/erase/CMakeLists.txt

@@ -0,0 +1,18 @@
+# Copyright (c) 2017-2018, NVIDIA CORPORATION. All rights reserved.


Soon to be 2020.

Suggested change

# Copyright (c) 2017-2018, NVIDIA CORPORATION. All rights reserved.

# Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved.

klecki · 2020-01-02T12:34:51Z

dali/operators/erase/erase.cc

+  The erase region covers the range from 10 to 200 in the vertical dimension (heigth) and goes from
+ 20 to 220 in the horizontal dimension (width). The range for the channel dimension goes from 0 to 3, as it
+ was not specified. That is
+ output[y, x, c] = 0               if 20 <= x < 220 and 10 <= y < 200


That formatting probably is going to be off, maybe use the :: for preformatted block?
Also below.

klecki · 2020-01-02T13:07:34Z

dali/operators/erase/erase_utils.h

+  if (spec.HasArgument("axis_names")) {
+    for (auto axis_name : spec.GetArgument<TensorLayout>("axis_names")) {
+      int d = layout.find(axis_name);
+      DALI_ENFORCE(d >= 0);


I would put a verbose message here, like:

Suggested change

DALI_ENFORCE(d >= 0);

DALI_ENFORCE(d >= 0, make_string("Axis ', axis_name, "' is not present in the input layout.");

klecki · 2020-01-02T13:15:44Z

dali/operators/erase/erase_utils.h

+
+namespace detail {
+
+SmallVector<int, 3> GetAxes(const OpSpec &spec, TensorLayout layout) {


BTW, I think at this point it would be good to factor out the handling of axes, axis_names, anchor and shape as they are shared with Slice (and related Operators) and the code should probably be reused - we will have unified behaviour for that instead of reimplementing it every time.

This one here gets a slightly different behaviour as you can specify multiple anchors, but that probably is doable with limiting it to 1 anchor for slice.

I'm fine with doing this as a follow-up.

I agree, let's do that in a follow-up PR

klecki · 2020-01-02T13:17:23Z

dali/operators/erase/erase_utils.h

+  } else if (spec.HasArgument("axes")) {
+    axes = spec.GetRepeatedArgument<int>("axes");
+  } else {
+    // no axes info, expecting all dimensions except 'C'


Either I missed it or there is no description of this behaviour in the docstring.

It's there in the description of "fill_value":

.AddOptionalArg("fill_value", R"code(Value to fill the erased region. Might be specified as a single value (e.g. 0) or a multi-channel value (e.g. (200, 210, 220)). If a multi-channel fill value is provided, the input layout should contain a channel dimension `C`)code", std::vector<float>{0, })

But still it doesn't say that the axes can be left unspecified and than you expect shapes for everything except 'C'

You are right, I'll fix that

klecki · 2020-01-02T13:22:50Z

dali/operators/erase/erase_utils.h

+  auto norm_anchor = spec.template GetArgument<bool>("normalized_anchor");
+
+  std::vector<float> roi_shape;
+  bool has_tensor_roi_shape = spec.HasTensorArgument("anchor");


Copy-paste mistake?

Suggested change

bool has_tensor_roi_shape = spec.HasTensorArgument("anchor");

bool has_tensor_roi_shape = spec.HasTensorArgument("shape");

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

jantonguirao · 2020-01-02T15:59:48Z

!build

dali-automaton · 2020-01-02T16:05:07Z

CI MESSAGE: [1054835]: BUILD STARTED

klecki · 2020-01-02T16:25:52Z

dali/operators/erase/erase.cc

@@ -70,7 +73,7 @@ Example 3:
    vector<float>(), true)
  .AddOptionalArg("axes",
    R"code(Order of dimensions used for anchor and shape arguments, as dimension indexes. For instance, `axes=(1, 0)`
- means the coordinates in `anchor` and `shape` refer to axes 1 and 0, in that particular order)code",
+ means the coordinates in `anchor` and `shape` refer to axes 1 and 0,1054794 in that particular order)code",


??

Suggested change

means the coordinates in `anchor` and `shape` refer to axes 1 and 0,1054794 in that particular order)code",

means the coordinates in `anchor` and `shape` refer to axes 1 and 0, in that particular order)code",

dali-automaton · 2020-01-02T16:48:10Z

CI MESSAGE: [1054835]: BUILD PASSED

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

jantonguirao · 2020-01-02T17:07:25Z

!build

dali-automaton · 2020-01-03T15:20:09Z

CI MESSAGE: [1056239]: BUILD STARTED

dali-automaton · 2020-01-03T16:09:01Z

CI MESSAGE: [1056239]: BUILD PASSED

JanuszL

erase.ipynb is not included in the docs.

dali/operators/erase/erase_utils.h

dali/kernels/erase/erase_cpu.h

JanuszL · 2020-01-07T15:44:02Z

dali/kernels/erase/erase_cpu_test.cc

+}
+
+INSTANTIATE_TEST_SUITE_P(EraseCpuTest, EraseCpuTest, testing::Combine(
+    testing::Values(std::array<int64_t, 3>{32, 15, 3}),  // data shape


Can you add a test case of 1C image as well?

JanuszL · 2020-01-07T15:49:39Z

dali/operators/erase/erase.cc

+
+Example 3:
+
+anchor=(0.15, 0.15), shape=(0.3, 0.3), axis_names="HW", fill_value=(100,), normalized_anchor=True, normalized_shape=True


Suggested change

anchor=(0.15, 0.15), shape=(0.3, 0.3), axis_names="HW", fill_value=(100,), normalized_anchor=True, normalized_shape=True

anchor=(0.15, 0.15), shape=(0.3, 0.3), axis_names="HW", fill_value=100, normalized_anchor=True, normalized_shape=True

For consistence?

dali/test/python/test_operator_erase.py

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

jantonguirao · 2020-01-08T13:21:04Z

!build

dali-automaton · 2020-01-08T13:25:25Z

CI MESSAGE: [1062421]: BUILD STARTED

dali-automaton · 2020-01-08T13:28:52Z

CI MESSAGE: [1062421]: BUILD FAILED

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

jantonguirao · 2020-01-08T13:36:52Z

!build

dali-automaton · 2020-01-08T13:40:09Z

CI MESSAGE: [1062437]: BUILD STARTED

dali-automaton · 2020-01-08T16:24:20Z

CI MESSAGE: [1062437]: BUILD FAILED

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

jantonguirao · 2020-01-08T18:17:30Z

!build

dali-automaton · 2020-01-08T18:20:40Z

CI MESSAGE: [1062776]: BUILD STARTED

dali-automaton · 2020-01-08T20:12:36Z

CI MESSAGE: [1062776]: BUILD PASSED

dali/kernels/erase/erase_cpu.h

jantonguirao force-pushed the erase_op branch 3 times, most recently from 2b6ebab to 3626e8b Compare December 27, 2019 11:39

jantonguirao added 6 commits December 30, 2019 11:03

Erase operator skeleton

00ef028

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

[WIP] Erase kernel

555440a

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

Multi-channel fill value

3157e01

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

Erase Op. Some python tests passing

296cd78

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

Extract utils

292fc84

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

Update docstr

bea6874

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

jantonguirao force-pushed the erase_op branch from 9b2745b to fc4c0af Compare December 30, 2019 11:05

Add more test cases

04760b4

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

jantonguirao force-pushed the erase_op branch from fc4c0af to 04760b4 Compare December 30, 2019 11:53

jantonguirao changed the title ~~[WIP] Erase CPU operator~~ Erase CPU operator Dec 30, 2019

jantonguirao requested a review from a team December 30, 2019 12:42

jantonguirao added 3 commits December 30, 2019 14:20

Support other types

dc49cf3

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

Allow normalized coordinates

e645f0f

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

Enable tensor inputs

b53d3ef

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

klecki requested changes Jan 2, 2020

View reviewed changes

Apply code review fixes

bb66120

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

klecki reviewed Jan 2, 2020

View reviewed changes

Update examples

bc9e024

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

jantonguirao force-pushed the erase_op branch from 851702e to bc9e024 Compare January 2, 2020 16:55

jantonguirao requested a review from a team January 2, 2020 17:02

klecki approved these changes Jan 4, 2020

View reviewed changes

jantonguirao requested a review from a team January 7, 2020 14:32

JanuszL requested changes Jan 7, 2020

View reviewed changes

JanuszL reviewed Jan 7, 2020

View reviewed changes

dali/operators/erase/erase_utils.h Show resolved Hide resolved

JanuszL reviewed Jan 7, 2020

View reviewed changes

dali/kernels/erase/erase_cpu.h Show resolved Hide resolved

JanuszL reviewed Jan 7, 2020

View reviewed changes

dali/test/python/test_operator_erase.py Show resolved Hide resolved

jantonguirao force-pushed the erase_op branch from 102202b to 2fdb1d6 Compare January 8, 2020 10:52

Code review fixes

91dfcfc

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

jantonguirao force-pushed the erase_op branch from 2fdb1d6 to 91dfcfc Compare January 8, 2020 11:01

jantonguirao added 2 commits January 8, 2020 14:00

Support shape in Uniform operator

7cee12d

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

Support for centered anchor

f4b3401

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

Fix lint

c3da203

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

jantonguirao requested a review from JanuszL January 8, 2020 14:31

Fix Uniform

f0cf946

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

JanuszL reviewed Jan 9, 2020

View reviewed changes

dali/kernels/erase/erase_cpu.h Show resolved Hide resolved

JanuszL approved these changes Jan 9, 2020

View reviewed changes

jantonguirao merged commit a524dd9 into NVIDIA:master Jan 9, 2020

		@@ -0,0 +1,18 @@
		# Copyright (c) 2017-2018, NVIDIA CORPORATION. All rights reserved.

	# Copyright (c) 2017-2018, NVIDIA CORPORATION. All rights reserved.
	# Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved.

	DALI_ENFORCE(d >= 0);
	DALI_ENFORCE(d >= 0, make_string("Axis ', axis_name, "' is not present in the input layout.");


		namespace detail {

		SmallVector<int, 3> GetAxes(const OpSpec &spec, TensorLayout layout) {

	bool has_tensor_roi_shape = spec.HasTensorArgument("anchor");
	bool has_tensor_roi_shape = spec.HasTensorArgument("shape");

	means the coordinates in `anchor` and `shape` refer to axes 1 and 0,1054794 in that particular order)code",
	means the coordinates in `anchor` and `shape` refer to axes 1 and 0, in that particular order)code",


		Example 3:

		anchor=(0.15, 0.15), shape=(0.3, 0.3), axis_names="HW", fill_value=(100,), normalized_anchor=True, normalized_shape=True

Erase CPU operator #1609

Erase CPU operator #1609

Conversation

jantonguirao commented Dec 24, 2019 • edited Loading

Why we need this PR?

What happened in this PR?

klecki commented Jan 2, 2020

dali-automaton commented Jan 2, 2020

dali-automaton commented Jan 2, 2020

klecki left a 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

jantonguirao commented Jan 2, 2020

dali-automaton commented Jan 2, 2020

klecki Jan 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dali-automaton commented Jan 2, 2020

jantonguirao commented Jan 2, 2020

dali-automaton commented Jan 3, 2020

dali-automaton commented Jan 3, 2020

JanuszL left a 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 Jan 8, 2020

dali-automaton commented Jan 8, 2020

dali-automaton commented Jan 8, 2020

jantonguirao commented Jan 8, 2020

dali-automaton commented Jan 8, 2020

dali-automaton commented Jan 8, 2020

jantonguirao commented Jan 8, 2020

dali-automaton commented Jan 8, 2020

dali-automaton commented Jan 8, 2020

jantonguirao commented Dec 24, 2019 •

edited

Loading

klecki Jan 2, 2020 •

edited

Loading