Pad operator: Add support for per-sample shape and alignment requirements #2432

jantonguirao · 2020-11-03T13:29:43Z

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

Why we need this PR?

It adds new feature needed to support different Pad shape/align requirements per sample

What happened in this PR?

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

What solution was applied:
Pad operator: shape/align to support argument inputs
(Bonus, needed for testing) Uniform operator: shape to support argument inputs
Affected modules and functionalities:
Pad, Uniform
Key points relevant for the review:
N/A
Validation and testing:
Tests added
Documentation (including examples):
N/A

JIRA TASK: [DALI-1669]

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

dali/operators/generic/pad.cu

@@ -31,6 +31,8 @@ bool Pad<GPUBackend>::SetupImpl(std::vector<OutputDesc> &output_desc,
  int ndim = in_shape.sample_dim();
  int nsamples = in_shape.num_samples();

+  this->ReadArguments(spec_, ws);


gave me an error without this->

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

mzient · 2020-11-03T13:54:21Z

dali/operators/generic/pad.h

-    if (remainder > 0)
-      extent += alignment - remainder;
-    return extent;
+  void ReadShapeListArg(std::vector<TensorShape<>> &out, const std::string &arg_name,


Can't you use GetShapeArgument from operator/common.h?

mzient · 2020-11-03T13:56:15Z

dali/operators/random/uniform.h

    output_desc[0].type = TypeTable::GetTypeInfo(DALI_FLOAT);
+    auto& sh = output_desc[0].shape;
+    if (spec_.HasTensorArgument("shape")) {


Likewise - use GetShapeArgument?

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

mzient · 2020-11-03T17:34:26Z

dali/pipeline/operator/common.h

-  out_tls.resize(out_tls.shape().num_samples / ndim, ndim);
+  ndim = GetShapeLikeArgument<ArgumentType>(out_tls.shapes, spec, argument_name,
+                                            ws, ndim, batch_size);
+  out_tls.resize(ndim == 0 ? 0 : out_tls.shapes.size() / ndim, ndim);


This is wrong (and it's my fault). Please change semantics of GetShapeLikeArgument to return number of samples, not dimensions (or both, perhaps). The return value is not used anywhere except tests, so it won't break anything.

mzient · 2020-11-04T07:46:30Z

dali/operators/random/uniform.cc

@@ -75,7 +75,7 @@ This argument is mutually exclusive with ``values``.
 This argument is mutually exclusive with ``range``.
 )code", std::vector<float>({}))
  .AddOptionalArg("shape",
-    R"code(Shape of the samples.)code", std::vector<int>{1});
+    R"code(Shape of the samples.)code", std::vector<int>{1}, true);


Suggested change

R"code(Shape of the samples.)code", std::vector<int>{1}, true);

R"code(Shape of the samples.)code", std::vector<int>{}, true);

The default has been changed to generate scalars, not 1-element 1D tensors. It should stay like this!

mzient · 2020-11-04T13:08:17Z

include/dali/core/tensor_shape.h

