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 warning for non-integer resampling frequencies #1490
Add warning for non-integer resampling frequencies #1490
Conversation
cdc8aaf
to
5f3feed
Compare
torchaudio/functional/functional.py
Outdated
"so please manually convert frequencies to integer values that maintain the ratio" | ||
"prior to passing them into the function" | ||
"Example: To downsample a 44100 hz waveform by a factor of 8, you can use" | ||
"`orig_freq=8` and `new_freq=1` instead of `orig_freq=44100` and `new_freq=5512.5`" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can go farther and say it will raise an error in the future.
@cpuhrsch what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine if we throw an exception in the next release, but I'd create a pre-emptive issue for this and link it here so that users can chime in whether they want this or not. The overall topic seems to be "non-integral sampling rates", so maybe we can link to an umbrella issue like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you plan the future changes and make a note in #1487, then mention that in the message ?
cc626c6
to
665694b
Compare
665694b
to
5c65489
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks!
assert len(w) == 1 | ||
|
||
|
||
class FunctionalComplex(TestBaseMixin): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carolineechen I think this was introduced by mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in #1524
Add warning and suggestion when the frequency inputs to the resample function are not integer values
cc #1487