Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PowerSpectrum CPU operator #1460

Merged
merged 33 commits into from
Nov 27, 2019
Merged

Conversation

jantonguirao
Copy link
Contributor

@jantonguirao jantonguirao commented Nov 12, 2019

Why we need this PR?

Need for a PowerSpectrum CPU operator

What happened in this PR?

  • Power spectrum CPU implemented in terms of Fft1DCpu kernel

JIRA TASK: [DALI-1148]

@jantonguirao jantonguirao changed the title Fft op impl [WIP] PowerSpectrum CPU Nov 12, 2019
@jantonguirao jantonguirao force-pushed the fft_op_impl branch 5 times, most recently from 84a7d68 to 9505613 Compare November 15, 2019 14:06
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
…about data layout (different layouts not yet tested)

Signed-off-by: Joaquin Anton <janton@nvidia.com>
…e any dimension in the input data)

Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
…lex FFT spectrum

Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao jantonguirao changed the title [WIP] PowerSpectrum CPU Add PowerSpectrum CPU operator Nov 20, 2019
@jantonguirao
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1003248]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1003248]: BUILD FAILED

kernels::KernelContext ctx;
auto in_shape = input.shape();
int nsamples = input.size();
auto nthreads = ws.HasThreadPool() ? ws.GetThreadPool().size() : 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@klecki - is it possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked with him and it is safe to assume that we have a thread pool. So I'd change that

shape = waveform.shape

out_shape = list(shape)
out_shape[axis] = nfft/2+1
Copy link
Contributor

@JanuszL JanuszL Nov 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will differ between python2 and python3

Copy link
Contributor Author

@jantonguirao jantonguirao Nov 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix

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

@szalpal szalpal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, I'd rebuild the structure a little bit, so that it's more suitable for future GPU operator. My proposal:

template<typename Backend>
class PowerSpectrum : Operator<Backend> {
  bool CanInferOutputs() const override { return true; }
  kernels::KernelManager kmgr_;
  kernels::signal::fft::FftArgs fft_args_;
};

class PowerSpectrumCpu: PowerSpectrum<CPUBackend> {
  bool SetupImpl(std::vector<OutputDesc> &output_desc, const workspace_t<CPUBackend> &ws) override;
  void RunImpl(workspace_t<CPUBackend> &ws) override;
};

But the more important question is: could the nfft argument be per-sample?

dali/operators/signal/CMakeLists.txt Show resolved Hide resolved
template <typename Backend>
class PowerSpectrum : public Operator<Backend> {
public:
explicit inline PowerSpectrum(const OpSpec &spec)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
explicit inline PowerSpectrum(const OpSpec &spec)
explicit PowerSpectrum(const OpSpec &spec) // NOLINT

If method is defined in class body it's implicitly inline. I believe that declaration is redundant. Also linter complaints on explicit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will change

template <>
bool PowerSpectrum<CPUBackend>::SetupImpl(std::vector<OutputDesc> &output_desc,
const workspace_t<CPUBackend> &ws) {
output_desc.resize(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
output_desc.resize(1);
output_desc.resize(kNumOutputs);

That way it's clearer what the 1 actually is

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

DALI_SCHEMA(PowerSpectrum)
.DocStr(R"code(Power spectrum of signal.)code")
.NumInput(1)
.NumOutput(1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.NumOutput(1)
.NumOutput(kNumOutputs)

I think this magic number can be described better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

using Operator<Backend>::RunImpl;

kernels::KernelManager kmgr_;
kernels::signal::fft::FftArgs fft_args_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
kernels::signal::fft::FftArgs fft_args_;
const kernels::signal::fft::FftArgs fft_args_;

This would provide good sanity check, that the args are consistent through all the calls

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as you can see in the constructor, I need to set the members individually with a switch. I'd have to create temporaries instead if I marked this const. Doable but it'd decrease readability, I think

#include "dali/kernels/signal/fft/fft_cpu.h"
#include "dali/pipeline/data/views.h"

#define FFT_SUPPORTED_NDIMS (1, 2, 3)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only these particular dims are supported? Is it kernel's restrictions?

Copy link
Contributor Author

@jantonguirao jantonguirao Nov 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is restricted at the kernels side. Also, this is what would (and a little beyond) make sense for PowerSpectrum. You are calculating the power spectrum on either a 1D signal, or 2D with the second dimension being the number of channels. 3 dims is actually quite unlikely but one might have the windows already extracted (or provided externally) and use the PowerSpectrum operator to generate the Spectrogram

output_desc[0].shape.set_tensor_shape(i, req.output_shapes[0][0].shape);
}
), // NOLINT
(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parenthesis is redundant I guess ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How so? Can I have this fail case without the parenthesis?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Work on my machine ;)

), DALI_FAIL(make_string("Unsupported output type: ", output_type_))) // NOLINT

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Signed-off-by: Joaquin Anton <janton@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1008058]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1008058]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1008058]: BUILD PASSED

@jantonguirao jantonguirao merged commit 319b113 into NVIDIA:master Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants