Add Translate affine transform generator #2297

jantonguirao · 2020-09-24T15:23:12Z

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

Why we need this PR?

It adds a new feature need to be able to produce affine transform matrices that can be later used in operators such as WarpAffine, CoordsTransform, etc

What happened in this PR?

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

What solution was applied:
Introduced a TransformBaseOp that defines most of the logic necessary to implement the different transformations. TransformBaseOp uses the curiously recurring template pattern (CRTP)
Added the implementation of TranslateTransform, including tests
Affected modules and functionalities:
New operator
Key points relevant for the review:
TransformBaseOp and TranslateTransform
Validation and testing:
Python tests added
Documentation (including examples):
Doc str

JIRA TASK: [DALI-1626]

mzient · 2020-09-25T12:21:16Z

dali/operators/geometry/transform_base_op.h

+
+  template <typename T>
+  void RunImplTyped(workspace_t<Backend> &ws, dims<>) {
+    DALI_FAIL(make_string("Unsupported number of dimensions ", ndim_));  // NOLINT


Suggested change

DALI_FAIL(make_string("Unsupported number of dimensions ", ndim_)); // NOLINT

DALI_FAIL(make_string("Unsupported number of dimensions ", ndim_));

leftover from VALUE_SWITCH, I guess...

mzient · 2020-09-25T12:23:49Z

dali/operators/geometry/transform_base_op.h

+      for (int i = 0; i < nsamples_; i++) {
+        DALI_ENFORCE(shape[i] == shape[0],
+          make_string("Samples are expected to have the same dimensionality. Got: ", shape));
+      }


Suggested change

for (int i = 0; i < nsamples_; i++) {

DALI_ENFORCE(shape[i] == shape[0],

make_string("Samples are expected to have the same dimensionality. Got: ", shape));

}

DALI_ENFORCE(is_uniform(shape), "All matrices must have the same shape.");

...and I'd move this check higher.

dali/operators/geometry/transform_base_op.h

mzient · 2020-09-25T12:35:43Z

dali/operators/geometry/transform_ops.cc

+    } else {
+      assert(ndim == offset_.size());
+      auto &matrix = matrices[0];
+      matrix = affine_mat_t<T, ndim>::identity();


Suggested change

matrix = affine_mat_t<T, ndim>::identity();

matrix = 1;

much shorter :)

mzient · 2020-09-25T12:36:52Z

dali/operators/geometry/transform_ops.cc

+      }
+    } else {
+      assert(ndim == offset_.size());
+      auto &matrix = matrices[0];


Either do it once or query for offset argument each time. Now you keep rewriting the matrices with a value that can't change at run-time.

dali/operators/geometry/transform_ops.cc

mzient · 2020-09-25T12:42:21Z

dali/operators/geometry/transform_ops.cc

+
+DALI_SCHEMA(TranslateTransform)
+  .DocStr(R"(TranslateTransform)")
+  .AddArg("offset", "Translation vector", DALI_FLOAT_VEC, true)


I wonder if we should rename it to "T" to aligned with CoordTransform

Suggested change

.AddArg("offset", "Translation vector", DALI_FLOAT_VEC, true)

.AddArg("T", "Translation vector", DALI_FLOAT_VEC, true)

mzient · 2020-09-28T11:39:24Z

dali/operators/geometry/transform_base_op.h

+    }
+
+    auto &out = ws.template OutputRef<Backend>(0);
+    out.SetLayout("");  // TODO(janton): Decide what layout we want for transforms


I think we can live with no layout for plain matrices - unless we want to somehow stress that they're row-major - then maybe "rc" (row col).

mzient · 2020-09-28T11:45:27Z

dali/operators/geometry/transform_base_op.h

