Add squeeze operator #2792

majra20 · 2021-03-15T16:03:13Z

Why we need this PR?

It adds new feature needed because it add new functionality

What happened in this PR?

What solution was applied:
- Added squeeze operator
Affected modules and functionalities:
- Squeeze operator which allows to squeeze dimensions of size one
Key points relevant for the review:
- Squeeze operator code, does it do everything we wanted
Validation and testing:
- Python tests: test_operator_squeeze.py
Documentation (including examples):
NA

JIRA TASK: DALI-1851

dali/operators/generic/reshape.cc

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

dali/operators/generic/squeeze.cc

dali/test/python/test_operator_squeeze.py

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

dali/test/python/test_operator_squeeze.py

dali/operators/generic/squeeze.cc

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

mzient · 2021-03-16T12:27:04Z

dali/operators/generic/squeeze.cc

+namespace dali {
+
+DALI_SCHEMA(Squeeze)
+  .DocStr(R"code(Collapses the dimensions given as axes or axis_names.


Suggested change

.DocStr(R"code(Collapses the dimensions given as axes or axis_names.

.DocStr(R"code(Removes the dimensions given as ``axes`` or ``axis_names``.

"Collapse" could be misunderstood as "flatten" or "combine" - which, I believe, would make for a very useful operator, but it's not this one.

mzient · 2021-03-16T12:29:05Z

dali/operators/generic/squeeze.cc

+DALI_SCHEMA(Squeeze)
+  .DocStr(R"code(Collapses the dimensions given as axes or axis_names.
+
+It's an error to collapse a dimension that would cause the total volume to change.)code")


Suggested change

It's an error to collapse a dimension that would cause the total volume to change.)code")

It's an error to remove a dimension that would cause the total volume to change.)code")

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

jantonguirao · 2021-03-16T13:15:37Z

dali/operators/generic/squeeze.cc

+All indices must be in the range of valid dimensions of the input)code", std::vector<int>(), true)
+  .AddOptionalArg("axis_names", R"code(Layout columns which should be removed.
+
+All squeezed dimensions should have size 1 unless the total volume of the tensor is 0 before and after squeeze.


Suggested change

All squeezed dimensions should have size 1 unless the total volume of the tensor is 0 before and after squeeze.

All squeezed dimensions should have size 1, unless the total volume of the tensor is 0 before and after squeeze.

nitpick

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

dali/operators/generic/squeeze.cc

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

mzient · 2021-03-16T13:49:18Z

dali/operators/generic/squeeze.cc

+  DALI_ENFORCE(spec.HasArgument("axes") + spec.HasArgument("axis_names") == 1,
+    make_string("Provided both axes and axis_names argument"));


Suggested change

DALI_ENFORCE(spec.HasArgument("axes") + spec.HasArgument("axis_names") == 1,

make_string("Provided both axes and axis_names argument"));

DALI_ENFORCE(spec.HasArgument("axes") + spec.HasArgument("axis_names") == 1,

spec.HasArgument("axes") ? "Provided both ``axes`` and ``axis_names`` arguments"

: "Missing argument ``axes`` or ``axis_names``." );

mzient · 2021-03-16T13:52:33Z

dali/operators/generic/squeeze.cc

+  this->SetOutputType(ws);
+
+  GenerateSrcDims(ws);
+  Reshape<Backend>::CalculateOutputShape(ws);


This would invoke one from the base class in case you overrode it - and you wouldn't even know. Use this-> to enable dependent name lookup.

Suggested change

Reshape<Backend>::CalculateOutputShape(ws);

this->CalculateOutputShape(ws);

mzient · 2021-03-16T13:55:03Z

dali/operators/generic/squeeze.cc

+  DALI_ENFORCE(in_layout.size() == ndim || in_layout.empty(),
+      make_string("Layout for data has ",
+      in_layout.size(), " elements but data has ", ndim, " dimensions."));


Suggested change

DALI_ENFORCE(in_layout.size() == ndim || in_layout.empty(),

make_string("Layout for data has ",

in_layout.size(), " elements but data has ", ndim, " dimensions."));

This is checked in the executor.

mzient · 2021-03-16T13:57:02Z

dali/operators/generic/squeeze.h

+ private:
+  void GenerateSrcDims(const Workspace &ws);
+
+  std::vector<int> axes_;


Suggested change

std::vector<int> axes_;

SmallVector<int, 6> axes_;

This will avoid calling to_vector and a needless dynamic allocation.

mzient · 2021-03-16T13:58:19Z

dali/operators/generic/squeeze.cc

+      in_layout.size(), " elements but data has ", ndim, " dimensions."));
+
+  this->src_dims_.clear();
+  auto axes = axis_names_.empty() ? axes_ : GetDimIndices(in_layout, axis_names_).to_vector();


If you convert axes_ to SmallVector, you can skip to_vector and avoid allocating a handful of numbers from the heap.

Suggested change

auto axes = axis_names_.empty() ? axes_ : GetDimIndices(in_layout, axis_names_).to_vector();

auto axes = axis_names_.empty() ? axes_ : GetDimIndices(in_layout, axis_names_);

mzient · 2021-03-16T14:00:54Z

dali/operators/generic/squeeze.cc

+  this->src_dims_.clear();
+  auto axes = axis_names_.empty() ? axes_ : GetDimIndices(in_layout, axis_names_).to_vector();
+  std::sort(axes.begin(), axes.end());
+  axes.erase(std::unique(axes.begin(), axes.end()), axes.end());


I think that specifying an axis multiple times should be an error, not something swept under the carpet.

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

mzient · 2021-03-16T14:28:31Z

dali/operators/generic/squeeze.cc

+    if (!in_layout.empty()) {
+      out_layout += in_layout[d];
+    }
+  }
+  this->layout_ = out_layout;


Rather a nitpick:

Suggested change

if (!in_layout.empty()) {

out_layout += in_layout[d];

}

}

this->layout_ = out_layout;

}

