Add operators for batch reordering #2417

mzient · 2020-10-29T20:29:35Z

BatchPermutation - obtains random indices of samples
PermuteBatch - reorders tensors within a batch according to given list of indices.

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

Why we need this PR?

Pick one, remove the rest

It adds new feature needed for mosaicing, soft-labels, etc

What happened in this PR?

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

What solution was applied:
- BatchPermutation operator - generate a batch of scalars ranged from 0 to batch_size-1
- PermuteBatch operator - copies tensors from input at given index
Affected modules and functionalities:
- N/A
Key points relevant for the review:
- N/A
Validation and testing:
- Python test
Documentation (including examples):
- Docstrings, no examples.

JIRA TASK: DALI-1706

- BatchPermutation - obtains random indices of samples - PermuteBatch - reorders tensors within a batch. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

JanuszL · 2020-10-30T09:13:32Z

dali/operators/generic/permute_batch.cc

+    int src = indices_[i];
+    tp.AddWork([&, i, src, size](int tid) {
+      output.SetMeta(i, input.GetMeta(i));
+      memcpy(output[i].raw_mutable_data(), input[src].raw_data(), size);


Why not:

Suggested change

memcpy(output[i].raw_mutable_data(), input[src].raw_data(), size);

cudaStream_t stream = 0;

output[i].Copy(input[src], stream);

I had some problems with it, but I think it turned out to be something else. I can give it another shot.

JanuszL · 2020-10-30T09:19:24Z

dali/operators/random/batch_permutation.cc

+      R"(If true, the output can contain repetitions and omissions.)", false);
+
+void BatchPermutation::NoRepetitions(int N) {
+  tmp_out_.resize(N);


Can you make the tmp_out_ an argument?

JanuszL · 2020-10-30T09:19:54Z

dali/operators/random/batch_permutation.cc

+  else
+    NoRepetitions(N);
+  for (int i = 0; i < N; ++i) {
+    out_view.data[i][0] = tmp_out_[i];


tmp_out_ comes out of the blue, see my previous comment.

mzient · 2020-10-30T12:47:08Z

!build

dali-automaton · 2020-10-30T12:50:32Z

CI MESSAGE: [1748765]: BUILD STARTED

Add core/random.h include file for random sequence generators. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

dali-automaton · 2020-10-30T13:40:37Z

CI MESSAGE: [1748765]: BUILD FAILED

jantonguirao · 2020-10-30T13:43:24Z

dali/operators/generic/permute_batch.cc

+  .AddArg("indices", R"(List of indices, matching current batch size, or a batch
+of scalars representing indices of the tensors in the input batch.
+
+The indices must be within ``[0..batch_size)`` range. Repetitions and omissions are allowed.)",


Suggested change

The indices must be within ``[0..batch_size)`` range. Repetitions and omissions are allowed.)",

The indices must be within ``[0, batch_size)`` range. Repetitions and omissions are allowed.)",

jantonguirao · 2020-10-30T13:45:03Z

dali/operators/generic/permute_batch.cc

+  auto &tp = ws.GetThreadPool();
+  int N = indices_.size();
+  for (int i = 0; i < N; i++) {
+    auto size = volume(output_shape.tensor_shape_span(i));


we have

auto size = output_shape.tensor_size(i);

jantonguirao · 2020-10-30T13:45:45Z

dali/operators/generic/permute_batch.cc

+    auto size = volume(output_shape.tensor_shape_span(i));
+    int src = indices_[i];
+    tp.AddWork([&, i, src](int tid) {
+      output.SetMeta(i, input.GetMeta(i));


I think Copy should also copy the meta. Isn't it the case?

SetMeta will set it on the underlying TensorList, if the TensorVector is in a contiguous state - but there's a bug, it should be GetMeta(src).

jantonguirao · 2020-10-30T13:46:38Z

dali/operators/generic/permute_batch.cc

+  int element_size = output.type().size();
+
+  for (int i = 0; i < N; i++) {
+    auto size = volume(out_shape.tensor_shape_span(i)) * element_size;


Suggested change

auto size = volume(out_shape.tensor_shape_span(i)) * element_size;

auto size = out_shape.tensor_size(i) * element_size;

jantonguirao · 2020-10-30T13:47:19Z

dali/operators/generic/permute_batch.h

+
+  bool SetupImpl(vector<OutputDesc> &outputs, const workspace_t<Backend> &ws) override {
+    outputs.resize(1);
+    auto &input = ws.template InputRef<Backend>(0);


Suggested change

auto &input = ws.template InputRef<Backend>(0);

const auto &input = ws.template InputRef<Backend>(0);

to make sure we don't magically change the type of the input

In setup it won't happen, because the workspace is const-qualified. I'll change it in Run, though.

jantonguirao · 2020-10-30T13:50:04Z

dali/operators/generic/permute_batch.h

+ public:
+  explicit PermuteBatch(const OpSpec &spec)
+  : PermuteBatchBase<GPUBackend>(spec)
+  , sg_(1<<18, spec.GetArgument<int>("batch_size")) {}


Can you run scatter gather with fewer samples than requested here? In other words, is this specifying max size?

jantonguirao · 2020-10-30T13:57:28Z

include/dali/core/random.h

+        out[i] = x;
+    }
+  }
+  // we're above hi now - no fixed points posisble


Suggested change

// we're above hi now - no fixed points posisble

// we're above hi now - no fixed points possible

jantonguirao · 2020-10-30T14:00:18Z

dali/test/python/test_operator_batch_permute.py

+    pipe.set_outputs(data, fn.permute_batch(data, indices=perm), perm)
+    pipe.build()
+
+    for i in range(10):


Suggested change

for i in range(10):

num_iters = 10

for i in range(10):

Maybe make it an argument of the function.

It doesn't matter - in general, the more the better and there's no need to make it a test argument.

Fix support for non-tensor permutation. Add test for raising errors for out-of-bounds tensor index. Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

mzient · 2020-10-30T15:08:54Z

!build

dali-automaton · 2020-10-30T15:10:37Z

CI MESSAGE: [1749098]: BUILD STARTED

dali-automaton · 2020-10-30T16:57:58Z

CI MESSAGE: [1749098]: BUILD PASSED

Added operators:

c918f9b

- BatchPermutation - obtains random indices of samples - PermuteBatch - reorders tensors within a batch. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient requested a review from a team October 29, 2020 20:29

JanuszL reviewed Oct 30, 2020

View reviewed changes

mzient force-pushed the PermuteBatch branch from d5caf77 to d143e12 Compare October 30, 2020 11:49

JanuszL approved these changes Oct 30, 2020

View reviewed changes

Address review issues.

009e36d

Add core/random.h include file for random sequence generators. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient force-pushed the PermuteBatch branch from d143e12 to 009e36d Compare October 30, 2020 13:40

jantonguirao approved these changes Oct 30, 2020

View reviewed changes

mzient force-pushed the PermuteBatch branch from 6e080f0 to 5a0e3a2 Compare October 30, 2020 15:04

Address review issues.

9acd223

Fix support for non-tensor permutation. Add test for raising errors for out-of-bounds tensor index. Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

mzient force-pushed the PermuteBatch branch from 5a0e3a2 to 9acd223 Compare October 30, 2020 15:06

mzient merged commit 8234d35 into NVIDIA:master Oct 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add operators for batch reordering #2417

Add operators for batch reordering #2417

mzient commented Oct 29, 2020

JanuszL Oct 30, 2020

mzient Oct 30, 2020

JanuszL Oct 30, 2020

mzient Oct 30, 2020

JanuszL Oct 30, 2020

mzient Oct 30, 2020

mzient commented Oct 30, 2020

dali-automaton commented Oct 30, 2020

dali-automaton commented Oct 30, 2020

jantonguirao Oct 30, 2020

jantonguirao Oct 30, 2020

jantonguirao Oct 30, 2020

mzient Oct 30, 2020

jantonguirao Oct 30, 2020

jantonguirao Oct 30, 2020

mzient Oct 30, 2020

jantonguirao Oct 30, 2020

mzient Oct 30, 2020

jantonguirao Oct 30, 2020

jantonguirao Oct 30, 2020

mzient Oct 30, 2020

mzient commented Oct 30, 2020

dali-automaton commented Oct 30, 2020

dali-automaton commented Oct 30, 2020

	memcpy(output[i].raw_mutable_data(), input[src].raw_data(), size);
	cudaStream_t stream = 0;
	output[i].Copy(input[src], stream);

	The indices must be within ``[0..batch_size)`` range. Repetitions and omissions are allowed.)",
	The indices must be within ``[0, batch_size)`` range. Repetitions and omissions are allowed.)",

	auto size = volume(out_shape.tensor_shape_span(i)) * element_size;
	auto size = out_shape.tensor_size(i) * element_size;

	auto &input = ws.template InputRef<Backend>(0);
	const auto &input = ws.template InputRef<Backend>(0);

	// we're above hi now - no fixed points posisble
	// we're above hi now - no fixed points possible

Add operators for batch reordering #2417

Add operators for batch reordering #2417

Conversation

mzient commented Oct 29, 2020

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

mzient commented Oct 30, 2020

dali-automaton commented Oct 30, 2020

dali-automaton commented Oct 30, 2020

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

mzient commented Oct 30, 2020

dali-automaton commented Oct 30, 2020

dali-automaton commented Oct 30, 2020