Add DCT 1D CPU kernel #1569

jantonguirao · 2019-12-12T15:40:35Z

Why we need this PR?

It adds new DCT kernel, needed for the MFCC operator

What happened in this PR?

Explain solution of the problem, new feature added.
Created a DCT 1D CPU kernel implementation. The kernel takes an arbitrarily sized tensor and performs a DCT transformation over the selected axis.
What was changed, added, removed?
Dct1DCpu kernel, tests
What is most important part that reviewers should focus on?
The kernel implementation
Was this PR tested? How?
Unit tests
Were docs and examples updated, if necessary?
Doxygen

JIRA TASK: [DALI-1185]

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

jantonguirao · 2019-12-13T18:08:03Z

!build

dali-automaton · 2019-12-13T18:10:22Z

CI MESSAGE: [1032164]: BUILD STARTED

dali-automaton · 2019-12-13T20:22:06Z

CI MESSAGE: [1032164]: BUILD PASSED

JanuszL · 2019-12-13T23:55:25Z

dali/kernels/signal/dct/dct_args.h

+ * @brief DCT kernel arguments
+ */
+struct DctArgs {
+  /// @brief DCT type. Supported types are 1, 2, 3, 4


What this value maps to?

It doesn't map to anything. See:
https://en.wikipedia.org/wiki/Discrete_cosine_transform#Formal_definition

So this reference should be put here.

It's in the operator documentation, but ok, I'll add it here as well

dali/kernels/signal/dct/dct_cpu.cc

JanuszL · 2019-12-14T00:05:22Z

dali/kernels/signal/dct/dct_cpu.cc

+               make_string("Axis is out of bounds: ", args.axis));
+  int64_t n = in.shape[args.axis];
+
+  if (args.dct_type == 1) {


We have that check repeated in FillCosineTableTypeI. Do we need to have that in two places?

It's ok - this is a public API, so we throw - in FillCosineTable we assert (it should not be reachable with such parameter, regardless of what external caller does)..

JanuszL · 2019-12-14T00:10:45Z

dali/kernels/signal/dct/dct_cpu_test.cc

+
+template <typename T>
+void ReferenceDctTypeI(span<T> out, span<const T> in, bool normalize) {
+  int64_t in_length = in.size();


Shouldn't we put similar assert as in the implementation?

mzient · 2019-12-16T09:34:14Z

dali/kernels/signal/dct/dct_cpu.cc

+  for (int64_t k = 0; k < ndct; k++) {
+    T norm_factor = (k == 0) ? factor_k_0 : factor_k_i;
+    for (int64_t n = 0; n < input_length; n++) {
+      table[idx++] = norm_factor * std::cos(phase_mul * (n + T(0.5)) * k);


If it's a one-off calculation, then the code below give more precision:

Suggested change

table[idx++] = norm_factor * std::cos(phase_mul * (n + T(0.5)) * k);

table[idx++] = T(norm_factor * std::cos(phase_mul * (n + 0.5) * k);

mzient · 2019-12-16T09:34:46Z

dali/kernels/signal/dct/dct_cpu.cc

+
+template <typename T>
+void FillCosineTableTypeII(T *table, int64_t input_length, int64_t ndct, bool normalize) {
+  T phase_mul = M_PI / input_length;


Suggested change

T phase_mul = M_PI / input_length;

double phase_mul = M_PI / input_length;

mzient · 2019-12-16T09:36:26Z

dali/kernels/signal/dct/dct_cpu.cc

+  T factor_k_0 = 1, factor_k_i = 1;
+  if (normalize) {
+    factor_k_i = std::sqrt(2 / T(input_length));
+    factor_k_0 = factor_k_i / std::sqrt(T(2));


Isn't it just:

Suggested change

factor_k_0 = factor_k_i / std::sqrt(T(2));

factor_k_0 = std::sqrt(input_length);

?

1 / std::sqrt(input_length) actually. Changed it

mzient · 2019-12-16T09:37:01Z

dali/kernels/signal/dct/dct_cpu.cc

+void FillCosineTableTypeI(T *table, int64_t input_length, int64_t ndct, bool normalize) {
+  assert(input_length > 1);
+  assert(!normalize);
+  T phase_mul = M_PI / (input_length - 1);


Suggested change

T phase_mul = M_PI / (input_length - 1);

double phase_mul = M_PI / (input_length - 1);

mzient · 2019-12-16T09:37:33Z

dali/kernels/signal/dct/dct_cpu.cc

+  T phase_mul = M_PI / input_length;
+  T factor_n_0 = 0.5, factor_n_i = 1;
+  if (normalize) {
+    factor_n_i = std::sqrt(T(2) / input_length);
+    factor_n_0 = factor_n_i / std::sqrt(T(2));
+  }
+  int64_t idx = 0;
+  for (int64_t k = 0; k < ndct; k++) {
+    table[idx++] = factor_n_0;  // n = 0
+    for (int64_t n = 1; n < input_length; n++) {
+      table[idx++] = factor_n_i * std::cos(phase_mul * n * (k + T(0.5)));
+    }
+  }


mzient · 2019-12-16T09:37:43Z

dali/kernels/signal/dct/dct_cpu.cc

+  T phase_mul = M_PI / input_length;
+  T factor = normalize ? std::sqrt(T(2)/input_length) : T(1);
+  int64_t idx = 0;
+  for (int64_t k = 0; k < ndct; k++) {
+    for (int64_t n = 0; n < input_length; n++) {
+      table[idx++] = factor * std::cos(phase_mul * (n + T(0.5)) * (k + T(0.5)));
+    }
+  }


...and again.

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

jantonguirao · 2019-12-18T14:03:32Z

!build

dali-automaton · 2019-12-18T14:05:45Z

CI MESSAGE: [1038528]: BUILD STARTED

dali-automaton · 2019-12-18T15:41:00Z

CI MESSAGE: [1038528]: BUILD PASSED

jantonguirao requested a review from a team December 12, 2019 15:40

jantonguirao changed the title ~~Add DCT 1D CPU kernel~~ [WIP] Add DCT 1D CPU kernel Dec 12, 2019

jantonguirao changed the title ~~[WIP] Add DCT 1D CPU kernel~~ Add DCT 1D CPU kernel Dec 13, 2019

jantonguirao added 6 commits December 13, 2019 19:02

DCT kernel

0330834

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

Linter fixes

db9d0fb

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

Add 2D layout tests

5d2ba76

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

Orthogonal normalization

93418ec

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

Reuse cosine table

e21c094

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

Fix lint

3952d38

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

jantonguirao force-pushed the dct_cpu_kernel branch from bce6537 to 3952d38 Compare December 13, 2019 18:03

JanuszL reviewed Dec 13, 2019

View reviewed changes

dali/kernels/signal/dct/dct_cpu.cc Show resolved Hide resolved

JanuszL reviewed Dec 14, 2019

View reviewed changes

JanuszL approved these changes Dec 14, 2019

View reviewed changes

mzient reviewed Dec 16, 2019

View reviewed changes

Code review fixes

2c94cea

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

awolant approved these changes Dec 18, 2019

View reviewed changes

jantonguirao merged commit a23606c into NVIDIA:master Dec 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DCT 1D CPU kernel #1569

Add DCT 1D CPU kernel #1569

jantonguirao commented Dec 12, 2019 •

edited

jantonguirao commented Dec 13, 2019

dali-automaton commented Dec 13, 2019

dali-automaton commented Dec 13, 2019

JanuszL Dec 13, 2019

mzient Dec 16, 2019

JanuszL Dec 16, 2019

jantonguirao Dec 18, 2019

JanuszL Dec 14, 2019

mzient Dec 16, 2019

JanuszL Dec 14, 2019

jantonguirao Dec 18, 2019

mzient Dec 16, 2019

mzient Dec 16, 2019

mzient Dec 16, 2019

jantonguirao Dec 18, 2019

mzient Dec 16, 2019

mzient Dec 16, 2019

mzient Dec 16, 2019

jantonguirao commented Dec 18, 2019

dali-automaton commented Dec 18, 2019

dali-automaton commented Dec 18, 2019

	table[idx++] = norm_factor * std::cos(phase_mul * (n + T(0.5)) * k);
	table[idx++] = T(norm_factor * std::cos(phase_mul * (n + 0.5) * k);

	T phase_mul = M_PI / input_length;
	double phase_mul = M_PI / input_length;

	factor_k_0 = factor_k_i / std::sqrt(T(2));
	factor_k_0 = std::sqrt(input_length);

	T phase_mul = M_PI / (input_length - 1);
	double phase_mul = M_PI / (input_length - 1);

Add DCT 1D CPU kernel #1569

Add DCT 1D CPU kernel #1569

Conversation

jantonguirao commented Dec 12, 2019 • edited

Why we need this PR?

What happened in this PR?

jantonguirao commented Dec 13, 2019

dali-automaton commented Dec 13, 2019

dali-automaton commented Dec 13, 2019

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

jantonguirao commented Dec 18, 2019

dali-automaton commented Dec 18, 2019

dali-automaton commented Dec 18, 2019

jantonguirao commented Dec 12, 2019 •

edited