Enable named Slice arguments: start/rel_start, end/rel_end, shape/rel_shape #2625

jantonguirao · 2021-01-21T18:21:14Z

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

Why we need this PR?

Refactoring to improve usability of Slice operator

What happened in this PR?

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

What solution was applied:
Introduced named arguments start, rel_start, end, rel_end, shape, rel_shape to define the slice parameters instead of the regular (positional) inputs anchor and shape
Affected modules and functionalities:
Slice, ImageDecoderSlice
Key points relevant for the review:
SliceAttr implementation, error checking
Validation and testing:
Tests added
Documentation (including examples):
Operator doc updated

JIRA TASK: [DALI-1544]

jantonguirao · 2021-01-22T11:46:52Z

!build

dali-automaton · 2021-01-22T11:50:58Z

CI MESSAGE: [2005916]: BUILD STARTED

dali-automaton · 2021-01-22T13:29:46Z

CI MESSAGE: [2005916]: BUILD PASSED

jantonguirao · 2021-01-22T14:15:11Z

dali/operators/decoder/image_decoder.cc

mzient · 2021-01-25T11:26:20Z

dali/operators/generic/slice/slice.cc

+Relative and absolute arguments can be used for different arguments (e.g. ``rel_start``
+and ``shape``) but should not be specified for the same argument (e.g. ``start`` and ``rel_start``
+should not be provided together).


Suggested change

Relative and absolute arguments can be used for different arguments (e.g. ``rel_start``

and ``shape``) but should not be specified for the same argument (e.g. ``start`` and ``rel_start``

should not be provided together).

Relative and absolute arguments can be mixed (for example, ``rel_start`` can be used with ``shape``) as long

as start and shape or end are uniquely defined.

mzient · 2021-01-27T09:45:47Z

dali/operators/generic/slice/slice_attr.h

+    if (has_end_ || has_shape_) {  // ``start`` can be omitted
+      DALI_ENFORCE(!spec_.HasArgument("normalized_anchor")
+                && !spec_.HasArgument("normalized_shape"),
+        "``normalized_anchor``/``normalized_shape`` is only relevant when using positional "


The fact that they are irrelevant doesn't mean they are forbidden (could be ignored, for example).

Suggested change

"``normalized_anchor``/``normalized_shape`` is only relevant when using positional "

"``normalized_anchor`` and ``normalized_shape`` can be used only with positional "

mzient · 2021-01-27T09:47:33Z

dali/operators/generic/slice/slice_attr.h

+    else if (rel_shape_.IsDefined())
+      rel_shape_.Acquire(spec_, ws, curr_batch_size, args_shape);
+
+    if (has_end_ || has_shape_) {  // ``start`` can be omitted


Food for thought: maybe we could live with only the start - which would work like a[start:]

dali/operators/generic/slice/slice_attr.h

mzient · 2021-01-27T14:09:45Z

dali/operators/generic/slice/slice_attr.h

@@ -122,14 +222,17 @@ class SliceAttr {
          axes = GetDimIndices(shape_layout, axis_names_).to_vector();
        }

+        constexpr double i64min = std::numeric_limits<int64_t>::min();


Suggested change

constexpr double i64min = std::numeric_limits<int64_t>::min();

constexpr double i64min = static_cast<double>(std::numeric_limits<int64_t>::min());

...or you'll fall victim to clang's warnings.

mzient · 2021-01-27T14:16:45Z

dali/pipeline/workspace/workspace.h

-      return GetInputShape<GPUBackend>(input_idx).num_samples();
-    }
-    return GetInputShape<CPUBackend>(input_idx).num_samples();
+    return GetInputShape(input_idx).num_samples();


This is extremely wasteful - the shape is returned by value, making a copy of an std::vector.

mzient · 2021-01-27T14:16:52Z

dali/pipeline/workspace/workspace.h

+    DALI_ENFORCE(NumInput() > 0, "No inputs found");
+    DALI_ENFORCE(input_idx >= 0 && input_idx < NumInput(),
+                 make_string("Invalid input index: ", input_idx, "; while NumInput: ", NumInput()));
+    return GetInputShape(input_idx).sample_dim();


mzient · 2021-02-02T21:21:40Z

!build

dali-automaton · 2021-02-02T21:26:18Z

CI MESSAGE: [2038719]: BUILD STARTED

dali-automaton · 2021-02-02T22:13:53Z

CI MESSAGE: [2038719]: BUILD FAILED

mzient · 2021-02-03T07:25:38Z

dali/operators/generic/slice/slice_attr.h

+  void ProcessPositionalInputArgs(int data_idx,
+                                  const AnchorT *slice_anchor_data,
+                                  const ShapeT *slice_shape_data,
+                                  const EndT *slice_end_data) {


It raised a warning in clang in the lambda - but slice_end_data is not used in this function at all. Do we need this argument at all here?

dali/operators/generic/slice/slice_attr.h

mzient · 2021-02-04T10:56:11Z

!build

dali-automaton · 2021-02-04T11:00:44Z

CI MESSAGE: [2044839]: BUILD STARTED

dali-automaton · 2021-02-04T11:21:07Z

CI MESSAGE: [2044839]: BUILD FAILED

dali/operators/generic/slice/slice_attr.h

mzient · 2021-02-04T12:40:28Z

!build

dali-automaton · 2021-02-04T12:45:40Z

CI MESSAGE: [2045077]: BUILD STARTED

dali-automaton · 2021-02-04T14:06:48Z

CI MESSAGE: [2045077]: BUILD FAILED

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

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

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

* This argument was captured by lambda but not used, which caused a warning in clang build Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

…nalInputArgs. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

jantonguirao · 2021-02-09T10:18:14Z

!build

dali-automaton · 2021-02-09T10:20:44Z

CI MESSAGE: [2058496]: BUILD STARTED

dali-automaton · 2021-02-09T11:35:04Z

CI MESSAGE: [2058496]: BUILD FAILED

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

jantonguirao · 2021-02-09T11:58:42Z

!build

dali-automaton · 2021-02-09T12:00:37Z

CI MESSAGE: [2058664]: BUILD STARTED

dali-automaton · 2021-02-09T13:31:55Z

CI MESSAGE: [2058664]: BUILD PASSED

jantonguirao changed the title ~~[WIP] Enable named Slice arguments~~ Enable named Slice arguments: start/rel_start, end/rel_end, shape/rel_shape Jan 22, 2021

awolant assigned awolant, banasraf and mzient and unassigned banasraf Jan 25, 2021

mzient reviewed Jan 25, 2021

View reviewed changes

dali/operators/decoder/image_decoder.cc Show resolved Hide resolved

mzient reviewed Jan 25, 2021

View reviewed changes

jantonguirao force-pushed the slice_named_args branch from 72b0c92 to 4a22d0f Compare January 25, 2021 14:55

mzient reviewed Jan 27, 2021

View reviewed changes

dali/operators/generic/slice/slice_attr.h Show resolved Hide resolved

mzient reviewed Jan 27, 2021

View reviewed changes

dali/operators/generic/slice/slice_attr.h Show resolved Hide resolved

mzient reviewed Jan 27, 2021

View reviewed changes

awolant approved these changes Jan 29, 2021

View reviewed changes

mzient approved these changes Feb 2, 2021

View reviewed changes

mzient reviewed Feb 3, 2021

View reviewed changes

mzient reviewed Feb 4, 2021

View reviewed changes

dali/operators/generic/slice/slice_attr.h Outdated Show resolved Hide resolved

mzient reviewed Feb 4, 2021

View reviewed changes

dali/operators/generic/slice/slice_attr.h Outdated Show resolved Hide resolved

mzient reviewed Feb 4, 2021

View reviewed changes

dali/operators/generic/slice/slice_attr.h Outdated Show resolved Hide resolved

mzient reviewed Feb 4, 2021

View reviewed changes

dali/operators/generic/slice/slice_attr.h Outdated Show resolved Hide resolved

jantonguirao and others added 10 commits February 9, 2021 11:17

[WIP] Enable named Slice arguments

4dcc21f

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

Support named Slice arguments start/rel_start end/rel_end shape/rel_s…

bec4204

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

Update op documentation

64fa714

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

Enforce normalized_* flags are not provided when using named arguments

f49e4df

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

Code review fixes

4ea97f0

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

Code review fixes

ab07cd9

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

Code review fixes

5e4b4a6

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

Code review fixes

8665830

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

Remove unused argument slice_end_data

d01c463

* This argument was captured by lambda but not used, which caused a warning in clang build Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Remove unused slice_end argument and related code from ProcessPositio…

41dc054

…nalInputArgs. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

jantonguirao force-pushed the slice_named_args branch from 2c1112e to 41dc054 Compare February 9, 2021 10:18

Fix schema

623ab08

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

jantonguirao merged commit 10ddecc into NVIDIA:master Feb 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable named Slice arguments: start/rel_start, end/rel_end, shape/rel_shape #2625

Enable named Slice arguments: start/rel_start, end/rel_end, shape/rel_shape #2625

jantonguirao commented Jan 21, 2021 •

edited

Loading

jantonguirao commented Jan 22, 2021

dali-automaton commented Jan 22, 2021

dali-automaton commented Jan 22, 2021

jantonguirao commented Jan 22, 2021

mzient Jan 25, 2021

mzient Jan 27, 2021

mzient Jan 27, 2021 •

edited

Loading

mzient Jan 27, 2021

mzient Jan 27, 2021

mzient Jan 27, 2021

mzient commented Feb 2, 2021

dali-automaton commented Feb 2, 2021

dali-automaton commented Feb 2, 2021

mzient Feb 3, 2021

mzient commented Feb 4, 2021

dali-automaton commented Feb 4, 2021

dali-automaton commented Feb 4, 2021

mzient commented Feb 4, 2021

dali-automaton commented Feb 4, 2021

dali-automaton commented Feb 4, 2021

jantonguirao commented Feb 9, 2021

dali-automaton commented Feb 9, 2021

dali-automaton commented Feb 9, 2021

jantonguirao commented Feb 9, 2021

dali-automaton commented Feb 9, 2021

dali-automaton commented Feb 9, 2021

	"``normalized_anchor``/``normalized_shape`` is only relevant when using positional "
	"``normalized_anchor`` and ``normalized_shape`` can be used only with positional "

	constexpr double i64min = std::numeric_limits<int64_t>::min();
	constexpr double i64min = static_cast<double>(std::numeric_limits<int64_t>::min());

Enable named Slice arguments: start/rel_start, end/rel_end, shape/rel_shape #2625

Enable named Slice arguments: start/rel_start, end/rel_end, shape/rel_shape #2625

Conversation

jantonguirao commented Jan 21, 2021 • edited Loading

Why we need this PR?

What happened in this PR?

jantonguirao commented Jan 22, 2021

dali-automaton commented Jan 22, 2021

dali-automaton commented Jan 22, 2021

jantonguirao commented Jan 22, 2021

mzient Jan 25, 2021

Choose a reason for hiding this comment

mzient Jan 27, 2021

Choose a reason for hiding this comment

mzient Jan 27, 2021 • edited Loading

Choose a reason for hiding this comment

mzient Jan 27, 2021

Choose a reason for hiding this comment

mzient Jan 27, 2021

Choose a reason for hiding this comment

mzient Jan 27, 2021

Choose a reason for hiding this comment

mzient commented Feb 2, 2021

dali-automaton commented Feb 2, 2021

dali-automaton commented Feb 2, 2021

mzient Feb 3, 2021

Choose a reason for hiding this comment

mzient commented Feb 4, 2021

dali-automaton commented Feb 4, 2021

dali-automaton commented Feb 4, 2021

mzient commented Feb 4, 2021

dali-automaton commented Feb 4, 2021

dali-automaton commented Feb 4, 2021

jantonguirao commented Feb 9, 2021

dali-automaton commented Feb 9, 2021

dali-automaton commented Feb 9, 2021

jantonguirao commented Feb 9, 2021

dali-automaton commented Feb 9, 2021

dali-automaton commented Feb 9, 2021

jantonguirao commented Jan 21, 2021 •

edited

Loading

mzient Jan 27, 2021 •

edited

Loading