resample type option added in pitch_shift() #863
Conversation
|
Thanks @tae898 for this! Changes so far seem good, though I think it should also be applied to There's also a bit of overlap (in scope) with (WIP) pr #857, where res_type modes have been expanded. I suspect this one will merge before #857, so I'm happy to smooth over the differences over there. |
librosa/effects.py
Outdated
| @@ -287,13 +290,15 @@ def pitch_shift(y, sr, n_steps, bins_per_octave=12): | |||
| ... bins_per_octave=24) | |||
| ''' | |||
|
|
|||
| if bins_per_octave < 1 or not np.issubdtype(type(bins_per_octave), np.integer): | |||
| if bins_per_octave < 1 or not np.issubdtype(type(bins_per_octave), | |||
There was a problem hiding this comment.
not sure why git diff picks up these two lines? did anything change?
There was a problem hiding this comment.
looks like OP's text editor automatically wrapped a line here. NBD.
There was a problem hiding this comment.
not sure why git diff picks up these two lines? did anything change?
Oh yeah cuz I made a new line before np.integer to comply with the "80 char per line limit"
Thanks for the feedback. Btw, what do you mean by that it should also be applied to 'time_stretch"? time_stretch() doesn't call core.resample, but only pitch_shift() does. So I don't think I can add, or there is no need to add, an optional 'res_type' argument to it? |
sorry, you're completely correct! (I was thinking of a similar pair of functions in a separate project.) |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lostanlen)
There was a problem hiding this comment.
Dismissed @lostanlen from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @bmcfee)
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
|
LGTM, thanks again for implementing this! |
Reference Issue
What does this implement/fix? Explain your changes.
As for resampling, 'kaiser_best' can take twice as much time as 'kaiser_fast'. This can be a bottle neck in some applications where a lot of resampling calls are made. It'd be nice if this is made optional.
Any other comments?
This change is