Rotate per-frame #3820

stiepan · 2022-04-13T09:46:01Z

Signed-off-by: Kamil Tokarski ktokarski@nvidia.com

Category:

New feature (non-breaking change which adds functionality)

Description:

Adds support for sequence processing to rotate operator.
Adds support for per-frame tensor input to angle and axis parameters.
Adds sequence processing tests (where processing batches of expanded frames is a reference)
- Test processing video input
- Test processing synthetic sequence of random 3D inputs.

Additional information:

If not specified, the output shape is inferred from rotation parameters and the input shape: in case of per-frame arguments it requires some coalescing of different shapes of frames that belong to the same output sequence. The coalescing is performed by choosing: for each extent the maximal value from all frames, then the parity correction of the output shape is performed by majority vote.

Affected modules and functionalities:

warp base, to utilize SequenceOperator and pass around unfolded extents (to map frames to sequences when coalescing shape)
rotate params provider to perform shape coalescing
python tests of rotate op
sequences test utils - extending the framework so that it can account for shape coalescing in baseline pipline

Key points relevant for the review:

Checklist

Tests

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: ROTATE.10

JIRA TASK: DALI-2508

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

mzient · 2022-04-20T10:26:32Z

dali/operators/image/remap/rotate_params.h

+    // kernels::vec2shape reverses the extents, store them in that order in parity vector
+    parity[2 - i] = in_size[dominant_src_axis[i]] % 2;


What's wrong with storing it naturally and returning vec2shape(parity)? I think the code would be more readable that way.

Because it has + operator defined:)

Fair point :) But now that you've mentioned it, it also has a max. How about switching the order to x, y, z and returning two vectors? You could calculate the size on ivec2/3 and convert to TensorShape at the very end.

Great, thanks for pointing to it.

stiepan · 2022-04-20T13:55:24Z

!build

dali-automaton · 2022-04-20T14:00:14Z

CI MESSAGE: [4629623]: BUILD STARTED

mzient · 2022-04-20T14:24:27Z

dali/operators/image/remap/rotate_params.h

+      for (int dim_idx = 0; dim_idx < spatial_ndim; dim_idx++) {
+        bool should_be_odd = 2 * acc_parity[dim_idx] > num_frames;
+        if (acc_shape[dim_idx] % 2 != should_be_odd) {
+          acc_shape[dim_idx]++;
+        }
+      }


if acc_shape was an ivec, you could do:

Suggested change

for (int dim_idx = 0; dim_idx < spatial_ndim; dim_idx++) {

bool should_be_odd = 2 * acc_parity[dim_idx] > num_frames;

if (acc_shape[dim_idx] % 2 != should_be_odd) {

acc_shape[dim_idx]++;

}

}

acc_shape += (acc_shape % 2) ^ (2 * acc_parity > num_frames);

(it must be ^, because operator != returns a single boolean).

mzient · 2022-04-20T14:29:44Z

dali/test/python/test_operator_rotate.py

+  assert(len(arrays))
+  acc_max = arrays[0]
+  for array in arrays[1:]:
+    acc_max = np.maximum(acc_max, array)
+  return acc_max


Suggested change

assert(len(arrays))

acc_max = arrays[0]

for array in arrays[1:]:

acc_max = np.maximum(acc_max, array)

return acc_max

# find the elementwise maximum of the arrays in the list

return np.max(arrays, axis=0)

It will automatically treat the outer list as an extra dimension.
Perhaps you don't even need a function for that (see below).

mzient · 2022-04-20T14:31:23Z

dali/test/python/test_operator_rotate.py

+    corrected_shapes = [
+        np.array(get_3d_output_size(math.radians(angle), axis, shape, True), dtype=np.int32)
+        for shape, angle, axis in zip(input_shapes, angles, axes)]
+  max_shape = maximum_array(no_correction_shapes)


Suggested change

max_shape = maximum_array(no_correction_shapes)

max_shape = np.max(no_correction_shapes, axis=0) # elementwise maximum

mzient · 2022-04-20T14:34:18Z

dali/test/python/test_operator_rotate.py

