Simpler and faster resampling

[adefossez]

This is the implementation for #1057
Implementation is taken from julius/resample.py and adapted for consistency with the previous implementation. See the issues linked above for a comparison in speed.
Output should be exactly the same as I changed the default parameters to match those of the previous implementation.

[mthrok]

Thanks for working on this. I made some comments.

[mthrok]

In my understanding, rolloff was the constant 0.99 in the original implementation.
If so, why not keep it that way? What's the necessity to export it as public API?

[adefossez]

I was thinking it could be useful to power users (rolloff being always a trade off between limiting aliasing while not losing too much signal, and there is not perfect value). It is not so different from allowing the end user to change lowpass_filter_width, and it does not break backward compatibility. But if you prefer I can remove it.

[adefossez]

I have renamed the parameter to lowpass_cutoff_ratio for clarity, if you agree on keeping it in the api :)

[mthrok]

I have somewhat mixing feeling about adding the new parameter. But I am not DSP expert so I cannot judge. If you go strong on adding it, I do not have a strong counter argument other than keeping API same. However, the way you described, I was thinking it could be useful to power users sounds it's still the assumption, it would be nice if we had some evidence.

Is there a library that expose similar parameter? I know that lowpass_filter_width is originated from Kaldi.

[mthrok]

Looking at the CI jobs, the tests which were feeding float sampling rate are now failing.

Can I suggest to split this PR into the three PRs?

1. The speed up part
2. Changing sampling rate from float to int
3. Adding parameter for lowpass_cutoff_ratio

Then, the speed up part should be merged easily, and for the others I can bring them up in our team meeting to get more feedback.

[adefossez]

I'm wondering whether I should keep the assertion that the sample rates are indeed integers (even if typed as float). People might be using the current implementation with true floats, and in that case my code would throw an exception, while the previous one would not, and the change would not be backward compatible.

In the original code, a mixture of integer and float values was used. I don't think this is completely accurate as for instance resampling from 2 to 2.9999 is going to be quite different from resampling from 2 to 3. I could try to emulate this in the new implementation but I need a bit more time to make sure I am doing the same thing as the previous implementation.

I would need to add some form of unit tests because non integer change of sample rate are not covered in the tests. I could add a saved file from the current implementation as a test resource, but I'm afraid this would scare future contributors from trying to change that bit, while in fact the current implementation is doing something a bit weird and somewhat incorrect.

[vincentqb]

I'm wondering whether I should keep the assertion that the sample rates are indeed integers (even if typed as float). People might be using the current implementation with true floats, and in that case my code would throw an exception, while the previous one would not, and the change would not be backward compatible.

In the original code, a mixture of integer and float values was used. I don't think this is completely accurate as for instance resampling from 2 to 2.9999 is going to be quite different from resampling from 2 to 3. I could try to emulate this in the new implementation but I need a bit more time to make sure I am doing the same thing as the previous implementation.

I would need to add some form of unit tests because non integer change of sample rate are not covered in the tests. I could add a saved file from the current implementation as a test resource, but I'm afraid this would scare future contributors from trying to change that bit, while in fact the current implementation is doing something a bit weird and somewhat incorrect.

As mentioned in #891, we do wish to change sample rate to floats in torchaudio. Parts of the code that assume integer sample rate should check that condition to avoid silent correctness issue later.

I second @mthrok suggestion above to split out the change from int to float (and the new parameter).

[adefossez]

@vincentqb for some reason I thought the old code was doing something weird if the sample rate were floats, which I was worried about, but actually it was equivalent to just converting those with int first, which is what the new code does too.
So I've removes here everything change but the change to the algorithm itself, and this is functionally equivalent to the previous implementation.

[adefossez]

Ping @vincentqb @mthrok :) let me know if you are good with the current version, it is completely equivalent to the previous one.

[vincentqb]

This LGTM, thanks!

[mthrok]

@adefossez Sorry for not getting back to you sooner. Let me come back to this the next week.

[mthrok]

@adefossez

Sorry for not getting back to you sooner. The PR looks good. Thanks!