Deprecate squeeze_labels option from MXNet iterator and enhance .squeeze function to match numpy style interface #2450

jantonguirao · 2020-11-10T15:57:41Z

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

Why we need this PR?

Pick one, remove the rest

It fixes a bug in MxNet generic iterator, when batch_size is 1

What happened in this PR?

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

What solution was applied:
The option squeeze_labels=True caused the batch dimension to disappear when batch_size=1
Affected modules and functionalities:
MXNet generic iterator
Key points relevant for the review:
All
Validation and testing:
Existing tests
Documentation (including examples):
N/A

JIRA TASK: [Use DALI-XXXX or NA]

JanuszL · 2020-11-16T10:35:43Z

dali/python/nvidia/dali/plugin/mxnet.py

                 Whether the iterator should squeeze the labels before
                 copying them to the ndarray.
+                 This argument is deprecated and will be removed from future releases
+                 without further notice.


Suggested change

without further notice.

JanuszL · 2020-11-16T10:41:14Z

dali/test/python/test_backend_impl.py

+                       (-2, (3, 1, 6)),
+                       (None, (1, 1, 6)),
+                       (1, (1, 1, 6)),
+                       #(None, (1, 1, 1)),  # Numpy produces a scalar in this case (probably we should too)


Should we address this now?

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

mzient · 2020-11-16T12:59:58Z

dali/pipeline/data/tensor.h

   */
  inline void Squeeze() {
-    std::vector<Index> shape(shape_.begin(), shape_.end());
+    SmallVector<int64_t, 6> shape(shape_.begin(), shape_.end());


Suggested change

SmallVector<int64_t, 6> shape(shape_.begin(), shape_.end());

auto &shape = shape_.shape;

mzient · 2020-11-16T13:00:14Z

dali/pipeline/data/tensor.h

    shape.erase(std::remove(shape.begin(), shape.end(), 1), shape.end());
-    if (shape.empty()) {
-      shape.push_back(1);
+    shape_ = shape;


This line is not necessary when you follow the suggestion above.

mzient · 2020-11-16T13:01:36Z

dali/pipeline/data/tensor.h

+    DALI_ENFORCE(dim >= -ndim && dim <= (ndim - 1),
+                 make_string("axis ", dim, " is out of bound for a tensor with ", shape_.size(),
+                             " dimensions."));
+    if (dim < 0)


I think we should remove corresponding dimension from the layout, if present.

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

mzient · 2020-11-16T19:16:59Z

dali/pipeline/data/tensor.h

-    shape.erase(std::remove(shape.begin(), shape.end(), 1), shape.end());
-    if (shape.empty()) {
-      shape.push_back(1);
+    SmallVector<int64_t, 6> out_shape;


Suggested change

SmallVector<int64_t, 6> out_shape;

DynamicTensorShapeContainer out_shape;

mzient · 2020-11-16T19:17:12Z

dali/pipeline/data/tensor.h

+      if (!in_layout.empty())
+        out_layout += in_layout[d];
+    }
+    shape_ = out_shape;


Suggested change

shape_ = out_shape;

shape_ = std::move(out_shape);

mzient · 2020-11-16T19:18:29Z

dali/pipeline/data/tensor.h

+   * equal to 1.
+   * @param dim Dimension to be squeezed. Negative indexing is also suppo9rted
+   */
+  inline void Squeeze(int dim) {


Following that definition (the function does or does not squeeze), maybe return a bool?

Suggested change

inline void Squeeze(int dim) {

inline bool Squeeze(int dim) {

mzient · 2020-11-16T19:18:46Z

dali/pipeline/data/tensor.h

+      auto layout = GetLayout();
+      if (!layout.empty()) {
+        SetLayout(layout.first(dim) + layout.sub(dim + 1));
+      }


Suggested change

}

}

return true;

mzient · 2020-11-16T19:18:59Z

dali/pipeline/data/tensor.h

    }
-    shape_ = shape;
  }


Suggested change

}

return false;

}

mzient · 2020-11-16T19:19:24Z

include/dali/core/tensor_layout.h

@@ -302,10 +312,29 @@ class TensorLayout {

  DALI_HOST_DEV
  friend constexpr TensorLayout operator+(const TensorLayout &a, const TensorLayout &b);
+  DALI_HOST_DEV
+  friend constexpr TensorLayout operator+(const TensorLayout &a, const char &b);


Don't pass primitive types by reference

Suggested change

friend constexpr TensorLayout operator+(const TensorLayout &a, const char &b);

friend constexpr TensorLayout operator+(const TensorLayout &a, char b);

Yeah, it is a leftover (I was trying to figure out a compile error, that turned out to be the lack of operator+=). I'll revert to pass by value

mzient · 2020-11-16T19:19:54Z

include/dali/core/tensor_layout.h

 };

 static_assert(sizeof(TensorLayout) == 16, "Tensor layout size should be exactly 16B");

