Add out-of-bounds-policy (including pad support) to Slice/Crop #2000

jantonguirao · 2020-06-05T13:12:42Z

Why we need this PR?

Pick one, remove the rest

It adds new feature needed to allow cropping windows that are larger than the input shape, or are anchored outside of bounds

What happened in this PR?

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

What solution was applied:
Slice and Crop operators has two new arguments: out_of_bounds_policy and fill_values. Out of bounds policy determines how the operator should behave when attempting to slice outside of the input region. Valid arguments are "error" (not allowed), "pad" (the output will be padded), "trim_to_shape" the output will be resized to fit the input bounds. If "pad" is provided, fill_values will determine the value (or values) to use when padding
Bug fix in Slice GPU kernel for the case where padding is needed, channel dimension is last last and there is no channel slicing
Affected modules and functionalities:
Slice and Crop operators (They share SliceBase implementation)
Key points relevant for the review:
Bug fix correctness
Python tests logic
Validation and testing:
Python tests added
Documentation (including examples):
Operator schema documentation updated

JIRA TASK: [DALI-1409]

jantonguirao · 2020-06-05T13:19:49Z

!build

dali-automaton · 2020-06-05T13:25:33Z

CI MESSAGE: [1373423]: BUILD STARTED

dali-automaton · 2020-06-05T13:27:58Z

CI MESSAGE: [1373423]: BUILD FAILED

jantonguirao · 2020-06-05T13:38:51Z

!build

dali-automaton · 2020-06-05T13:40:40Z

CI MESSAGE: [1373476]: BUILD STARTED

dali-automaton · 2020-06-05T15:09:07Z

CI MESSAGE: [1373476]: BUILD FAILED

jantonguirao · 2020-06-08T16:16:17Z

!build

dali-automaton · 2020-06-08T16:20:41Z

CI MESSAGE: [1378365]: BUILD STARTED

jantonguirao · 2020-06-09T09:46:08Z

dali/kernels/slice/slice_gpu.cuh

-    SliceFunc<NextDims, OutputType, InputType, false>(out, in, out_strides, in_strides, anchor,
-                                                      in_shape, fill_values, channel_dim, offset,
-                                                      block_end);
+    SliceFunc<NextDims, OutputType, InputType, false>(


The changes in this file are a bug fix

jantonguirao · 2020-06-09T09:46:26Z

dali/kernels/slice/slice_kernel_test.h

@@ -242,6 +242,22 @@ struct ArgsGen_CompletelyOutOfBounds{
  }
 };

+template <typename OutputType, int Dims = 3>


New test case to cover the bug fix

jantonguirao · 2020-06-09T09:47:57Z

dali/operators/image/crop/crop_attr.h

-        } else {
-          DALI_FAIL("not supported number of dimensions (" +
-                    std::to_string(input_shape.size()) + ")");
+        auto crop_shape = input_shape;


Simplified and unified to one place the logic of CropAttr

jantonguirao · 2020-06-09T09:54:27Z

dali/operators/generic/slice/slice_base.cc

+        using Impl = SliceBaseCpu<InputType, InputType, Dims>;
+        if (!impl_)
+          impl_ = std::make_unique<Impl>();
+        FillArgs(reinterpret_cast<Impl*>(impl_.get())->Args(), ws);


A little ugly but IMO better than duplicating the logic to FillArgs (which has nothing to do with CPU/GPU)

jantonguirao · 2020-06-09T10:59:37Z

!build

dali-automaton · 2020-06-09T12:00:53Z

CI MESSAGE: [1380987]: BUILD STARTED

dali-automaton · 2020-06-09T13:57:41Z

CI MESSAGE: [1380987]: BUILD FAILED

JanuszL · 2020-06-09T14:39:56Z

dali/operators/generic/slice/out_of_bounds_policy.h

+      for (int d = 0; d < input_shape.size(); d++) {
+        if (slice_anchor[d] < 0)
+          slice_anchor[d] = 0;
+        if (slice_anchor[d] + slice_shape[d] >= input_shape[d])


What if slice_anchor is bigger than the input_shape?

JanuszL · 2020-06-09T14:42:07Z

dali/operators/generic/slice/out_of_bounds_policy.h

+    default:
+      for (int d = 0; d < input_shape.size(); d++) {
+        auto slice_end = slice_anchor[d] + slice_shape[d];
+        if (slice_anchor[d] < 0 || slice_anchor[d] > input_shape[d] || slice_end < 0 ||


slice_anchor[d] > input_shape[d] make sense only when slice_shape == 0, otherwise it should be covered by slice_end > input_shape[d]).
I would test for slice_shape separately no matter what policy is used.

JanuszL · 2020-06-09T14:43:44Z

dali/operators/generic/slice/slice.cc

+Supported values are:
+- \"error\" (default) : Attempting to slice outside of the bounds of the image will produce an error.
+- \"pad\": The input will be padded as needed with zeros or any other value specified with ``fill_values`` argument.
+- \"trim_to_shape\": The slice window will be resized so that it stays within the bounds of the input.


Suggested change

- \"trim_to_shape\": The slice window will be resized so that it stays within the bounds of the input.

- \"trim_to_shape\": The slice window will be cut to the bounds of the input.

dali/operators/generic/slice/slice_attr.h

JanuszL · 2020-06-09T15:00:42Z

dali/operators/generic/slice/slice_attr.h

@@ -58,15 +72,15 @@ class SliceAttr {
    }
  }

-  void ProcessArguments(DeviceWorkspace &ws) {
+
+  void ProcessArguments(const MixedWorkspace &ws) {


MixedWorkspace ??

Yes, remember this is used by ImageDecoderSlice as well

But DeviceWorkspace variant is gone. Is it intended?

It's in line 50.

dali/operators/generic/slice/slice_base.cc

JanuszL · 2020-06-09T15:14:01Z

dali/operators/generic/slice/slice_base.cu

@@ -1,4 +1,4 @@
-// Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved.
+// Copyright (c) 2020, NVIDIA CORPORATION. All rights reserved.


Suggested change

// Copyright (c) 2020, NVIDIA CORPORATION. All rights reserved.

// Copyright (c) 2019-2020, NVIDIA CORPORATION. All rights reserved.

dali/operators/image/crop/crop.cc

JanuszL · 2020-06-09T15:43:28Z

dali/operators/image/crop/crop_attr.h

+        int w_dim = shape_layout.find('W');
+
+        DALI_ENFORCE(h_dim >= 0 && w_dim >= 0,
+          "Height and Width must be present in the layout. Got: " + shape_layout.str());


Suggested change

"Height and Width must be present in the layout. Got: " + shape_layout.str());

"[H]eight and [W]idth must be present in the layout. Got: " + shape_layout.str());

dali/test/python/test_operator_slice.py

klecki · 2020-06-15T12:04:49Z

dali/operators/generic/slice/slice_base.h

+  template <typename OutputType, int Dims>
+  void FillArgs(std::vector<kernels::SliceArgs<OutputType, Dims>>& slice_args,
+                const workspace_t<Backend> &ws) {
+    this->DataDependentSetup(ws);


I might be nitpicky, but this will be more general comment to the code structure in the FillArgs function.

The DataDependentSetup does some magic, but in fact it just obtains the cropping information from the arguments (passed in a crop or slice way). Maybe more direct call to Slice/Crop attr here would be clearer, or some other name that highlights that fact.

Shouldn't the fill_values be validated once against the number of channels, preferably by OutOfBoundsAttrs?

This fills a little bit boilerplatey, without the channel business and by adding a function to convert from CropWindow to SliceChannels it would be compacted to basically:

this->ProcessCroppingAttrs(ws); for (samples_idx : samples) { auto crop_win_gen = this->GetCropWindowGenerator(i); CropWindow win = crop_win_gen(in_shape[i], in_layout); ApplySliceBoundsPolicy(out_of_bounds_policy_, in_shape[i], win.anchor, win.shape); slice_args[i] = ToSliceArgs(win);

Basically, your ToSliceArgs would do what this FillArgs does, except for maybe the validation of fill_values that could happen somewhere else. That and the name change from DataDependentSetup to ProcessCroppingAttrs. Correct?

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

klecki · 2020-06-15T13:14:48Z

dali/operators/generic/slice/slice_base.cu

 }

-}  // namespace detail
+template <>
+bool SliceBase<GPUBackend>::SetupImpl(std::vector<OutputDesc> &output_desc,


Isn't this the same as for CPU? (Same with RunImpl).

Yes, pretty much, except for the usage of GPU and CPU kernel respective. I don't want to include both kernels in the same header

klecki · 2020-06-15T13:19:17Z

dali/kernels/slice/slice_kernel_test.h

+    args.shape[1] = 2 * input_shape[1];
+    args.shape[2] = input_shape[2];
+    args.fill_values = {128};
+    args.channel_dim = -1;


From the description of the PR the bug was supposed to happen, when the channel dim is last, but there is no channel slicing. Doesn't this specify a no-channel variant with the -1?

No, channel_dim=-1 means I don't care which dimension is channels. That is because we have a single pad value (128 in this case) so the channel dimension doesn't need special treatment

klecki · 2020-06-15T13:54:16Z

dali/test/python/test_operator_slice.py

+        outs = pipe.run()
+
+def test_slice_with_out_of_bounds_error():
+    in_shape = (40, 80, 3)


Could there be any reason to test slice with non-uniform batch?

Doesn't make much sense for this particular test case. Do you mean generally speaking?

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

jantonguirao · 2020-06-16T13:08:15Z

!build

dali-automaton · 2020-06-16T13:25:27Z

CI MESSAGE: [1398972]: BUILD STARTED

dali-automaton · 2020-06-16T15:27:41Z

CI MESSAGE: [1398972]: BUILD FAILED

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

jantonguirao · 2020-06-17T08:23:10Z

!build

dali-automaton · 2020-06-17T08:25:37Z

CI MESSAGE: [1401940]: BUILD STARTED

dali-automaton · 2020-06-17T10:15:30Z

CI MESSAGE: [1401940]: BUILD PASSED

jantonguirao force-pushed the slice_op_pad_support branch from e64a660 to c217394 Compare June 5, 2020 13:37

jantonguirao commented Jun 9, 2020

View reviewed changes

jantonguirao changed the title ~~[WIP][DO NOT REVIEW] Add support for pad in Slice/Crop~~ Add out-of-bounds-policy (including pad support) to Slice/Crop Jun 9, 2020

jantonguirao commented Jun 9, 2020

View reviewed changes

jantonguirao requested a review from a team June 9, 2020 10:01