+  parity = sum([np.array([extent % 2 for extent in shape], dtype=np.int32)
+               for shape in corrected_shapes])


Suggested change

parity = sum([np.array([extent % 2 for extent in shape], dtype=np.int32)

for shape in corrected_shapes])

parity = np.sum(np.array(corrected_shapes, dtype=np.int32) % 2, axis=0)

dali-automaton · 2022-04-20T15:09:13Z

CI MESSAGE: [4629623]: BUILD PASSED

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

mzient · 2022-04-21T10:47:07Z

dali/test/python/test_operator_rotate.py

+      for j in range(3):
+        if rotation[i, j] > maxv:
+          maxv = rotation[i, j]
+          dominant_axis[i] = j


You're overwriting dominant_axis here, which is still used in the outer loop to get maxv.
Edit: this is not important - these nested loops can be reimplemented as a one-liner, see below.

mzient · 2022-04-21T11:16:43Z

dali/test/python/test_operator_rotate.py

+
+
+def get_3d_output_size(angle, axis, input_size, parity_correction=False):
+  rotation = np.abs(get_3d_lin_rotation(angle, axis))


Nitpick: Some other name would be nice - this is not really a rotation matrix...

mzient · 2022-04-21T11:17:02Z

dali/test/python/test_operator_rotate.py

+    for i in range(3):
+      maxv = rotation[i, dominant_axis[i]]
+      for j in range(3):
+        if rotation[i, j] > maxv:
+          maxv = rotation[i, j]
+          dominant_axis[i] = j


Suggested change

for i in range(3):

maxv = rotation[i, dominant_axis[i]]

for j in range(3):

if rotation[i, j] > maxv:

maxv = rotation[i, j]

dominant_axis[i] = j

dominant_axis = np.argmax(rotation, axis=1)

mzient · 2022-04-21T11:19:12Z

dali/test/python/test_operator_rotate.py

+      if out_size[i] % 2 != in_size[dominant_axis[i]] % 2:
+        out_size[i] += 1
+
+  return np.array(list(reversed(out_size)), dtype=np.int32)


Suggested change

return np.array(list(reversed(out_size)), dtype=np.int32)

return out_size[::-1]

mzient · 2022-04-21T11:53:15Z

dali/test/python/sequences_test_utils.py

@@ -28,6 +28,49 @@
 vid_file = os.path.join(data_root, 'db', 'video',
                        'sintel', 'sintel_trailer-720p.mp4')

+class ParamsProvider:
+    def __init__(self, input_params):


How about:

Suggested change

def __init__(self, input_params):

def __init__(self, **input_params : ParamDesc):

?

I wanted to leave existing tests as they are. I split the ParamsProvider into a base class and actual class, the first is just about providing input data to the provider instance while the actual one take input arguments description (added docs there with some type annotation) and computes parameters. This way derived classes can easily control format of tensor arguments description.

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan · 2022-04-25T09:27:59Z

!build

dali-automaton · 2022-04-25T09:30:11Z

CI MESSAGE: [4667693]: BUILD STARTED

dali-automaton · 2022-04-25T10:39:54Z

CI MESSAGE: [4667693]: BUILD FAILED

dali-automaton · 2022-04-25T10:57:24Z

CI MESSAGE: [4667693]: BUILD PASSED

* Add support for FHWC and FDHWC layouts * Add support for per-frame tensor input to angle and axis parameters Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan added 7 commits April 13, 2022 11:45

PoC: Rotate per-frame

88dac15

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Majority vote on accumulated shape parity

348eb32

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Rotate test cleanup, fix the batch_size warning

0973b76

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Add rotate per frame tests

592ed5d

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Introduce params provider to sequence test utility

748f6ff

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Add switch over frame/non-frame output size inference

e414f22

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Simplify tests, add tensor output size input

c2d3a32

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

mzient self-assigned this Apr 20, 2022

stiepan added 3 commits April 20, 2022 10:40

Fix: set 3d parity in reversed order

9745d5d

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Add 3D sequence tests

2a3cd73

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Fix lint issues

b8b83c3

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan marked this pull request as ready for review April 20, 2022 09:07

stiepan changed the title ~~WIP: Rotate per-frame~~ Rotate per-frame Apr 20, 2022