+  const TransformImpl &This() const noexcept { return static_cast<const TransformImpl&>(*this); }
+
+ protected:
+  void CheckInputDimensionality(const workspace_t<CPUBackend> &ws) {


I'd avoid the name "dimensionality" - especially as a standalone word. Here I'd use

Suggested change

void CheckInputDimensionality(const workspace_t<CPUBackend> &ws) {

void CheckInputShape(const workspace_t<CPUBackend> &ws) {

mzient · 2020-09-28T11:56:23Z

dali/operators/geometry/transform_base_op.h

+          "Unexpected number of dimensions: expected ", ndim_, 
+          " dimensions but the input transform has ", in_t_ndim));


Suggested change

"Unexpected number of dimensions: expected ", ndim_,

" dimensions but the input transform has ", in_t_ndim));

"The input describes a ", in_t_dim, "D transform but other arguments suggest a ",

ndim_, "D transform"));

mzient · 2020-09-28T12:05:57Z

dali/operators/geometry/transform_base_op.h

+  int nsamples_ = -1;
+  bool has_input_ = false;
+
+  std::vector<uint8_t> matrices_data_;


Grammar Nazi to the rescue:

Suggested change

std::vector<uint8_t> matrices_data_;

std::vector<uint8_t> matrix_data_;

or, if you insist on plural:

Suggested change

std::vector<uint8_t> matrices_data_;

std::vector<uint8_t> raw_matrices_;

BTW - if you're type-punning these, why not a Tensor?

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

JanuszL · 2020-09-29T08:57:07Z