if (!in_layout.empty())

this->layout_ = permute(in_layout, this->src_dims_);

I think it does something different then what I meant. I will skip it.

majra20 · 2021-03-16T14:43:53Z

!build

dali-automaton · 2021-03-16T14:46:19Z

CI MESSAGE: [2171679]: BUILD STARTED

dali-automaton · 2021-03-16T14:50:23Z

CI MESSAGE: [2171679]: BUILD FAILED

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

majra20 · 2021-03-16T14:58:28Z

!build

dali-automaton · 2021-03-16T15:01:08Z

CI MESSAGE: [2171766]: BUILD STARTED

dali-automaton · 2021-03-16T16:47:42Z

CI MESSAGE: [2171766]: BUILD PASSED

majra20 assigned jantonguirao and mzient Mar 15, 2021

mzient reviewed Mar 15, 2021

View reviewed changes

dali/operators/generic/reshape.cc Outdated Show resolved Hide resolved

Rafal Maj added 14 commits March 16, 2021 09:02

src_dims arg added to reshape

8ce3c72

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

Code review adjustments

9b27f6d

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

Add use_src_dims_ flag

3baa8bd

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

src_dims arg added to reshape

6bea33a

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

Code review adjustments

b0eeaec

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

squeeze operator

1c830f4

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

Rebase

04e9394

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

Schema desc, 0d tensor handling

51691e1

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

rebase

4023438

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

rebase

00a3158

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

Change of copyright year

1b5eb2d

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

Bypass init in reshape constructor

c18eac3

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

Fix build

aef7e64

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

Fix lint

2f1404d

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

jantonguirao reviewed Mar 16, 2021

View reviewed changes

Adjustments after code review

5f66bb0

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

majra20 force-pushed the squeeze_operator branch from 7cb1dc6 to 5f66bb0 Compare March 16, 2021 08:52

Rafal Maj added 2 commits March 16, 2021 10:00

Add documentation about arguments

310518c

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

Add degenerate test cases

2f3ecc4

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

jantonguirao reviewed Mar 16, 2021

View reviewed changes

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

jantonguirao reviewed Mar 16, 2021

View reviewed changes

