Tensor indexing #3195

mzient · 2021-07-28T19:11:00Z

Signed-off-by: Michał Zientkiewicz mzient@gmail.com

Description

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactoring (Redesign of existing code that doesn't affect functionality)
Other (e.g. Documentation, Tests, Configuration)

What happened in this PR

Added NumPy-like tensor indexing operator:

the operator supports up to 32 dimensions with at, lo, hi (and in the future also step) arguments.
the arguments are passed separately for dimensions
adds __getitem__ to DataNode which instantiates subscript operator
adds dali.newaxis and dali.newaxis(name)
fixes a bug in TensorVector: stale type in sample views when only the type changes in Resize
fixes a bug in ExpandDims: incorrenct layout handling for 0D input

Additional information

Affected modules and functionalities:

DataNode python object
Operators: added a new operator
ExpandDims - bug fix
TensorVector - bug fix

Key points relevant for the review:

Indexing corner cases.

Checklist

Tests

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: SUBSCR.* except SUBSCR.06

JIRA TASK: 1617

dali-automaton · 2021-07-28T23:42:31Z

CI MESSAGE: [2659393]: BUILD STARTED

dali-automaton · 2021-07-29T02:16:15Z

CI MESSAGE: [2659393]: BUILD FAILED

dali-automaton · 2021-07-29T10:07:17Z

CI MESSAGE: [2661776]: BUILD STARTED

dali-automaton · 2021-07-29T11:37:42Z

CI MESSAGE: [2661776]: BUILD PASSED

dali/test/python/test_dali_variable_batch_size.py

mzient · 2021-07-29T12:55:35Z

dali/pipeline/data/tensor_vector.cc

+  } else if (IsValidType(tl_->type())) {
+    tensors_[idx]->set_type(tl_->type());


This kept stale type in the views if the pointer or size didn't change.

We have Reset above, so it happened only with empty TV that got a type?

It happend with something that had non-empty shape and type but 0 volume. And it was obviously a problem in TensorVector, since it had internally inconsistent type (TV had no_type and TLV had a proper type).

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

* Add layout handling * Add Python front-end for slicing Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

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

… set on views when pointer or size is not changed. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

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

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

dali/operators/generic/slice/subscript.h

JanuszL · 2021-07-29T14:08:40Z

dali/operators/generic/slice/subscript.h

+        DALI_FAIL(make_string("The subscriptg for dimension ", i,
+                              " is specified both as an index and as a range."));


Suggested change

DALI_FAIL(make_string("The subscriptg for dimension ", i,

" is specified both as an index and as a range."));

DALI_FAIL(make_string("The subscriptg for dimension ", i,

" should not be specified both as an index and as a range."));

must not ;)

JanuszL · 2021-07-29T14:09:06Z

dali/operators/generic/slice/subscript.h

+      }
+      if (step.IsDefined()) {
+        DALI_FAIL(make_string("The subscriptg for dimension ", i,
+                              " is specified as an index - it cannot have a step."));


Suggested change

" is specified as an index - it cannot have a step."));

" should not be specified as an index and have have a step."));

JanuszL · 2021-07-29T14:15:50Z

dali/operators/generic/slice/subscript.h

