Enable support for different layouts in the MelFilterBank GPU Op #2620

banasraf · 2021-01-18T16:55:10Z

Signed-off-by: Rafal Banas.Rafal97@gmail.com

Why we need this PR?

It adds a support for the layouts other than #ft in the MelFilterBank GPU Op. Needed for time-major layout support.

What happened in this PR?

What solution was applied:
The frequency axis is queried from the layout string
Affected modules and functionalities:
Operators impl.
Key points relevant for the review:
docstring, tests
Validation and testing:
I've extended the existing tests to run on "tf" layout
Documentation (including examples):
Docstring updated

JIRA TASK: NA

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

banasraf · 2021-01-18T17:01:55Z

!build

dali-automaton · 2021-01-18T17:05:53Z

CI MESSAGE: [1991582]: BUILD STARTED

JanuszL · 2021-01-18T17:10:18Z

dali/operators/audio/mel_scale/mel_filter_bank.cc

-the fft bin index and the window index, respectively.
+Expects an input with at least 2 dimensions.
+
+Please note that the CPU implementation supports only the layout with the last two


How hard would be to rework CPU implementation as well?

It should use a separate implementation of the actual processing loop, but it should some 30-50 lines or so (if the kenrel isn't totally botched).

I plan to implement the CPU part. I can do it by contributing to this PR so that we don't introduce the note about the lack of support in the CPU version

It's not a problem in nightly, IMO. What I would not like to see is this partial support making it into tagged release.

dali-automaton · 2021-01-18T18:59:00Z

CI MESSAGE: [1991582]: BUILD PASSED

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

mzient · 2021-01-19T17:14:34Z

dali/kernels/audio/mel_scale/mel_filter_bank_cpu.cc

    int nfilter = args_.nfilter;

    std::memset(out, 0, sizeof(T) * nfilter * nwindows);
    for (int64_t fftbin = fftbin_start_; fftbin <= fftbin_end_; fftbin++) {
-      auto *in_row_start = in + fftbin * in_stride;
+      auto *in_row_start = in + fftbin * nwindows;


Why? Having strides had some potential for more generic usage.

mzient · 2021-01-19T17:16:53Z

dali/kernels/audio/mel_scale/mel_filter_bank_cpu.cc

+        int f2 = interval_ends_[m + 2];
+        for (; fftbin < f1; ++fftbin) {
+          auto weight_up = T(1) - weights_down_[fftbin];
+          if (args_.normalize)


Please move it after the loops as val *= norm_factors_[m];

mzient · 2021-01-20T08:39:53Z

dali/operators/audio/mel_scale/mel_filter_bank.cc

@@ -23,8 +23,9 @@ DALI_SCHEMA(MelFilterBank)
    .DocStr(R"code(Converts a spectrogram to a mel spectrogram by applying a bank of
 triangular filters.

-Expects an input with at least 2 dimensions where the last two dimensions correspond to
-the fft bin index and the window index, respectively.
+Expects an input with at least 2 dimensions.


This should not be a requirement - I mean, you can just calculate a single Mel spectrum (not necessarily a spectrogram).
It can be worked around by adding artificial dimensions for use by the kernels.
If, on the other hand, there are more than 2 dimensions, all trailing and leading dimensions can be collapsed.
Can we even handle a case when f is somewhere in the middle - think, AfB layout?

The GPU kernel supports any f axis

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

mzient · 2021-01-20T18:21:47Z

dali/kernels/audio/mel_scale/mel_filter_bank_cpu.cc

+  while (axis > 1) {
+    in_shape = collapse_dim(in_shape, 0);
+    out_shape = collapse_dim(out_shape, 0);
+    axis--;
+  }
+  while (axis < in_shape.size() - 2) {
+    in_shape = collapse_dim(in_shape, in_shape.size() - 2);
+    out_shape = collapse_dim(out_shape, out_shape.size() - 2);
+  }


Minor:

Suggested change

while (axis > 1) {

in_shape = collapse_dim(in_shape, 0);

out_shape = collapse_dim(out_shape, 0);

axis--;

}

while (axis < in_shape.size() - 2) {

in_shape = collapse_dim(in_shape, in_shape.size() - 2);

out_shape = collapse_dim(out_shape, out_shape.size() - 2);

}

if (axis > 1) {

in_shape = collapse_dims(in_shape, {std::make_pair(0, axis)});

out_shape = collapse_dims(out_shape, {0, axis});

axis = 1;

}

if (axis < in_shape.size() - 2) {

in_shape = collapse_dims(in_shape, {std::make_pair(axis+1, in_shape.size()-axis-1)});

out_shape = collapse_dims(out_shape, {std::make_pair(axis+1, in_shape.size()-axis-1)});

}

mzient · 2021-01-20T18:26:06Z

dali/kernels/audio/mel_scale/mel_filter_bank_cpu.cc

+    in_shape = collapse_dim(in_shape, in_shape.size() - 2);
+    out_shape = collapse_dim(out_shape, out_shape.size() - 2);
+  }
+  bool is_freq_last = axis == in_shape.size() - 1 || in_shape[in_shape.size() - 1] == 1;


mzient · 2021-01-20T18:28:19Z

dali/kernels/audio/mel_scale/mel_filter_bank_gpu.cu

@@ -122,22 +122,28 @@ class MelFilterBankGpu<T, Dims>::Impl : public MelFilterImplBase<T, Dims> {
    }
  }

-  void Setup(ScratchpadEstimator &se, const TensorListShape<Dims> &in_shape) {
+  void Setup(ScratchpadEstimator &se, TensorListShape<> in_shape) {


Suggested change

void Setup(ScratchpadEstimator &se, TensorListShape<> in_shape) {

void Setup(ScratchpadEstimator &se, const TensorListShape<> &in_shape) {

It looks like you're not modifying it - no need to copy.

Leftovers from a previous change I removed finally. Thanks.

mzient · 2021-01-20T18:29:50Z

dali/test/python/test_operator_mel_filter_bank.py

+                        (128, 16000.0, 0.0, 8000.0, (10, 513, 100), 'Ctf'),
+                        (128, 48000.0, 4000.0, 24000.0, (513, 100), 'tf'),
+                        (128, 44100.0, 0.0, 22050.0, (513, 100), 'tf'),
+                        (128, 44100.0, 1000.0, 22050.0, (513, 100), 'tf')]:


Add a test with 1D input.

JanuszL · 2021-01-20T22:25:11Z

dali/operators/audio/mel_scale/mel_filter_bank.cc

+  auto ndim = in_shape.sample_dim();
+  args_.axis = layout.empty() ? std::max(0, ndim - 2) : layout.find('f');
+  DALI_ENFORCE(args_.axis >= 0 && args_.axis < ndim,
+    make_string("'f' axis not present in the layout. Got: ", layout));


Suggested change

make_string("'f' axis not present in the layout. Got: ", layout));

make_string("'f' axis not present in the layout. Got: `", layout, "`"));

JanuszL · 2021-01-20T22:26:04Z

dali/operators/audio/mel_scale/mel_filter_bank_gpu.cc

+  auto ndim = in_shape.sample_dim();
+  args_.axis = layout.empty() ? std::max(0, ndim - 2) : layout.find('f');
+  DALI_ENFORCE(args_.axis >= 0 && args_.axis < ndim,
+    make_string("'f' axis not present in the layout. Got: ", layout));


Suggested change

make_string("'f' axis not present in the layout. Got: ", layout));

make_string("'f' axis not present in the layout. Got: `", layout, "`"));

JanuszL · 2021-01-20T22:26:44Z

dali/kernels/audio/mel_scale/mel_filter_bank_cpu.cc

+    interval_ends_.resize(nfilter + 2);
+    interval_ends_[0] = fftbin_start_;
+    interval_ends_[nfilter + 1] = fftbin_end_ + 1;
+    for (int interval = 1; interval < nfilter + 1; interval++, mel += mel_delta_) {


Can this loop be merged with previous one?

It could be, but I see no reason to do it. know it's initialization only, but the inner loop would likely break the optimizations in the inner one (as the compiler would deem it less frequent).
Also, it would contain an ugly if (interval == 0)

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

jantonguirao · 2021-01-21T11:48:29Z

!build

dali-automaton · 2021-01-21T11:51:18Z

CI MESSAGE: [2001716]: BUILD STARTED

dali-automaton · 2021-01-21T13:32:10Z

CI MESSAGE: [2001716]: BUILD PASSED

Enable other layouts support. Extend tests.

eb12e10

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

JanuszL self-assigned this Jan 18, 2021

JanuszL reviewed Jan 18, 2021

View reviewed changes

JanuszL approved these changes Jan 18, 2021

View reviewed changes

awolant self-assigned this Jan 19, 2021

awolant approved these changes Jan 19, 2021

View reviewed changes

Enable time-major layout in MelFilterBank CPU implementation

f714b22

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

JanuszL self-requested a review January 19, 2021 17:12

awolant self-requested a review January 19, 2021 17:13

mzient reviewed Jan 19, 2021

View reviewed changes

mzient reviewed Jan 20, 2021

View reviewed changes

JanuszL approved these changes Jan 20, 2021

View reviewed changes

awolant approved these changes Jan 20, 2021

View reviewed changes

jantonguirao added 2 commits January 20, 2021 18:44

Code review fixes and removing static ndims from Mel Filter Bank kernels

e3bc841

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

Allow empty layouts

cb1f0c8

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

mzient reviewed Jan 20, 2021

View reviewed changes

JanuszL reviewed Jan 20, 2021

View reviewed changes

1D test

6483b2c

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

JanuszL approved these changes Jan 21, 2021

View reviewed changes

Code review fixes

6fc1b7d

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

jantonguirao merged commit a2f39ba into NVIDIA:master Jan 21, 2021

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

Enable support for different layouts in the MelFilterBank GPU Op #2620

Enable support for different layouts in the MelFilterBank GPU Op #2620

banasraf commented Jan 18, 2021

banasraf commented Jan 18, 2021

dali-automaton commented Jan 18, 2021

JanuszL Jan 18, 2021

mzient Jan 18, 2021

jantonguirao Jan 19, 2021

mzient Jan 19, 2021

dali-automaton commented Jan 18, 2021

mzient Jan 19, 2021

mzient Jan 19, 2021 •

edited

Loading

mzient Jan 20, 2021

banasraf Jan 20, 2021 •

edited

Loading

mzient Jan 20, 2021 •

edited

Loading

mzient Jan 20, 2021

mzient Jan 20, 2021

jantonguirao Jan 21, 2021

mzient Jan 20, 2021

JanuszL Jan 20, 2021

JanuszL Jan 20, 2021

JanuszL Jan 20, 2021

mzient Jan 21, 2021 •

edited

Loading

jantonguirao commented Jan 21, 2021

dali-automaton commented Jan 21, 2021

dali-automaton commented Jan 21, 2021

-  while (axis > 1) {
-    in_shape = collapse_dim(in_shape, 0);
-    out_shape = collapse_dim(out_shape, 0);
-    axis--;
-  }
-  while (axis < in_shape.size() - 2) {
-    in_shape = collapse_dim(in_shape, in_shape.size() - 2);
-    out_shape = collapse_dim(out_shape, out_shape.size() - 2);
-  }
+if (axis > 1) {
+  in_shape = collapse_dims(in_shape, {std::make_pair(0, axis)});
+  out_shape = collapse_dims(out_shape, {0, axis});
+  axis = 1;
+}
+if (axis < in_shape.size() - 2) {
+  in_shape = collapse_dims(in_shape, {std::make_pair(axis+1, in_shape.size()-axis-1)});
+  out_shape = collapse_dims(out_shape, {std::make_pair(axis+1, in_shape.size()-axis-1)});
+}

	void Setup(ScratchpadEstimator &se, TensorListShape<> in_shape) {
	void Setup(ScratchpadEstimator &se, const TensorListShape<> &in_shape) {

	make_string("'f' axis not present in the layout. Got: ", layout));
	make_string("'f' axis not present in the layout. Got: `", layout, "`"));

Enable support for different layouts in the MelFilterBank GPU Op #2620

Enable support for different layouts in the MelFilterBank GPU Op #2620

Conversation

banasraf commented Jan 18, 2021

Why we need this PR?

What happened in this PR?

banasraf commented Jan 18, 2021

dali-automaton commented Jan 18, 2021

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

dali-automaton commented Jan 18, 2021

Choose a reason for hiding this comment

mzient Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banasraf Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

mzient Jan 20, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

jantonguirao commented Jan 21, 2021

dali-automaton commented Jan 21, 2021

dali-automaton commented Jan 21, 2021

mzient Jan 19, 2021 •

edited

Loading

banasraf Jan 20, 2021 •

edited

Loading

mzient Jan 20, 2021 •

edited

Loading

mzient Jan 21, 2021 •

edited

Loading