dali/operators/generic/squeeze.cc Outdated Show resolved Hide resolved

jantonguirao reviewed Mar 16, 2021

View reviewed changes

dali/operators/generic/squeeze.cc Outdated Show resolved Hide resolved

Code review comments

e0838c5

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

mzient reviewed Mar 16, 2021

View reviewed changes

Rafal Maj added 2 commits March 16, 2021 13:40

Docs update

74a87a7

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

Docs update

32fb656

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

jantonguirao reviewed Mar 16, 2021

View reviewed changes

jantonguirao approved these changes Mar 16, 2021

View reviewed changes

Docs update

519afb7

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

mzient reviewed Mar 16, 2021

View reviewed changes

dali/operators/generic/squeeze.cc Outdated Show resolved Hide resolved

Code review adjustments

308c2b2

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

mzient reviewed Mar 16, 2021

View reviewed changes

Code review adjustments

c913fc2

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

mzient reviewed Mar 16, 2021

View reviewed changes

mzient approved these changes Mar 16, 2021

View reviewed changes

Fix lint

a44e2da

Signed-off-by: Rafal Maj <rmaj@nvidia.com>

majra20 merged commit 58623e0 into NVIDIA:master Mar 16, 2021

majra20 deleted the squeeze_operator branch March 17, 2021 08:14

JanuszL mentioned this pull request May 19, 2021

DALI 2021 roadmap #2978

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add squeeze operator #2792

Add squeeze operator #2792

majra20 commented Mar 15, 2021

mzient Mar 16, 2021 •

edited

Loading

mzient Mar 16, 2021

jantonguirao Mar 16, 2021

mzient Mar 16, 2021 •

edited

Loading

mzient Mar 16, 2021

mzient Mar 16, 2021

mzient Mar 16, 2021

mzient Mar 16, 2021

mzient Mar 16, 2021

mzient Mar 16, 2021 •

edited

Loading

majra20 Mar 16, 2021

majra20 commented Mar 16, 2021

dali-automaton commented Mar 16, 2021

dali-automaton commented Mar 16, 2021

majra20 commented Mar 16, 2021

dali-automaton commented Mar 16, 2021

dali-automaton commented Mar 16, 2021

	.DocStr(R"code(Collapses the dimensions given as axes or axis_names.
	.DocStr(R"code(Removes the dimensions given as ``axes`` or ``axis_names``.

	It's an error to collapse a dimension that would cause the total volume to change.)code")
	It's an error to remove a dimension that would cause the total volume to change.)code")

	All squeezed dimensions should have size 1 unless the total volume of the tensor is 0 before and after squeeze.
	All squeezed dimensions should have size 1, unless the total volume of the tensor is 0 before and after squeeze.

		DALI_ENFORCE(spec.HasArgument("axes") + spec.HasArgument("axis_names") == 1,
		make_string("Provided both axes and axis_names argument"));

	Reshape<Backend>::CalculateOutputShape(ws);
	this->CalculateOutputShape(ws);

	DALI_ENFORCE(in_layout.size() == ndim \|\| in_layout.empty(),
	make_string("Layout for data has ",
	in_layout.size(), " elements but data has ", ndim, " dimensions."));

	auto axes = axis_names_.empty() ? axes_ : GetDimIndices(in_layout, axis_names_).to_vector();
	auto axes = axis_names_.empty() ? axes_ : GetDimIndices(in_layout, axis_names_);

Add squeeze operator #2792

Add squeeze operator #2792

Conversation

majra20 commented Mar 15, 2021

Why we need this PR?

What happened in this PR?

mzient Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient Mar 16, 2021 • 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

mzient Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majra20 commented Mar 16, 2021

dali-automaton commented Mar 16, 2021

dali-automaton commented Mar 16, 2021

majra20 commented Mar 16, 2021

dali-automaton commented Mar 16, 2021

dali-automaton commented Mar 16, 2021

mzient Mar 16, 2021 •

edited

Loading

mzient Mar 16, 2021 •

edited

Loading

mzient Mar 16, 2021 •

edited

Loading