+/** @brief Appends a single element to the layout string */
+DALI_HOST_DEV
+constexpr TensorLayout operator+(const TensorLayout &a, const char &b) {


Suggested change

constexpr TensorLayout operator+(const TensorLayout &a, const char &b) {

constexpr TensorLayout operator+(const TensorLayout &a, char b) {

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

jantonguirao · 2020-11-17T12:32:19Z

!build

dali-automaton · 2020-11-17T12:36:26Z

CI MESSAGE: [1806157]: BUILD STARTED

dali-automaton · 2020-11-17T14:03:12Z

CI MESSAGE: [1806157]: BUILD PASSED

jantonguirao changed the title ~~Remove squeeze_labels option from MxNet iterator~~ Remove squeeze_labels option from MXNet iterator Nov 10, 2020

jantonguirao force-pushed the mxnet_iter_squeeze branch from b99befe to 456775f Compare November 16, 2020 10:31

jantonguirao changed the title ~~Remove squeeze_labels option from MXNet iterator~~ Deprecate squeeze_labels option from MXNet iterator and enhance .squeeze function to resemble numpy Nov 16, 2020

jantonguirao changed the title ~~Deprecate squeeze_labels option from MXNet iterator and enhance .squeeze function to resemble numpy~~ Deprecate squeeze_labels option from MXNet iterator and enhance .squeeze function to match numpy style interface Nov 16, 2020

jantonguirao force-pushed the mxnet_iter_squeeze branch from 456775f to ab2b497 Compare November 16, 2020 10:34

JanuszL reviewed Nov 16, 2020

View reviewed changes

jantonguirao force-pushed the mxnet_iter_squeeze branch from ab2b497 to e96da5e Compare November 16, 2020 10:49

Deprecate squeeze_labels option and enhance tensor.squeeze function

11020ca

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

jantonguirao force-pushed the mxnet_iter_squeeze branch from e96da5e to 11020ca Compare November 16, 2020 11:00

mzient reviewed Nov 16, 2020

View reviewed changes

JanuszL approved these changes Nov 16, 2020

View reviewed changes

Code review fixes

3245d58

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

jantonguirao force-pushed the mxnet_iter_squeeze branch from 6a7b240 to 3245d58 Compare November 16, 2020 17:35

mzient reviewed Nov 16, 2020

View reviewed changes

dali/pipeline/data/tensor.h

}

shape_ = shape;

}

Copy link

Contributor

mzient Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

}

return false;

}

mzient reviewed Nov 16, 2020

View reviewed changes

jantonguirao force-pushed the mxnet_iter_squeeze branch from b0232e5 to 3a8e056 Compare November 17, 2020 09:41

mzient approved these changes Nov 17, 2020

View reviewed changes

Code review fixes

92f5940

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

jantonguirao force-pushed the mxnet_iter_squeeze branch from c1753e6 to 92f5940 Compare November 17, 2020 12:31

jantonguirao merged commit fc78b95 into NVIDIA:master Nov 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate squeeze_labels option from MXNet iterator and enhance .squeeze function to match numpy style interface #2450

Deprecate squeeze_labels option from MXNet iterator and enhance .squeeze function to match numpy style interface #2450

jantonguirao commented Nov 10, 2020

JanuszL Nov 16, 2020

JanuszL Nov 16, 2020

jantonguirao Nov 16, 2020

mzient Nov 16, 2020

mzient Nov 16, 2020

mzient Nov 16, 2020

mzient Nov 16, 2020

mzient Nov 16, 2020

mzient Nov 16, 2020

mzient Nov 16, 2020

mzient Nov 16, 2020

mzient Nov 16, 2020

mzient Nov 16, 2020 •

edited

Loading

jantonguirao Nov 17, 2020

mzient Nov 16, 2020

jantonguirao commented Nov 17, 2020

dali-automaton commented Nov 17, 2020

dali-automaton commented Nov 17, 2020

	SmallVector<int64_t, 6> shape(shape_.begin(), shape_.end());
	auto &shape = shape_.shape;

	SmallVector<int64_t, 6> out_shape;
	DynamicTensorShapeContainer out_shape;

	inline void Squeeze(int dim) {
	inline bool Squeeze(int dim) {

	friend constexpr TensorLayout operator+(const TensorLayout &a, const char &b);
	friend constexpr TensorLayout operator+(const TensorLayout &a, char b);

	constexpr TensorLayout operator+(const TensorLayout &a, const char &b) {
	constexpr TensorLayout operator+(const TensorLayout &a, char b) {

Deprecate squeeze_labels option from MXNet iterator and enhance .squeeze function to match numpy style interface #2450

Deprecate squeeze_labels option from MXNet iterator and enhance .squeeze function to match numpy style interface #2450

Conversation

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

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 Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jantonguirao commented Nov 17, 2020

dali-automaton commented Nov 17, 2020

dali-automaton commented Nov 17, 2020

mzient Nov 16, 2020 •

edited

Loading