Improved Reshape #1634

mzient · 2020-01-09T14:53:05Z

Add relative extents
Add "remaining volume" extent (-1) support

Signed-off-by: Michal Zientkiewicz michalz@nvidia.com

Why we need this PR?

Pick one, remove the rest

It adds new feature needed for RNN-t feature frame splicing

What happened in this PR?

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

What solution was applied:
- Add rel_shape to Reshape parameter which accepts (floating point) relative shapes
- Interpret negative extent as "all remaining volume" - it's calculated as a ratio of source tensor volume to the product of explicitly defined extents
Affected modules and functionalities:
- Reshape operator
Key points relevant for the review:
- reshape.cc
Validation and testing:
- Python test
Documentation (including examples):
- Parameter docstrings

JIRA TASK: N/A

* Add relative extents * Add "remaining volume" extent (-1) support Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

mzient · 2020-01-09T14:53:13Z

!build

dali-automaton · 2020-01-09T14:55:28Z

CI MESSAGE: [1064365]: BUILD STARTED

jantonguirao · 2020-01-09T15:22:58Z

dali/operators/util/reshape.cc

-                                "is preserved if number of dimension matches existing layout "
-                                "or reset to empty otherwise",
-                                "");
+  .AddOptionalArg<float>("rel_shape", "The relative shape of the output. Number of dimensions "


you should point out that "shape" and "rel_shape" are mutually exclusive

jantonguirao · 2020-01-09T15:35:22Z

dali/operators/util/reshape.cc

+  bool has_rel_shape_arg = spec.HasArgument("rel_shape") || spec.HasTensorArgument("rel_shape");
+  DALI_ENFORCE(has_shape_input + has_shape_arg + has_rel_shape_arg == 1,
+    "Reshape: shape input, `shape` argument and `rel_shape` argument are mutually exclusive");
+  DALI_ENFORCE(has_shape_input || has_shape_arg || has_rel_shape_arg || has_layout_arg,


Note: you are already checking that at least one shape argument is provided, you don't need to check that again.

There's a bug - it should be <= 1.

dali-automaton · 2020-01-09T15:42:11Z

CI MESSAGE: [1064365]: BUILD FAILED

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

mzient · 2020-01-09T16:20:30Z

!build

dali-automaton · 2020-01-09T16:25:10Z

CI MESSAGE: [1064465]: BUILD STARTED

dali-automaton · 2020-01-09T17:11:13Z

CI MESSAGE: [1064465]: BUILD PASSED

klecki · 2020-01-10T10:58:06Z

dali/operators/util/reshape.cc

+    TYPE_SWITCH(tl.type().id(), type2id, type,
+      (int8_t, uint8_t, int16_t, uint16_t, int32_t, uint32_t, int64_t, uint64_t),
+      (this->ShapeFromInput(view<const type>(tl));),
+      (DALI_FAIL("Reshape: shape input must have integral type; got: " + tl.type().name() +


must have integral type unless it's the if above and we handle float.

Shape input is only for absolute shape. Relative shape is specified as an argument input.
The regular input is only here for historical reasons (from before CPU/support unification).

klecki · 2020-01-10T11:02:19Z

dali/operators/util/reshape.cc

@@ -28,34 +29,75 @@ DALI_SCHEMA(Reshape)
  .AllowSequences()
  .SupportVolumetric()
  .AddOptionalArg<int>("shape", "The desired shape of the output. Number of elements in "


It would be nice to provide the information, that Reshape can also take optional input with shapes. Now there is no such information.

klecki · 2020-01-10T11:03:11Z

dali/operators/util/reshape.cc

      break;
    case ShapeSource::Input:
-      ShapeFromInput(ws.template InputRef<CPUBackend>(1));
+      ShapeFromInput(ws.template InputRef<CPUBackend>(1), use_rel_shape_);


This won't ever work with relative shapes provided as inputs, as even if we get second input with float values, we won't set use_rel_shape_=true.

Also, does it actually work on GPU to receive the shapes as CPU input? (It's not like it will be widely used due to it's limitations).

It will always be false.

It does work and is actually tested.

mzient · 2020-01-10T11:51:01Z

dali/operators/util/reshape.cc

+      if (use_rel_shape_)
+        ShapeFromInput(ws.ArgumentInput("shape"), false);
+      else
+        ShapeFromInput(ws.ArgumentInput("rel_shape"), false);


...lol, bug.

mzient · 2020-01-10T12:58:06Z

!build

dali-automaton · 2020-01-10T13:00:27Z

CI MESSAGE: [1066198]: BUILD STARTED

klecki · 2020-01-10T13:16:10Z

dali/test/python/test_operator_reshape.py

-  for test in test_reshape_arg():
-    test[0](*test[1:])
-  for test in test_reshape_input():
+  #for test in test_reshape_arg():


Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

mzient · 2020-01-10T13:57:52Z

!build

dali-automaton · 2020-01-10T14:00:30Z

CI MESSAGE: [1066265]: BUILD STARTED

dali-automaton · 2020-01-10T14:06:16Z

CI MESSAGE: [1066198]: BUILD PASSED

mzient · 2020-01-10T14:48:49Z

dali/test/python/test_operator_reshape.py

+
+        rng = ops.Uniform(range=[100,128])
+        cast = ops.Cast(dtype=types.INT32)
+        widths = cast(rng()) * 2.0


This makes sure we always get even width - we're halving it, so it must not be odd or the volume will not match.

dali/operators/util/reshape.cc

dali-automaton · 2020-01-10T15:35:12Z

CI MESSAGE: [1066265]: BUILD PASSED

Improved Reshape

f98cfae

* Add relative extents * Add "remaining volume" extent (-1) support Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

mzient requested a review from a team January 9, 2020 14:53

jantonguirao reviewed Jan 9, 2020

View reviewed changes

Bugfix. Added explicit layout clearing.

ce059b7

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

jantonguirao approved these changes Jan 9, 2020

View reviewed changes

klecki reviewed Jan 10, 2020

View reviewed changes

klecki requested changes Jan 10, 2020

View reviewed changes

mzient commented Jan 10, 2020

View reviewed changes

mzient requested a review from klecki January 10, 2020 12:57

klecki reviewed Jan 10, 2020

View reviewed changes

Bug fix. Add more tests.

30778fd

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

mzient force-pushed the ImprovedReshape branch from 65a9861 to 30778fd Compare January 10, 2020 13:57

mzient requested a review from klecki January 10, 2020 13:57

klecki approved these changes Jan 10, 2020

View reviewed changes

mzient commented Jan 10, 2020

View reviewed changes

JanuszL reviewed Jan 10, 2020

View reviewed changes

dali/operators/util/reshape.cc Show resolved Hide resolved

JanuszL approved these changes Jan 10, 2020

View reviewed changes

mzient merged commit 6b7683c into NVIDIA:master Jan 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved Reshape #1634

Improved Reshape #1634

mzient commented Jan 9, 2020

mzient commented Jan 9, 2020

dali-automaton commented Jan 9, 2020

jantonguirao Jan 9, 2020

jantonguirao Jan 9, 2020

mzient Jan 9, 2020

dali-automaton commented Jan 9, 2020

mzient commented Jan 9, 2020

dali-automaton commented Jan 9, 2020

dali-automaton commented Jan 9, 2020

klecki Jan 10, 2020

mzient Jan 10, 2020

klecki Jan 10, 2020

klecki Jan 10, 2020

klecki Jan 10, 2020

mzient Jan 10, 2020

mzient Jan 10, 2020

mzient commented Jan 10, 2020

dali-automaton commented Jan 10, 2020

klecki Jan 10, 2020

mzient commented Jan 10, 2020

dali-automaton commented Jan 10, 2020

dali-automaton commented Jan 10, 2020

mzient Jan 10, 2020 •

edited

Loading

dali-automaton commented Jan 10, 2020

Improved Reshape #1634

Improved Reshape #1634

Conversation

mzient commented Jan 9, 2020

Why we need this PR?

What happened in this PR?

mzient commented Jan 9, 2020

dali-automaton commented Jan 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dali-automaton commented Jan 9, 2020

mzient commented Jan 9, 2020

dali-automaton commented Jan 9, 2020

dali-automaton commented Jan 9, 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

mzient commented Jan 10, 2020

dali-automaton commented Jan 10, 2020

Choose a reason for hiding this comment

mzient commented Jan 10, 2020

dali-automaton commented Jan 10, 2020

dali-automaton commented Jan 10, 2020

mzient Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

dali-automaton commented Jan 10, 2020

mzient Jan 10, 2020 •

edited

Loading