Transform points op #2288

mzient · 2020-09-21T09:18:01Z

Why we need this PR?

It adds new feature: operator that transforms coordinates
Allow empty shape (scalars) in Reshape op.

What happened in this PR?

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

What solution was applied:
- Added the operator
- Modified Reshape to accept empty list as a shape.
Affected modules and functionalities:
- The new operator(s)
- Reshape
Key points relevant for the review:
- N/A
Validation and testing:
- Python test
Documentation (including examples):
- Docstrings

JIRA TASK: DALI-1482, DALI-1605

JanuszL · 2020-09-21T15:10:35Z

dali/operators/geometry/coord_transform.cc

+DALI_SCHEMA(CoordTransform)
+  .DocStr(R"(Applies a linear transformation to points or vectors.
+
+


I think this bank is redundant.

JanuszL · 2020-09-21T15:13:31Z

dali/operators/geometry/coord_transform.h

+          )   // NOLINT
+        )  // NOLINT
+      ), (  // NOLINT
+        DALI_FAIL(make_string("Unsupported input point dimensionality: ", input_pt_dim_));


Suggested change

DALI_FAIL(make_string("Unsupported input point dimensionality: ", input_pt_dim_));

DALI_FAIL(make_string("Unsupported output point dimensionality: ", input_pt_dim_));

JanuszL · 2020-09-21T15:24:33Z

dali/operators/geometry/mt_transform_attr.cc

+        FillDiag(make_span(&per_sample_mtx_[i * mat_size], mat_size), M.data[i][0]);
+      }
+      translation_.resize(output_pt_dim_, 0);
+      Repeat(per_sample_translation_, translation_, N);


Can add a comment why the if (is_fused) is not relevant here?

It's doesn't change anything, but it should be here to avoid a redundant operation.

I know, but for consistency I would leave at least a comment that it is skipped on purpose here.

What I meant is: the if should be here. I added it.

mzient · 2020-09-21T17:54:15Z

!build

dali-automaton · 2020-09-21T18:01:08Z

CI MESSAGE: [1642398]: BUILD STARTED

dali-automaton · 2020-09-21T20:40:02Z

CI MESSAGE: [1642398]: BUILD PASSED

dali/operators/geometry/coord_transform.cc

jantonguirao · 2020-09-22T08:45:32Z

dali/operators/geometry/mt_transform_attr.cc

+    true)
+  .AddOptionalArg<vector<float>>("T", R"(The translation vector.
+
+If left unspecified, no translation is applied.


This is not necessarily true. You can still specify translation with MT

Will adjust.

jantonguirao · 2020-09-22T08:46:40Z

dali/operators/geometry/mt_transform_attr.cc

+        translation_.resize(output_pt_dim_, 0);
+      }
+    } else {
+      DALI_ENFORCE(mtx_.size() % cols == 0, make_string("Cannot form a matrix ",


Suggested change

DALI_ENFORCE(mtx_.size() % cols == 0, make_string("Cannot form a matrix ",

DALI_ENFORCE(mtx_.size() % cols == 0, make_string("Cannot form a matrix with ",

jantonguirao · 2020-09-22T08:50:32Z

dali/operators/geometry/mt_transform_attr.h

+  }
+
+
+  vector<float> mtx_;


do you need those to be public?

In the following scenarios it might be useful:

If this class is used by composition instead of inheritance.

...or the derived class has pImpl.

mzient · 2020-09-22T15:34:08Z

!build

dali-automaton · 2020-09-22T15:40:37Z

CI MESSAGE: [1645356]: BUILD STARTED

dali-automaton · 2020-09-22T18:43:53Z

CI MESSAGE: [1645356]: BUILD FAILED

mzient · 2020-09-22T21:01:06Z

!build

dali-automaton · 2020-09-22T21:05:51Z

CI MESSAGE: [1646310]: BUILD STARTED

dali-automaton · 2020-09-22T23:38:48Z

CI MESSAGE: [1646310]: BUILD FAILED

dali-automaton · 2020-09-23T08:01:36Z

CI MESSAGE: [1646310]: BUILD PASSED

klecki

Main points:

I think there is no check for degenerate argument of empty list, it will probably go through codepath for non-scalar argument provided.
repeating the obtaining and copying of regular argument in every iteration
ProcessMatrixArg is a bit long and hard to follow with the ifs, but isn't that bad.

some naming/typos.

klecki · 2020-09-23T12:07:30Z

dali/operators/geometry/coord_transform.h

+
+namespace dali {
+
+#define COORD_TRANSFORM_INPUT_TYPES (uint8_t, int16_t, uint16_t, int32_t, float)


What's the reason for this particular subset of types?
This is only a side comment, but if adding more types usually becomes expensive, maybe we need to look into that.

We need to limit the number of types, because the dimensionality is handled at compile-time and we already have 36 combinations there.
This set of types will allow us to process:

colors (uint8, uint16)

audio (int16, int32)

geometry (float)

klecki · 2020-09-23T12:26:53Z

dali/operators/geometry/mt_transform_attr.cc

+If left unspecified, identity matrix is used.
+
+The matrix ``M`` does not need to be square - if it's not, the output vectors will have a
+different number of components.


Maybe add that it will be different than the input if we need to be really verbose. It sounds a bit like each of them can have a different number of components.

klecki · 2020-09-23T12:31:42Z

dali/operators/geometry/mt_transform_attr.cc

+
+If left unspecified, no translation is applied unless MT argument is used.
+
+The number of components of this vector must match the number of rows in matrix ``M``.


If we provide the matrix M as a flattened list of values, there isn't exactly a good notion of rows. Should we specify that the matrix should be provided as flattened row-major matrix?
(Ideally we would be able to accept anything array like with additional shape, but that would be a bit over the top in this PR).

klecki · 2020-09-23T13:46:13Z

dali/operators/geometry/mt_transform_attr.cc

+        translation_.resize(output_pt_dim_, 0);
+      }
+    } else {
+      DALI_ENFORCE(mtx_.size() % cols == 0, make_string("Cannot form a matrix with ",


What if mtx_.size() == 0?

Then we end up with empty output. It will raise a meaningful error later, when selecting the kernel.

Handled explicitly.

klecki · 2020-09-23T13:49:49Z

dali/operators/geometry/mt_transform_attr.cc

+    if (mtx_.size() == 1) {
+      SetOutputPtDim(input_pt_dim_);
+      mtx_.resize(output_pt_dim_ * input_pt_dim_);
+      FillDiag(mtx_, mtx_[0]);


FillDiag sounds like filling only the diagonal, maybe something like MakeDiagonalMat?

klecki · 2020-09-23T14:02:57Z

dali/operators/geometry/mt_transform_attr.cc

+  if (is_fused)
+    translation_.clear();
+
+  if (HasMatrixRegularArg()) {


You will repeat the Regular arguments every iteration even though they do not change between them.

That's deliberate. I assume that at some point we'll allow the values of scalar arguments to change. Not that it would be very useful for this particular operator, but for random number generators, for example, it would have some merit.

klecki · 2020-09-23T14:07:13Z

dali/operators/geometry/mt_transform_attr.cc

+  } else if (HasMatrixInputArg()) {
+    const auto &M_inp = ws.ArgumentInput(name);
+    auto M = view<const float>(M_inp);
+    DALI_ENFORCE(is_uniform(M.shape), "Matrices for all samples int the batch must have "


Suggested change

DALI_ENFORCE(is_uniform(M.shape), "Matrices for all samples int the batch must have "

DALI_ENFORCE(is_uniform(M.shape), "Matrices for all samples in the batch must have "

Happens to me all the time :P

klecki · 2020-09-23T14:12:54Z

dali/operators/geometry/mt_transform_attr.cc

+    Repeat(per_sample_mtx_, mtx_, N);
+    if (is_fused)
+      Repeat(per_sample_translation_, translation_, N);
+  } else if (HasMatrixInputArg()) {


I feel like handling InputArg and regular arg could be two separate functions - this one is a tad long, I have to keep track of the if-nesting.

mzient · 2020-09-23T16:28:28Z

dali/test/python/test_operator_coord_transform.py

@@ -100,13 +100,13 @@ def _run_test(device, batch_size, out_dim, in_dim, in_dtype, out_dtype, M_kind,
                Y = Y.clip(info.min, info.max)

            ref.append(Y)
-            scale = max(scale, np.max(np.abs(Y)) - np.min(np.abs(Y)))
+            scale = max(scale, np.max(np.abs(Y)) - np.min(np.abs(Y))) if Y.size > 0 else 1


this crashed for empty input

mzient · 2020-09-23T17:19:25Z

!build

dali-automaton · 2020-09-23T17:25:43Z

CI MESSAGE: [1648586]: BUILD STARTED

dali-automaton · 2020-09-23T21:25:54Z

CI MESSAGE: [1648586]: BUILD FAILED

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Added tests form MTTransformAttr. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

* Adjust documentation for argument M. * Throw an error if M is empty. * Fix typo. * Check type in SetupImpl. * Improve error message for unsupported type. * Short-circuit if input is empty. * Move layout setting to backend-agnostic part. * Add tests for empty input. * Test layouts. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient · 2020-09-24T07:57:48Z

dali/operators/geometry/coord_flip.cc

-        R"code(Transforms coordinates so that they are flipped (point reflected) with respect
-to a center point.)code")
+        R"code(Transforms vectors or points by flipping (reflecting) their coordinates with
+respect to a given center.)code")


We're not reflecting with respect to a point - we're reflecting with respect to multiple subspaces.

mzient · 2020-09-24T08:06:10Z

!build

dali-automaton · 2020-09-24T08:11:20Z

CI MESSAGE: [1650835]: BUILD STARTED

dali-automaton · 2020-09-24T09:25:11Z

CI MESSAGE: [1650835]: BUILD FAILED

dali-automaton · 2020-09-24T10:07:03Z

CI MESSAGE: [1650835]: BUILD PASSED

mzient requested a review from a team September 21, 2020 09:18

mzient mentioned this pull request Sep 21, 2020

CoordTransform operator #2277

Closed

mzient force-pushed the TransformPointsOp branch 3 times, most recently from 01cfc50 to 3cf0a9c Compare September 21, 2020 15:05

JanuszL reviewed Sep 21, 2020

View reviewed changes

JanuszL approved these changes Sep 21, 2020

View reviewed changes

jantonguirao approved these changes Sep 22, 2020

View reviewed changes

mzient force-pushed the TransformPointsOp branch from bf9725f to 9eb20fb Compare September 22, 2020 21:00

klecki reviewed Sep 23, 2020

View reviewed changes

mzient force-pushed the TransformPointsOp branch from 9a237dd to 38c9152 Compare September 23, 2020 16:24

mzient commented Sep 23, 2020

View reviewed changes

mzient force-pushed the TransformPointsOp branch 2 times, most recently from da6242f to 9e6c2af Compare September 23, 2020 16:30

klecki approved these changes Sep 23, 2020

View reviewed changes

mzient added 7 commits September 24, 2020 09:25

Add CoordTransform operator.

e6d939e

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Rename operators/coord to operators/geometry.

24f4dfd

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

[WIP] Extract transform attrs to a separate class.

1a6fb29

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Extracted matrix & translation arguments MTTransformAttr.

a65d607

Added tests form MTTransformAttr. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Fix review issues.

90a36cd

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Minor fixes in messages / docs.

8410ff4

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient force-pushed the TransformPointsOp branch from 9e6c2af to c5f761a Compare September 24, 2020 07:56

mzient commented Sep 24, 2020

View reviewed changes

mzient merged commit 104da7a into NVIDIA:master Sep 24, 2020

		DALI_SCHEMA(CoordTransform)
		.DocStr(R"(Applies a linear transformation to points or vectors.

	DALI_FAIL(make_string("Unsupported input point dimensionality: ", input_pt_dim_));
	DALI_FAIL(make_string("Unsupported output point dimensionality: ", input_pt_dim_));

	DALI_ENFORCE(mtx_.size() % cols == 0, make_string("Cannot form a matrix ",
	DALI_ENFORCE(mtx_.size() % cols == 0, make_string("Cannot form a matrix with ",


		namespace dali {

		#define COORD_TRANSFORM_INPUT_TYPES (uint8_t, int16_t, uint16_t, int32_t, float)


		If left unspecified, no translation is applied unless MT argument is used.

		The number of components of this vector must match the number of rows in matrix ``M``.

	DALI_ENFORCE(is_uniform(M.shape), "Matrices for all samples int the batch must have "
	DALI_ENFORCE(is_uniform(M.shape), "Matrices for all samples in the batch must have "

Transform points op #2288

Transform points op #2288

Conversation

mzient commented Sep 21, 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

mzient commented Sep 21, 2020

dali-automaton commented Sep 21, 2020

dali-automaton commented Sep 21, 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

mzient commented Sep 22, 2020

dali-automaton commented Sep 22, 2020

dali-automaton commented Sep 22, 2020

mzient commented Sep 22, 2020

dali-automaton commented Sep 22, 2020

dali-automaton commented Sep 22, 2020

dali-automaton commented Sep 23, 2020

klecki left a 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

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 Sep 23, 2020

dali-automaton commented Sep 23, 2020

dali-automaton commented Sep 23, 2020

Choose a reason for hiding this comment

mzient commented Sep 24, 2020

dali-automaton commented Sep 24, 2020

dali-automaton commented Sep 24, 2020

dali-automaton commented Sep 24, 2020