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

fix resample type hint #888

Closed
wants to merge 1 commit into from
Closed

Conversation

faroit
Copy link
Contributor

@faroit faroit commented Aug 24, 2020

As indicated in the docstrings, the sample rates would need to be float. I also discovered that using int can result in an error

  File "/Users/faro/repositories/open-unmix-pytorch-dev/env/lib/python3.7/site-packages/torch/nn/modules/module.py", line 722, in _call_impl
    result = self.forward(*input, **kwargs)
  File "/Users/faro/repositories/open-unmix-pytorch-dev/env/lib/python3.7/site-packages/torchaudio/transforms.py", line 596, in forward
    waveform = kaldi.resample_waveform(waveform, self.orig_freq, self.new_freq)
  File "/Users/faro/repositories/open-unmix-pytorch-dev/env/lib/python3.7/site-packages/torchaudio/compliance/kaldi.py", line 930, in resample_waveform
    window_width, lowpass_cutoff, lowpass_filter_width, device, dtype)
  File "/Users/faro/repositories/open-unmix-pytorch-dev/env/lib/python3.7/site-packages/torchaudio/compliance/kaldi.py", line 807, in _get_LR_indices_and_weights
    assert lowpass_cutoff < min(orig_freq, new_freq) / 2
RuntimeError: Integer division of tensors using div or / is no longer supported, and in a future release div will perform true division as in Python 3. Use true_divide or floor_divide (// in Python) instead.

@vincentqb
Copy link
Contributor

vincentqb commented Aug 26, 2020

The convention we've been using so far as been int as you can see for instance in MFCC transform. To maintain the same behavior, we should replace the division by floor_divide. Do you want to open a pull request for this? Do you have a code to reproduce the error you mention?

#890

@faroit
Copy link
Contributor Author

faroit commented Aug 26, 2020

The convention we've been using so far as been int as you can see for instance in MFCC transform

I actually used int for decades but recently learned that float is more appropriate and mathematically correct. It is also often used in resamplers e.g, in sox.

To maintain the same behavior, we should replace the division by floor_divide. Do you want to open a pull request for this?

yes, but in any case, either the docstring or the type hint of thins function would need to changed as well.

@vincentqb
Copy link
Contributor

Thanks for the feedback. I'd be happy to update the code to floats, #891. However, this may interfere with C++ extensions when saving.

The current behavior of this particular assertion (in torchaudio) is to do integer division, so I'd expect to use floor_division. That being said, this code is ported from Kaldi: do you know if the original intent was floor or true division?

@mthrok
Copy link
Collaborator

mthrok commented May 25, 2021

This has been resolved in #1499 (part of #1487)

@mthrok mthrok closed this May 25, 2021
mpc001 pushed a commit to mpc001/audio that referenced this pull request Aug 4, 2023
[FX] updated NNC_compile example
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants