Add ROIRandomCrop operator #2638

jantonguirao · 2021-01-26T16:37:45Z

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

Why we need this PR?

Pick one, remove the rest

It adds new feature needed to perform biased random crop in segmentation applications

What happened in this PR?

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

What solution was applied:
Added a new operator ROIRandomCrop that takes a fixed cropping window shape and a region of interest and produces a cropping window anchor (start coordinates) so that the resulting cropping window contains as much of the ROI as possible.
Affected modules and functionalities:
New op
Key points relevant for the review:
Calculation of crop anchor.
Test coverage? corner cases?
Validation and testing:
Python tests
Documentation (including examples):
Operator documentation

JIRA TASK: [DALI-1816]

dali/operators/generic/roi_random_crop.cc

JanuszL · 2021-01-27T11:39:43Z

dali/operators/generic/roi_random_crop.cc

+Note: Using ``roi_end`` is mutually exclusive with ``roi_shape``.)code", nullptr, true)
+    .AddOptionalArg<std::vector<int>>("roi_shape",
+      R"code(ROI shape.
+
+Note: Using ``roi_shape`` is mutually exclusive with ``roi_end``.)code", nullptr, true)


Suggested change

Note: Using ``roi_end`` is mutually exclusive with ``roi_shape``.)code", nullptr, true)

.AddOptionalArg<std::vector<int>>("roi_shape",

R"code(ROI shape.

Note: Using ``roi_shape`` is mutually exclusive with ``roi_end``.)code", nullptr, true)

.. Note::

Using ``roi_end`` is mutually exclusive with ``roi_shape``.)code", nullptr, true)

.AddOptionalArg<std::vector<int>>("roi_shape",

R"code(ROI shape.

.. Note::

Using ``roi_shape`` is mutually exclusive with ``roi_end``.)code", nullptr, true)

JanuszL · 2021-01-27T11:40:12Z

dali/operators/generic/roi_random_crop.cc

+If provided, the cropping window start will be selected so that the cropping window is within the
+bounds of the input.
+
+Note: Providing ``in_shape`` is incompatible with feeding the input data directly as a positional input.


Suggested change

Note: Providing ``in_shape`` is incompatible with feeding the input data directly as a positional input.

.. Note::

Providing ``in_shape`` is incompatible with feeding the input data directly as a positional input.

dali/operators/generic/roi_random_crop.cc

JanuszL · 2021-01-27T11:50:25Z

dali/operators/generic/roi_random_crop.cc

+  if ((roi_end_.IsDefined() + roi_shape_.IsDefined()) != 1)
+    DALI_FAIL("Either ROI end or ROI shape should be defined, but not both");


Suggested change

if ((roi_end_.IsDefined() + roi_shape_.IsDefined()) != 1)

DALI_FAIL("Either ROI end or ROI shape should be defined, but not both");

DALI_ENFORCE(roi_end_.IsDefined() + roi_shape_.IsDefined()) == 1,

"Either ROI end or ROI shape should be defined, but not both");

JanuszL · 2021-01-27T11:58:01Z

dali/operators/generic/roi_random_crop.cc

+  if (roi_end_.IsDefined()) {
+    roi_end_.Acquire(spec_, ws, nsamples, sh);
+  } else {
+    assert(roi_shape_.IsDefined());


Do you need that assert as you already have one check in the constructor?

Isn't it one of the reasons to have asserts - to check for inconsistent internal state?

I put it there mostly for documentation, but also to double-check that the logic is sane

JanuszL · 2021-01-27T12:06:36Z

dali/operators/generic/roi_random_crop.cc

+    if (in_shape_arg_.IsDefined() && (ws.NumInput() == 1)) {
+      DALI_FAIL("``in_shape`` argument is incompatible with providing an input.")
+    }


Suggested change

if (in_shape_arg_.IsDefined() && (ws.NumInput() == 1)) {

DALI_FAIL("``in_shape`` argument is incompatible with providing an input.")

}

DALI_ENFORCE(in_shape_arg_.IsDefined() && (ws.NumInput() == 1) == false, "``in_shape`` argument is incompatible with providing an input.")

JanuszL · 2021-01-27T12:14:38Z

dali/operators/generic/roi_random_crop.cc

+        for (int d = 0; d < ndim; d++) {
+          DALI_ENFORCE(roi_start_[s].data[d] >= 0 &&
+                       sample_sh[d] >= (roi_start_[s].data[d] + roi_shape_[s].data[d]),
+                       "ROI can't be out of bounds.");


Can you print the offending shape and roi bounds?

JanuszL · 2021-01-27T12:31:15Z

dali/test/python/test_operator_roi_random_crop.py

+                                      shape=(ndim,), dtype=types.INT32, device='cpu')
+        roi_start = fn.random.uniform(range=(roi_min_start, roi_max_start + 1),
+                                      shape=(ndim,), dtype=types.INT32, device='cpu')
+        crop_start = fn.roi_random_crop(*inputs, crop_shape=crop_shape,


I think you should test roi_end argument as well.

JanuszL · 2021-01-27T12:34:26Z

dali/test/python/test_operator_roi_random_crop.py

+        roi_start = fn.random.uniform(range=(roi_min_start, roi_max_start + 1),
+                                      shape=(ndim,), dtype=types.INT32, device='cpu')
+        crop_start = fn.roi_random_crop(*inputs, crop_shape=crop_shape,
+                                        roi_start=roi_start, roi_shape=roi_shape,


Can you add a negative cases when both use_in_shape_arg and use_shape_like_in are provided, as well as roi_end with roi_shape?

JanuszL · 2021-01-27T12:36:24Z

dali/test/python/test_operator_roi_random_crop.py

+        outputs += [in_shape_out]
+    pipe.set_outputs(*outputs)
+    pipe.build()
+    for iter in range(niter):


Suggested change

for iter in range(niter):

for _ in range(niter):

JanuszL · 2021-01-27T12:37:03Z

dali/test/python/test_operator_roi_random_crop.py

+    for iter in range(niter):
+        outputs = pipe.run()
+        for idx in range(batch_size):
+            roi_start = outputs[0].at(idx).tolist()


Can you not use at as it is deprecated?

JanuszL · 2021-01-27T12:41:28Z

dali/test/python/test_operator_roi_random_crop.py

+
+            for d in range(ndim):
+                if in_shape is not None:
+                    assert crop_start[d] >= 0


Shouldn't always be crop_start[d] >= 0 no matter if in_shape is provided?

Not really. Out of bounds slicing is allowed. We are OK with a negative anchor as long as the cropping window contains the ROI (could happen for a big cropping window and a small ROI near the edge)

JanuszL · 2021-01-27T12:44:14Z

dali/operators/generic/roi_random_crop.cc

+    if (in_shape_arg_.IsDefined()) {
+      in_shape_.resize(nsamples, ndim);
+      in_shape_arg_.Acquire(spec_, ws, nsamples, sh);
+      for (int s = 0; s < nsamples; s++) {
+        auto sample_sh = in_shape_.tensor_shape_span(s);
+        for (int d = 0; d < ndim; d++) {
+          sample_sh[d] = in_shape_arg_[s].data[d];
+        }
+      }
+    } else {
+      auto &in = ws.template InputRef<CPUBackend>(0);
+      in_shape_ = in.shape();
+    }


What happens if no input nor in_shape_arg_ are provided?

Nothing special, we just don't bound the cropping window to be within the image

Now I see. I missed that L121 condition covers the L138 loop as well.

JanuszL · 2021-01-27T12:45:53Z

dali/operators/generic/roi_random_crop.cc

+        }
+      } else {
+        for (int d = 0; d < ndim; d++) {
+          DALI_ENFORCE(roi_start_[s].data[d] >= 0 && sample_sh[d] >= roi_end_[s].data[d],


Do we need to bother with roi_end_ here. Maybe it should be just roi_start_ and roi_shape_, and values from roi_end should be used to infer roi_shape_ if not provided directly.

It'd involve extra storage, and the code accessing this storage would look different than the arguments directly provided (because those are tensor views). This is why I preferred to do it on the fly

It's easier to check end than start+shape?

JanuszL · 2021-01-27T13:06:19Z

dali/operators/generic/roi_random_crop.cc

+                     ws.ArgumentInput("crop_shape").size() :
+                     ws.GetRequestedBatchSize(0);
+  crop_shape_.Acquire(spec_, ws, nsamples, true);
+  int ndim = crop_shape_[0].shape[0];


Maybe you should check if crop_shape_ is uniform?

That's what the true in Acquire does

It doesn't have to - each sample can be different.

This is the shape of a shape we are talking about. Uniformity in crop_shape means same number of dimensions

JanuszL · 2021-01-27T13:07:31Z

dali/operators/generic/roi_random_crop.cc

+          int64_t roi_extent = -1;
+          int64_t roi_start = roi_start_[sample_idx].data[d];
+          int64_t crop_extent = crop_shape_[sample_idx].data[d];
+          if (roi_end_.IsDefined()) {


I think you can do this in setup (roi_extent calculation).

I could but it'd involve extra storage to keep those.

JanuszL · 2021-01-27T14:15:30Z

dali/operators/generic/roi_random_crop.cc

+          if (roi_extent == crop_extent) {
+            crop_start[sample_idx].data[d] = roi_start;
+          } else if (roi_extent > crop_extent) {


How about:

Suggested change

if (roi_extent == crop_extent) {

crop_start[sample_idx].data[d] = roi_start;

} else if (roi_extent > crop_extent) {

if (roi_extent == crop_extent) {

crop_start[sample_idx].data[d] = roi_start;

} else {

int64_t range = roi_extent - crop_extent;

if (sample_sh) {

range = std::max<int64_t>(range, -roi_start); // make sure that if roi_extent < crop_extent we won't be < 0

}

int64_t range_start = std::min<int64_t>(roi_start, roi_start + range);

int64_t range_end = std::max<int64_t>(roi_start, roi_start + range)

auto dist = std::uniform_int_distribution<int64_t>(range_start, range_end);

crop_start[sample_idx].data[d] = dist(rngs_[sample_idx]);

}

Maybe you can get rid of if at all.

JanuszL · 2021-01-27T14:16:11Z

dali/operators/generic/roi_random_crop.cc

+          } else if (roi_extent > crop_extent) {
+            int64_t range_end = roi_start + roi_extent - crop_extent;
+            if (sample_sh)
+              range_end = std::min<int64_t>(sample_sh[d] - crop_extent, range_end);


I think you check if ROI is inside shape already in setup. So this is not needed.

It doesn't depend on the ROI but on the cropping window. Imagine a large cropping window, it would go easily out of bounds if we want to cover all the different anchors that cover the whole ROI. In the case that the cropping window is bounded the range of possible anchors is limited. Otherwise it isn't

mzient · 2021-01-28T14:20:16Z

dali/operators/generic/roi_random_crop.cc

+provided region of interest (ROI) is contained in it.
+
+If the ROI is bigger than the cropping window, the cropping window will be a subwindow of the ROI.
+If the ROI is smaller than the cropping window, the whole ROI should be contained in the cropping window.


Suggested change

If the ROI is smaller than the cropping window, the whole ROI should be contained in the cropping window.

If the ROI is smaller than the cropping window, the whole ROI shall be contained in the cropping window.

mzient · 2021-01-28T14:24:16Z

dali/operators/generic/roi_random_crop.cc

+If the ROI is smaller than the cropping window, the whole ROI should be contained in the cropping window.
+
+If an input shape (``in_shape``) is given, the resulting cropping window is selected to be within the
+bounds of that input shape. Alternatively, the input data subject to cropping can be fed directly to


Suggested change

bounds of that input shape. Alternatively, the input data subject to cropping can be fed directly to

bounds of that input shape. Alternatively, the input data subject to cropping can be passed to the operator, in

which case the cropping window will be constrained to the shape of the data.

fed suggests that the data itself is consumed - and I think we should make it very clear that the operator doesn't touch the data.

mzient · 2021-01-28T14:34:23Z

dali/operators/generic/roi_random_crop.cc

+        DALI_ENFORCE(sample_sh[d] >= crop_shape_[s].data[d],
+                     make_string("Cropping shape can't be bigger than the input shape. "
+                                 "Got: crop_shape[", crop_shape_[s].data[d], "] and sample_shape[",
+                                 sample_sh[d], "] for d=", d));


I wonder what we should do in this case...

Maybe we should allow (with an extra parameter) to clip the cropping window if it's larger than input? Or, alternatively, keep out-of-bounds window (pad)? Just thinking aloud.

I don't think we should complicate this even more unless we have a specific use in mind.

mzient · 2021-01-28T14:43:34Z

dali/operators/generic/roi_random_crop.cc

+  auto& thread_pool = ws.GetThreadPool();
+
+  for (int sample_idx = 0; sample_idx < nsamples; sample_idx++) {
+    thread_pool.AddWork(


I wonder if asynchronous execution is justified here - after all this operator performs ndim loops per sample - not much compared to the overhead of scheduling work to thread pool.

I can get rid of it

mzient · 2021-01-28T15:33:35Z

dali/test/python/test_operator_roi_random_crop.py

+            for idx in range(6, 10):
+                check_crop_start(np.array(outputs[idx][s]).tolist(), roi_start, roi_shape, crop_shape, in_shape)
+
+def test_random_mask_pixel():


Suggested change

def test_random_mask_pixel():

def test_roi_random_cropl():

?

yes, copy-paste :)

mzient · 2021-01-28T15:36:23Z

dali/test/python/test_operator_roi_random_crop.py

+        crop_shape = fn.random.uniform(range=(crop_min_extent, crop_max_extent + 1), 
+                                       shape=(ndim,), dtype=types.INT32, device='cpu')
+        roi_shape = fn.random.uniform(range=(roi_min_extent, roi_max_extent + 1),
+                                      shape=(ndim,), dtype=types.INT32, device='cpu')
+        roi_start = fn.random.uniform(range=(roi_min_start, roi_max_start + 1),
+                                      shape=(ndim,), dtype=types.INT32, device='cpu')


I see that scalar arguments are not tested. Perhaps you could do sth like:

def make_arg(lo, hi): return [lo] * ndim if lo == hi else fn.random.uniform(range=(lo, hi+1), shape=[ndim], dtype=types.INT32)

mzient · 2021-01-28T15:38:01Z

dali/test/python/test_operator_roi_random_crop.py

+        shape_like_in = dali.fn.external_source(lambda: np.zeros(shape_gen_f()),
+                                                device='cpu', batch=False)


Try variable batch size?

JanuszL · 2021-01-28T16:30:36Z

dali/operators/generic/roi_random_crop.cc

+  int nsamples = crop_start.shape.size();
+  int ndim = crop_start[0].shape[0];
+
+  auto& thread_pool = ws.GetThreadPool();


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

mzient · 2021-01-28T17:23:14Z

dali/test/python/test_operator_roi_random_crop.py

+        roi_start = np.array([(roi_min_start + roi_max_start) // 2] * ndim, dtype=np.int32) if random.choice([True, False]) \
+            else fn.random.uniform(range=(roi_min_start, roi_max_start + 1),
+                                    shape=(ndim,), dtype=types.INT32, device='cpu')
+        roi_end = roi_start + roi_shape


If this works, then it's by chance - if roi_start is a numpy.ndarray and roi_shape is a DataNode, this wouldn't work.

mzient · 2021-01-28T17:23:52Z

dali/test/python/test_operator_roi_random_crop.py

+        shape_like_in = dali.fn.external_source(data_gen_f, device='cpu')
+        in_shape = dali.fn.shapes(shape_like_in, dtype=types.INT32)
+
+        crop_shape =  [(crop_min_extent + crop_max_extent) // 2] * ndim if random.choice([True, False]) \


Numpy arrays are passed as argument inputs, so this still does not test non-tensor arguments. It must be a plain list.

You are right. I fixed that now

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

jantonguirao force-pushed the roi_random_crop branch from b198ac0 to 9749666 Compare January 26, 2021 16:38

jantonguirao assigned JanuszL and mzient Jan 27, 2021

JanuszL reviewed Jan 27, 2021

View reviewed changes

dali/operators/generic/roi_random_crop.cc Show resolved Hide resolved

JanuszL reviewed Jan 27, 2021

View reviewed changes

dali/operators/generic/roi_random_crop.cc Show resolved Hide resolved

JanuszL reviewed Jan 27, 2021

View reviewed changes

mzient reviewed Jan 28, 2021

View reviewed changes

jantonguirao force-pushed the roi_random_crop branch from 788bd74 to f483f2c Compare January 28, 2021 15:08

mzient reviewed Jan 28, 2021

View reviewed changes

JanuszL reviewed Jan 28, 2021

View reviewed changes

JanuszL approved these changes Jan 28, 2021

View reviewed changes

jantonguirao added 5 commits January 28, 2021 17:51

Add ROIRandomCrop operator

e6ddd67

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

move ROIRandomCrop out of segmentation namespace

ae0b349

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

Code review fixes

35102c3

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

Code review fixes

8d2c072

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

Code review fixes

6512b9d

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

jantonguirao force-pushed the roi_random_crop branch from f483f2c to 6512b9d Compare January 28, 2021 17:10

mzient reviewed Jan 28, 2021

View reviewed changes

mzient approved these changes Jan 29, 2021

View reviewed changes

Code review tests

fbb4c23

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

jantonguirao force-pushed the roi_random_crop branch from a66d70e to fbb4c23 Compare January 29, 2021 08:21

jantonguirao merged commit a5d34b3 into NVIDIA:master Feb 1, 2021

JanuszL mentioned this pull request May 19, 2021

DALI 2021 roadmap #2978

Closed

		if ((roi_end_.IsDefined() + roi_shape_.IsDefined()) != 1)
		DALI_FAIL("Either ROI end or ROI shape should be defined, but not both");

-          if (roi_extent == crop_extent) {
-            crop_start[sample_idx].data[d] = roi_start;
-          } else if (roi_extent > crop_extent) {
+          if (roi_extent == crop_extent) {
+            crop_start[sample_idx].data[d] = roi_start;
+          } else {
+            int64_t range = roi_extent - crop_extent;
+            if (sample_sh) {
+              range = std::max<int64_t>(range, -roi_start); // make sure that if roi_extent < crop_extent we won't be < 0
+            }
+            int64_t range_start = std::min<int64_t>(roi_start, roi_start + range);
+            int64_t range_end = std::max<int64_t>(roi_start, roi_start + range)
+            auto dist = std::uniform_int_distribution<int64_t>(range_start, range_end);
+            crop_start[sample_idx].data[d] = dist(rngs_[sample_idx]);
+         }

	If the ROI is smaller than the cropping window, the whole ROI should be contained in the cropping window.
	If the ROI is smaller than the cropping window, the whole ROI shall be contained in the cropping window.

	bounds of that input shape. Alternatively, the input data subject to cropping can be fed directly to
	bounds of that input shape. Alternatively, the input data subject to cropping can be passed to the operator, in
	which case the cropping window will be constrained to the shape of the data.

		shape_like_in = dali.fn.external_source(lambda: np.zeros(shape_gen_f()),
		device='cpu', batch=False)

Add ROIRandomCrop operator #2638

Add ROIRandomCrop operator #2638

Conversation

jantonguirao commented Jan 26, 2021 • edited by JanuszL 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

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

JanuszL Jan 27, 2021 • edited Loading

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

jantonguirao commented Jan 26, 2021 •

edited by JanuszL

Loading

JanuszL Jan 27, 2021 •

edited

Loading