dali/operators/geometry/affine_transforms/translate_transform.cc

 #include "dali/pipeline/data/views.h"

 namespace dali {

 DALI_SCHEMA(TranslateTransform)
-  .DocStr(R"(TranslateTransform)")
-  .AddArg("offset", "Translation vector", DALI_FLOAT_VEC, true)
+  .DocStr(R"(Produces a translation affine transform.


Suggested change

.DocStr(R"(Produces a translation affine transform.

.DocStr(R"(Produces a translation affine transform matrix.

JanuszL · 2020-09-29T08:57:26Z

dali/operators/geometry/affine_transforms/translate_transform.cc

-  .AddArg("offset", "Translation vector", DALI_FLOAT_VEC, true)
+  .DocStr(R"(Produces a translation affine transform.
+
+If another transform is passed as an input, the operator will combine the two transforms by applying


Suggested change

If another transform is passed as an input, the operator will combine the two transforms by applying

If another transform matrix is passed as an input, the operator will combine the two matrices by applying

JanuszL · 2020-09-29T08:58:03Z

dali/operators/geometry/affine_transforms/translate_transform.cc

+translation to the transform provided.
+
+.. note::
+    The output of this operator can be fed directly to ``MT`` argument of ``CoordTransform`` operator.


Can we use it in Warp operator for images as well?

As far as I know, WarpAffine operator expects the inverse of the transform, so it can't be directly fed into it until WarpAffine implements the flag to accept inverse transforms. @mzient Correct?

For translation, we'd just need to use the negated translation vector. Or, if you don't have point cloud to go with your image, you can construct the dst-to-src transform directly.

JanuszL · 2020-09-29T08:58:41Z

dali/operators/geometry/affine_transforms/translate_transform.cc

+)")
+  .AddArg(
+    "offset",
+    "Translation vector. The number of dimensions of the transform is inferred from this argument",


Suggested change

"Translation vector. The number of dimensions of the transform is inferred from this argument",

R"code(Translation vector.

The number of dimensions of the transform is inferred from this argument)code",

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

dali/test/python/test_operator_affine_transforms.py

dali/operators/geometry/affine_transforms/transform_base_op.h

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

dali/operators/geometry/affine_transforms/transform_base_op.cc

dali/test/python/test_operator_affine_transforms.py

dali/operators/geometry/affine_transforms/transform_base_op.h

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

jantonguirao · 2020-09-29T13:24:21Z

!build

dali-automaton · 2020-09-29T13:30:51Z

CI MESSAGE: [1662998]: BUILD STARTED

dali-automaton · 2020-09-29T14:13:18Z

CI MESSAGE: [1662998]: BUILD FAILED

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

dali-automaton · 2020-09-29T14:29:30Z

CI MESSAGE: [1663069]: BUILD STARTED

dali-automaton · 2020-09-29T14:55:58Z

CI MESSAGE: [1663069]: BUILD FAILED

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

dali-automaton · 2020-09-29T15:04:18Z

CI MESSAGE: [1663137]: BUILD STARTED

dali-automaton · 2020-09-29T18:00:15Z

CI MESSAGE: [1663137]: BUILD FAILED

dali-automaton · 2020-09-29T19:08:23Z

CI MESSAGE: [1663137]: BUILD PASSED

jantonguirao force-pushed the affine_transform_generators branch from 979a812 to dbf58f8 Compare September 25, 2020 12:14

mzient reviewed Sep 25, 2020

View reviewed changes

dali/operators/geometry/transform_base_op.h Outdated Show resolved Hide resolved

mzient reviewed Sep 25, 2020

View reviewed changes

dali/operators/geometry/transform_base_op.h Outdated Show resolved Hide resolved

mzient reviewed Sep 25, 2020

View reviewed changes

dali/operators/geometry/transform_ops.cc Outdated Show resolved Hide resolved

mzient reviewed Sep 25, 2020

View reviewed changes

dali/operators/geometry/transform_ops.cc Outdated Show resolved Hide resolved

mzient reviewed Sep 25, 2020

View reviewed changes

dali/operators/geometry/transform_ops.cc Outdated Show resolved Hide resolved

mzient reviewed Sep 25, 2020

View reviewed changes

mzient reviewed Sep 28, 2020

View reviewed changes

jantonguirao added 3 commits September 28, 2020 14:55

WIP Affine transform generators

986c4db

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

Add Translate Transform

41dafb5

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

Code review fixes

d0ecae9

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

jantonguirao force-pushed the affine_transform_generators branch from 8a27256 to d0ecae9 Compare September 28, 2020 13:03

jantonguirao added 2 commits September 28, 2020 17:57

Some code review fixes

d74b72b

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

Separate matrix definition from argument processing

635815b

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

jantonguirao force-pushed the affine_transform_generators branch from a596f6f to 635815b Compare September 29, 2020 07:53

jantonguirao changed the title ~~[WIP] Affine transform generators~~ Add Translate Affine transform generator Sep 29, 2020

jantonguirao changed the title ~~Add Translate Affine transform generator~~ Add Translate affine transform generator Sep 29, 2020

JanuszL reviewed Sep 29, 2020

View reviewed changes

jantonguirao force-pushed the affine_transform_generators branch 2 times, most recently from a86b681 to 0e8b347 Compare September 29, 2020 09:09

Add tests

6555902

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

jantonguirao force-pushed the affine_transform_generators branch from 0e8b347 to 6555902 Compare September 29, 2020 09:10

JanuszL reviewed Sep 29, 2020

View reviewed changes

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

mzient reviewed Sep 29, 2020

View reviewed changes

dali/operators/geometry/affine_transforms/transform_base_op.h Outdated Show resolved Hide resolved

jantonguirao force-pushed the affine_transform_generators branch 3 times, most recently from adf06b9 to ecc3501 Compare September 29, 2020 11:16

Apply suggestions

c15cebe

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

jantonguirao force-pushed the affine_transform_generators branch from ecc3501 to c15cebe Compare September 29, 2020 11:17

JanuszL reviewed Sep 29, 2020

View reviewed changes

dali/operators/geometry/affine_transforms/transform_base_op.cc Outdated Show resolved Hide resolved

mzient reviewed Sep 29, 2020

View reviewed changes

dali/test/python/test_operator_affine_transforms.py Outdated Show resolved Hide resolved

mzient reviewed Sep 29, 2020

View reviewed changes

dali/operators/geometry/affine_transforms/transform_base_op.h Outdated Show resolved Hide resolved

jantonguirao force-pushed the affine_transform_generators branch from 0566de4 to d7a6c63 Compare September 29, 2020 12:06

Calculate ndim as mat_dim - 1

1a505c2

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

jantonguirao force-pushed the affine_transform_generators branch from d7a6c63 to 1a505c2 Compare September 29, 2020 12:08

mzient approved these changes Sep 29, 2020

View reviewed changes

JanuszL approved these changes Sep 29, 2020

View reviewed changes

Fix clang warnings

ce75600

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

Avoid gcc/clang issues...

7a89678

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

jantonguirao merged commit 7813418 into NVIDIA:master Sep 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Translate affine transform generator #2297

Add Translate affine transform generator #2297

jantonguirao commented Sep 24, 2020 •

edited

Loading

mzient Sep 25, 2020

mzient Sep 25, 2020

mzient Sep 25, 2020 •

edited

Loading

mzient Sep 25, 2020 •

edited

Loading

mzient Sep 25, 2020

mzient Sep 28, 2020

mzient Sep 28, 2020

mzient Sep 28, 2020

mzient Sep 28, 2020

JanuszL Sep 29, 2020

JanuszL Sep 29, 2020

JanuszL Sep 29, 2020

jantonguirao Sep 29, 2020

mzient Sep 29, 2020 •

edited

Loading

JanuszL Sep 29, 2020

jantonguirao commented Sep 29, 2020

dali-automaton commented Sep 29, 2020

dali-automaton commented Sep 29, 2020

dali-automaton commented Sep 29, 2020

dali-automaton commented Sep 29, 2020

dali-automaton commented Sep 29, 2020

dali-automaton commented Sep 29, 2020

dali-automaton commented Sep 29, 2020

	DALI_FAIL(make_string("Unsupported number of dimensions ", ndim_)); // NOLINT
	DALI_FAIL(make_string("Unsupported number of dimensions ", ndim_));

	.AddArg("offset", "Translation vector", DALI_FLOAT_VEC, true)
	.AddArg("T", "Translation vector", DALI_FLOAT_VEC, true)

	void CheckInputDimensionality(const workspace_t<CPUBackend> &ws) {
	void CheckInputShape(const workspace_t<CPUBackend> &ws) {

		"Unexpected number of dimensions: expected ", ndim_,
		" dimensions but the input transform has ", in_t_ndim));

	std::vector<uint8_t> matrices_data_;
	std::vector<uint8_t> matrix_data_;

	std::vector<uint8_t> matrices_data_;
	std::vector<uint8_t> raw_matrices_;

	.DocStr(R"(Produces a translation affine transform.
	.DocStr(R"(Produces a translation affine transform matrix.

	If another transform is passed as an input, the operator will combine the two transforms by applying
	If another transform matrix is passed as an input, the operator will combine the two matrices by applying

-    "Translation vector. The number of dimensions of the transform is inferred from this argument",
+    R"code(Translation vector.
+The number of dimensions of the transform is inferred from this argument)code",

Add Translate affine transform generator #2297

Add Translate affine transform generator #2297

Conversation

jantonguirao commented Sep 24, 2020 • edited Loading

Why we need this PR?

What happened in this PR?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

mzient Sep 25, 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

mzient Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jantonguirao commented Sep 29, 2020

dali-automaton commented Sep 29, 2020

dali-automaton commented Sep 29, 2020

dali-automaton commented Sep 29, 2020

dali-automaton commented Sep 29, 2020

dali-automaton commented Sep 29, 2020

dali-automaton commented Sep 29, 2020

dali-automaton commented Sep 29, 2020

jantonguirao commented Sep 24, 2020 •

edited

Loading

mzient Sep 25, 2020 •

edited

Loading

mzient Sep 25, 2020 •

edited

Loading

mzient Sep 29, 2020 •

edited

Loading