+
+
+      if (s.IsDefined()) {
+        num_subscripts = i + 1;


Suggested change

num_subscripts = i + 1;

max_subscripts = i + 1;

As it is not a number because some subscripts may be not defined.

Mixed feelings here. It's used as the number (see subscripts_.resize(num_subscripts);). I would gladly use input ndim here if I had it - but it's not available at construction.

JanuszL · 2021-07-29T14:17:15Z

dali/operators/generic/slice/subscript.h

+  void GetRanges(const workspace_t<Backend> &ws, const TensorListShape<> &in_shape) {
+    int nsub = subscripts_.size();
+    int ndim = in_shape.sample_dim();
+    DALI_ENFORCE(ndim >= nsub, "Too many indices.");


Suggested change

DALI_ENFORCE(ndim >= nsub, "Too many indices.");

DALI_ENFORCE(ndim >= nsub, make_string("Too many indices. Received: ", nsub, ", while input has only " ndim, " dimensions.");

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

lgtm-com · 2021-08-02T16:33:59Z

This pull request introduces 1 alert when merging 981923a into db01018 - view on LGTM.com

new alerts:

1 for Unused import

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

dali-automaton · 2021-08-02T17:48:37Z

CI MESSAGE: [2681926]: BUILD STARTED

dali-automaton · 2021-08-02T18:47:37Z

CI MESSAGE: [2681926]: BUILD FAILED

JanuszL · 2021-08-02T18:52:09Z

dali/test/python/test_pipeline.py

@@ -1609,6 +1609,21 @@ def my_pipe():

    p = my_pipe(device_id=0, seed=1234, num_threads=3, set_affinity=True, py_num_workers=3)

+def test_not_iterable():
+    import nvidia.dali._utils.hacks as hacks


Why hacks? Workarounds?

JanuszL · 2021-08-02T19:10:47Z

dali/test/python/test_operator_subscript.py

+            assert np.array_equal(ref, cpu.at(i))
+            assert np.array_equal(ref, gpu.as_cpu().at(i))
+
+def test_constant_ranges():


I think it repeats L56 and hides it.

Some copy paste... I'll take a look.

dali/test/python/test_operator_subscript.py

dali/python/nvidia/dali/data_node.py

JanuszL · 2021-08-02T19:21:21Z

dali/python/nvidia/dali/data_node.py

+        import nvidia.dali.fn
+        if len(slice_args) == 0:
+            if len(new_axes) == 0 or new_axes[-1] < len(idxs):
+                print("Adding dim check for ", len(idxs))


Do we need these prints?

Surely not. These are remnants from initial testing - I wanted to be 100% sure that the code follows the intended path and that the right checks fire when expected.

dali/operators/generic/slice/subscript.h

* Fix some tests. * Fix review issues. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

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

dali-automaton · 2021-08-03T12:55:21Z

CI MESSAGE: [2687330]: BUILD STARTED

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

dali-automaton · 2021-08-03T14:16:04Z

CI MESSAGE: [2687741]: BUILD STARTED

dali-automaton · 2021-08-03T14:22:01Z

CI MESSAGE: [2687330]: BUILD FAILED

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

dali-automaton · 2021-08-03T14:26:17Z

CI MESSAGE: [2687788]: BUILD STARTED

dali-automaton · 2021-08-03T15:42:41Z

CI MESSAGE: [2687788]: BUILD PASSED

mzient force-pushed the SubscriptOp branch 2 times, most recently from b2f5f28 to 5223763 Compare July 28, 2021 23:14

mzient marked this pull request as ready for review July 28, 2021 23:40

JanuszL self-assigned this Jul 29, 2021

klecki self-assigned this Jul 29, 2021

JanuszL reviewed Jul 29, 2021

View reviewed changes

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

mzient commented Jul 29, 2021

View reviewed changes

mzient requested a review from JanuszL July 29, 2021 13:47

mzient added 11 commits July 29, 2021 15:51

CPU tensor indexing.

218c1ba

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

Add python wrapper for slicing.

039fd7a

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

* Add GPU subscript operator

48e3326

* Add layout handling * Add Python front-end for slicing Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Reduce max. non-collapsible dims to 16.

24e4c9c

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

Fix output layout calculation.

771da06

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

Enable tensor arguments.

6269261

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

Add tests. Fix ExpandDims for 0D input and nonempty new_axis_names.

887cf40

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

Add cpu-only and variable batch size tests.

6f28e43

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

Fix behaviour with swapped ends. Fix a bug in TensorVector - type not…

bb15eb9

… set on views when pointer or size is not changed. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Fix out-of-range error message. Add tests for out-of-range and incons…

0ac474e

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

Add missing file with tests.

847f53e

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

mzient force-pushed the SubscriptOp branch from 744cf99 to 847f53e Compare July 29, 2021 13:51

NVIDIA deleted a comment from dali-automaton Jul 29, 2021

JanuszL reviewed Jul 29, 2021

View reviewed changes

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

JanuszL reviewed Jul 29, 2021

View reviewed changes

mzient added 2 commits August 2, 2021 18:07

Add more tests.

2391452

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

Add comments.

981923a

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

Fix Python LGTM.

2726dd1

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

JanuszL reviewed Aug 2, 2021

View reviewed changes

dali/test/python/test_operator_subscript.py Outdated Show resolved Hide resolved

JanuszL reviewed Aug 2, 2021

View reviewed changes

dali/python/nvidia/dali/data_node.py Outdated Show resolved Hide resolved

JanuszL reviewed Aug 2, 2021

View reviewed changes

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

JanuszL reviewed Aug 2, 2021

View reviewed changes

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

mzient added 2 commits August 3, 2021 11:35

* Add message check to error tests.

f9eab5a

* Fix some tests. * Fix review issues. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Remove print.

6e878c8

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

JanuszL approved these changes Aug 3, 2021

View reviewed changes

klecki approved these changes Aug 3, 2021

View reviewed changes

Fix trailin full-range handling.

d93320a

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

mzient force-pushed the SubscriptOp branch from 41bcf97 to d93320a Compare August 3, 2021 14:06

klecki approved these changes Aug 3, 2021

View reviewed changes

Add comments.

a6d6e43

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

NVIDIA deleted a comment from dali-automaton Aug 3, 2021

Add subscript_dim_check to cpu_only and variable_batch_size tests.

13961ae

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

mzient merged commit df1e6c3 into NVIDIA:main Aug 3, 2021

JanuszL mentioned this pull request Sep 1, 2021

DALI 2021 roadmap #2978

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tensor indexing #3195

Tensor indexing #3195

mzient commented Jul 28, 2021 •

edited

dali-automaton commented Jul 28, 2021

dali-automaton commented Jul 29, 2021

dali-automaton commented Jul 29, 2021

dali-automaton commented Jul 29, 2021

mzient Jul 29, 2021

klecki Jul 30, 2021

mzient Aug 2, 2021 •

edited

JanuszL Jul 29, 2021

mzient Jul 29, 2021

JanuszL Jul 29, 2021

JanuszL Jul 29, 2021

mzient Jul 29, 2021

JanuszL Jul 29, 2021

lgtm-com bot commented Aug 2, 2021

dali-automaton commented Aug 2, 2021

dali-automaton commented Aug 2, 2021

JanuszL Aug 2, 2021

JanuszL Aug 2, 2021

mzient Aug 3, 2021

JanuszL Aug 2, 2021

mzient Aug 3, 2021

dali-automaton commented Aug 3, 2021

dali-automaton commented Aug 3, 2021

dali-automaton commented Aug 3, 2021

dali-automaton commented Aug 3, 2021

dali-automaton commented Aug 3, 2021

		} else if (IsValidType(tl_->type())) {
		tensors_[idx]->set_type(tl_->type());

		DALI_FAIL(make_string("The subscriptg for dimension ", i,
		" is specified both as an index and as a range."));

	" is specified as an index - it cannot have a step."));
	" should not be specified as an index and have have a step."));

	DALI_ENFORCE(ndim >= nsub, "Too many indices.");
	DALI_ENFORCE(ndim >= nsub, make_string("Too many indices. Received: ", nsub, ", while input has only " ndim, " dimensions.");

Tensor indexing #3195

Tensor indexing #3195

Conversation

mzient commented Jul 28, 2021 • edited

Description

What happened in this PR

Additional information

Affected modules and functionalities:

Key points relevant for the review:

Checklist

Tests

Documentation

DALI team only

Requirements

dali-automaton commented Jul 28, 2021

dali-automaton commented Jul 29, 2021

dali-automaton commented Jul 29, 2021

dali-automaton commented Jul 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient Aug 2, 2021 • 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

lgtm-com bot commented Aug 2, 2021

dali-automaton commented Aug 2, 2021

dali-automaton commented Aug 2, 2021

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

dali-automaton commented Aug 3, 2021

dali-automaton commented Aug 3, 2021

dali-automaton commented Aug 3, 2021

dali-automaton commented Aug 3, 2021

dali-automaton commented Aug 3, 2021

mzient commented Jul 28, 2021 •

edited

mzient Aug 2, 2021 •

edited