+  template <typename It,
+            typename = std::enable_if<is_integer_iterator<It>::value>>
+  TensorShape(It first, It last) {
+    int sz = last - first;


Suggested change

int sz = last - first;

std::ptrdiff_t sz = last - first;

although I don't know if we need to require contiguity at all

mzient · 2020-11-04T15:00:43Z

dali/pipeline/operator/common.h

+std::pair<int, int> GetShapeArgument(TensorListShape<out_ndim> &out_tls, const OpSpec &spec,
+                                     const std::string &argument_name, const ArgumentWorkspace &ws,


Why the formatting change? This grouping seems random.

CTRL+F did that. I can restore

mzient · 2020-11-04T15:01:00Z

dali/pipeline/operator/common.h

+std::pair<int, int> GetShapeLikeArgument(std::vector<ExtentType> &out_shape, const OpSpec &spec,
+                                         const std::string &argument_name,
+                                         const ArgumentWorkspace &ws, int ndim = -1,
+                                         int batch_size = -1 /* -1 = get from "batch_size" arg */) {


Random grouping of arguments.

mzient · 2020-11-04T15:03:40Z

dali/operators/random/uniform.h

+    if (spec_.ArgumentDefined("shape")) {
+      GetShapeArgument(output_desc[0].shape, spec_, "shape", ws, -1, batch_size_);
+    } else {
+      output_desc[0].shape = uniform_list_shape(batch_size_, TensorShape<>{1});


Suggested change

output_desc[0].shape = uniform_list_shape(batch_size_, TensorShape<>{1});

output_desc[0].shape = uniform_list_shape(batch_size_, TensorShape<0>());

This is a bug. Please fix!

mzient · 2020-11-04T15:32:06Z

include/dali/core/tensor_shape.h

+  template <typename It,
+            typename = std::enable_if<is_integer_iterator<It>::value>>
+  TensorShape(It first, It last) : Base() {


missing:
DALI_NO_EXEC_CHECK
DALI_HOST_DEV
Unnecessary explicit invocation of base's default constructor.

mzient · 2020-11-04T15:32:36Z

include/dali/core/tensor_shape.h

+  template <typename Collection,
+            typename = std::enable_if_t<is_integer_collection<Collection>::value>>
+  TensorShape(const Collection &c)  // NOLINT(runtime/explicit)
+    : TensorShape(dali::begin(c), dali::end(c)) {}


DALI_NO_EXEC_CHECK
DALI_HOST_DEV

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

jantonguirao · 2020-11-04T16:14:23Z

!build

dali-automaton · 2020-11-04T16:20:52Z

CI MESSAGE: [1762284]: BUILD STARTED

dali-automaton · 2020-11-04T18:15:59Z

CI MESSAGE: [1762284]: BUILD FAILED

JanuszL · 2020-11-04T21:39:17Z

dali/pipeline/operator/common.h

@@ -197,19 +198,20 @@ int GetShapeLikeArgument(std::vector<ExtentType> &out_shape,
        out_shape[i * ndim + d] = to_extent(tsvec[d]);
  }

-  return ndim;
+  return {batch_size, ndim};


Do we need to return pair?
Shape it fill is accepted as an argument, so we can do that with batch_size and ndim as well.
ret.first, ret.second is not very self descriptive.

The issue is that there are already input ndim and batch size arguments that can be omitted or in the case of ndim, also inferred from the type (TensorShape<ndim>). IMHO it would be too messy to use those both as optional inputs and also as outputs, that's why I am using return

I just wonder if this is not too pythonic. But sure.

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

jantonguirao · 2020-11-05T09:01:43Z

!build

dali-automaton · 2020-11-05T09:05:27Z

CI MESSAGE: [1765042]: BUILD STARTED

dali-automaton · 2020-11-05T11:00:53Z

CI MESSAGE: [1765042]: BUILD PASSED

jantonguirao commented Nov 3, 2020

View reviewed changes

jantonguirao force-pushed the pad_arg_input_shape branch 3 times, most recently from ee671f3 to 8e04c95 Compare November 3, 2020 13:39

Pad operator: Add support for per-sample shape and alignment requirem…

2427804

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

jantonguirao force-pushed the pad_arg_input_shape branch from 8e04c95 to 2427804 Compare November 3, 2020 13:41

mzient reviewed Nov 3, 2020

View reviewed changes

Code review fixes

d545d45

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

mzient reviewed Nov 3, 2020

View reviewed changes

JanuszL approved these changes Nov 3, 2020

View reviewed changes

mzient reviewed Nov 4, 2020

View reviewed changes

jantonguirao force-pushed the pad_arg_input_shape branch 4 times, most recently from fc34aa9 to ec781ee Compare November 4, 2020 14:59

mzient reviewed Nov 4, 2020

View reviewed changes

jantonguirao force-pushed the pad_arg_input_shape branch from ec781ee to 5da2d20 Compare November 4, 2020 15:26

mzient reviewed Nov 4, 2020

View reviewed changes

jantonguirao force-pushed the pad_arg_input_shape branch 2 times, most recently from 3560aa2 to 17287f5 Compare November 4, 2020 15:51

Code review fixes

939d164

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

jantonguirao force-pushed the pad_arg_input_shape branch from 17287f5 to 939d164 Compare November 4, 2020 15:51

mzient approved these changes Nov 4, 2020

View reviewed changes

JanuszL reviewed Nov 4, 2020

View reviewed changes

JanuszL approved these changes Nov 4, 2020

View reviewed changes

Fixes

4b98129

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

jantonguirao merged commit db2df9f into NVIDIA:master Nov 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pad operator: Add support for per-sample shape and alignment requirements #2432

Pad operator: Add support for per-sample shape and alignment requirements #2432

jantonguirao commented Nov 3, 2020

jantonguirao Nov 3, 2020

mzient Nov 3, 2020

mzient Nov 3, 2020

mzient Nov 3, 2020 •

edited

Loading

mzient Nov 4, 2020

mzient Nov 4, 2020

mzient Nov 4, 2020

jantonguirao Nov 4, 2020

mzient Nov 4, 2020

mzient Nov 4, 2020

mzient Nov 4, 2020

jantonguirao Nov 4, 2020

mzient Nov 4, 2020

mzient Nov 4, 2020

jantonguirao commented Nov 4, 2020

dali-automaton commented Nov 4, 2020

dali-automaton commented Nov 4, 2020

JanuszL Nov 4, 2020

jantonguirao Nov 5, 2020

JanuszL Nov 5, 2020

jantonguirao commented Nov 5, 2020

dali-automaton commented Nov 5, 2020

dali-automaton commented Nov 5, 2020

	R"code(Shape of the samples.)code", std::vector<int>{1}, true);
	R"code(Shape of the samples.)code", std::vector<int>{}, true);

		std::pair<int, int> GetShapeArgument(TensorListShape<out_ndim> &out_tls, const OpSpec &spec,
		const std::string &argument_name, const ArgumentWorkspace &ws,

	output_desc[0].shape = uniform_list_shape(batch_size_, TensorShape<>{1});
	output_desc[0].shape = uniform_list_shape(batch_size_, TensorShape<0>());

Pad operator: Add support for per-sample shape and alignment requirements #2432

Pad operator: Add support for per-sample shape and alignment requirements #2432

Conversation

jantonguirao commented Nov 3, 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

mzient Nov 3, 2020 • 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

jantonguirao commented Nov 4, 2020

dali-automaton commented Nov 4, 2020

dali-automaton commented Nov 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jantonguirao commented Nov 5, 2020

dali-automaton commented Nov 5, 2020

dali-automaton commented Nov 5, 2020

mzient Nov 3, 2020 •

edited

Loading