jantonguirao assigned prak-nv Apr 20, 2022

mzient reviewed Apr 20, 2022

View reviewed changes

stiepan added 2 commits April 21, 2022 11:18

Review remarks: use vectorized functions

2557216

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Fix the comment

2bacca2

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

mzient reviewed Apr 21, 2022

View reviewed changes

Review remarks: more vectoried methods in tests

6f27fc0

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan added 2 commits April 21, 2022 15:17

Split params provider, describe input_params arg

3853957

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Fix test docs

4600d01

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

mzient approved these changes Apr 21, 2022

View reviewed changes

prak-nv approved these changes Apr 25, 2022

View reviewed changes

stiepan merged commit f41733b into NVIDIA:main Apr 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rotate per-frame #3820

Rotate per-frame #3820

stiepan commented Apr 13, 2022 •

edited

mzient Apr 20, 2022 •

edited

stiepan Apr 20, 2022

mzient Apr 20, 2022

stiepan Apr 21, 2022

stiepan commented Apr 20, 2022

dali-automaton commented Apr 20, 2022

mzient Apr 20, 2022 •

edited

stiepan Apr 21, 2022

mzient Apr 20, 2022 •

edited

stiepan Apr 21, 2022

mzient Apr 20, 2022 •

edited

stiepan Apr 21, 2022

mzient Apr 20, 2022 •

edited

stiepan Apr 21, 2022

dali-automaton commented Apr 20, 2022

mzient Apr 21, 2022 •

edited

stiepan Apr 21, 2022

mzient Apr 21, 2022

stiepan Apr 21, 2022

mzient Apr 21, 2022

stiepan Apr 21, 2022

mzient Apr 21, 2022

stiepan Apr 21, 2022

mzient Apr 21, 2022

stiepan Apr 21, 2022 •

edited

stiepan commented Apr 25, 2022

dali-automaton commented Apr 25, 2022

dali-automaton commented Apr 25, 2022

dali-automaton commented Apr 25, 2022

		// kernels::vec2shape reverses the extents, store them in that order in parity vector
		parity[2 - i] = in_size[dominant_src_axis[i]] % 2;

	max_shape = maximum_array(no_correction_shapes)
	max_shape = np.max(no_correction_shapes, axis=0) # elementwise maximum

		parity = sum([np.array([extent % 2 for extent in shape], dtype=np.int32)
		for shape in corrected_shapes])

	parity = sum([np.array([extent % 2 for extent in shape], dtype=np.int32)
	for shape in corrected_shapes])
	parity = np.sum(np.array(corrected_shapes, dtype=np.int32) % 2, axis=0)



		def get_3d_output_size(angle, axis, input_size, parity_correction=False):
		rotation = np.abs(get_3d_lin_rotation(angle, axis))

	return np.array(list(reversed(out_size)), dtype=np.int32)
	return out_size[::-1]

	def __init__(self, input_params):
	def __init__(self, **input_params : ParamDesc):

Rotate per-frame #3820

Rotate per-frame #3820

Conversation

stiepan commented Apr 13, 2022 • edited

Category:

Description:

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Checklist

Tests

Documentation

DALI team only

Requirements

mzient Apr 20, 2022 • edited

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

stiepan commented Apr 20, 2022

dali-automaton commented Apr 20, 2022

mzient Apr 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient Apr 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient Apr 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient Apr 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dali-automaton commented Apr 20, 2022

mzient Apr 21, 2022 • edited

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

stiepan Apr 21, 2022 • edited

Choose a reason for hiding this comment

stiepan commented Apr 25, 2022

dali-automaton commented Apr 25, 2022

dali-automaton commented Apr 25, 2022

dali-automaton commented Apr 25, 2022

stiepan commented Apr 13, 2022 •

edited

mzient Apr 20, 2022 •

edited

mzient Apr 20, 2022 •

edited

mzient Apr 20, 2022 •

edited

mzient Apr 20, 2022 •

edited

mzient Apr 20, 2022 •

edited

mzient Apr 21, 2022 •

edited

stiepan Apr 21, 2022 •

edited