Added parallel STFT implementation

[JPery]

Hi!

As I comented in #98, I added a parallel STFT implementation based on the map_fn function following the indications of @zaccharieramzi here.

I've added a use_parallel_stft param (disabled by default) that allows to use this function. I've put this param in as many functions as I can. I also added test cases for every function I can, including an specific test that checks that the result of the tf.signal.stft is equals to the result of the parallel_stft function.

Hope this could serve us well meanwhile tensorflow resolves its issues with the fft implementation.

[keunwoochoi]

Thanks a lot for this work. This is a pretty strong PR and all the logics seem making sense to me.
I left some comments but all are easy fixes.

[keunwoochoi]

Agreed that using cpu_count() is better than defaulting to 10. What I meant was to let user control the number of process optionally.

…

On Nov 26, 2020, at 23:56, J. Pery ***@***.***> wrote:  @JPery commented on this pull request. In kapre/backend.py: > @@ -248,3 +252,35 @@ def mu_law_decoding(signal_mu, quantization_channels): tf.math.sign(signal) * (tf.math.exp(tf.math.abs(signal) * tf.math.log1p(mu)) - 1.0) / mu ) return signal + + +def parallel_stft(signals, frame_length, frame_step, fft_length=None, window_fn=window_ops.hann_window, How about adding n_cpu=None in the function signature and it defaults to be multiprocessing.cpu_count()? The tf.map_fn docs say that the default value is 10. I think it's a better approach to use the available cpu number instead. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[keunwoochoi]

@JPery Thanks a lot again! I'm merging it to a temporary branch and will ultimately merge it to the next release.

[oliskir]

Hi,

I have been testing Kapre's STFT layer against the spectrogram computation methods we have implemented in Ketos which are based on numpy's rfft and tile functions.

On CPU, I observe the same behaviour as noted previously by @JPery : Kapre is significantly slower than the numpy-based implementation, taking about 30 times longer to compute spectrograms. (For reference, I'm working on a Intel(R) Core(TM) i5-10210U CPU @ 1.60GHz.)

I have tried switching to the parallel_stft branch and setting use_parallel_stft=True when I create the STFT layer, hoping that this would solve the issue. However, Kapre remains about a factor 30 slower. Is there something that I've misunderstood? I was under the impression that the fix implemented @JPery should lead to faster execution on CPU.

Note: When I run my script with use_parallel_stft=True, tensorflow prints the following warning/info message:

WARNING:tensorflow:Setting parallel_iterations > 1 has no effect when executing eagerly. Consider calling map_fn with tf.function to execute fn in parallel.

Could this be the reason why I am not seeing any improvement?
I'm using tensorflow version 2.7.0.

Thanks,
Oliver

[JPery]

Hi, @oliskir,

Can you try to disable TF eager execution to see if there is any difference?

[oliskir]

Thanks for the suggestion @JPery . Disabling eager execution did the trick:

audio duration: 3.0 s
sampling rate: 12000 Hz
window: 2400 samples, step: 120 samples
spec shape: (281, 1201)

eager execution enabled:

WARNING:tensorflow:Setting parallel_iterations > 1 has no effect when executing eagerly. Consider calling map_fn with tf.function to execute fn in parallel.

batch_size	ketos (s)	kapre (s)	ketos/kapre
1	0.02	0.63	0.04
2	0.04	1.25	0.03
4	0.07	2.49	0.03
8	0.14	5.02	0.03
16	0.28	10.02	0.03

eager execution disabled:

batch_size	ketos (s)	kapre (s)	ketos/kapre
1	0.02	0.11	0.21
2	0.04	0.11	0.37
4	0.07	0.11	0.61
8	0.15	0.12	1.29
16	0.28	0.12	2.34

[oliskir]

I understand that parallel_stft feature will eventually be merged into the master branch of Kapre?
Nice work. Thanks!

[JPery]

Hi, @oliskir.

This is an issue at tensorflow's level. I don't know if they're working on it but it's a reported issue, which I mentioned here.

I don't know what @keunwoochoi wants to do, but, imho, we should wait until this issue is fixed in tensorflow. This is why the parallel implementation is offered in different branch.