Some functions added #19
Conversation
|
Thanks for doing this! It appears that the CI build is (mostly) failing due to changes in the pytest API since I last worked on this package. We should address those in a separate PR, but in the meantime I'll leave some comments on this one. |
|
Quick update: I've fixed the pytest issues, so if you could rebase to the current master, the travis build should work again. |
| return __rubberband(y, sr, **rbargs) | ||
|
|
||
|
|
||
| def change_tempo(y, sr, X, Y, rbargs=None): |
There was a problem hiding this comment.
how is this different from time_stretch?
There was a problem hiding this comment.
It is not, it just saves the step of creating the coefficient between the two tempos.
pyrubberband/pyrb.py
Outdated
|
|
||
| def change_tempo(y, sr, X, Y, rbargs=None): | ||
| '''Multiply the frequencies inside an audio time series. | ||
| The equivalent of the -f option. |
There was a problem hiding this comment.
Ohhh, my bad, problems of doing copy-paste. Thanks for noticing it.
Added functions were not in global import
| Sampling rate of `y` | ||
|
|
||
| X : float | ||
| Shift magnitudes in spectrum by X |
There was a problem hiding this comment.
Can this parameter have a more descriptive name? X doesn't convey much here, maybe factor or shift would be more appropriate.
The description of this parameter (and the function docstring itself) is also a bit confusing. I assume this means that energy at frequency f maps to frequency f*factor? I think it would be best to be explicit about what the exact calculation is.
There was a problem hiding this comment.
I agree, factor seems a better name.
The rubberband-cli documentation just says Change frequency by multiple X. I was looking in the rubberband repository, and I guess by this line that this does f*factor. I think we could safely describe this parameter as you said.
pyrubberband/pyrb.py
Outdated
|
|
||
| def change_tempo(y, sr, X, Y, rbargs=None): | ||
| '''Multiply the frequencies inside an audio time series. | ||
| The equivalent of the -f option. |
There was a problem hiding this comment.
This doesn't seem accurate; copypasta?
Again, X and Y are not descriptive variable names, and particularly confusing given that there's already a lower-case y parameter.
There was a problem hiding this comment.
Yes! It was a copypasta and I committed the changes already. I guess original_tempo and final_tempo are better names than X and Y.
|
|
||
| rbargs.setdefault('--tempo', '%s:%s' % (X, Y)) | ||
|
|
||
| return __rubberband(y, sr, **rbargs) |
There was a problem hiding this comment.
Can you add unit tests for this new functionality?
There was a problem hiding this comment.
Let me take a look at how do they work! For change_tempo and frequency_shift, right?
Reference Issue
No reference issue
What does this implement/fix? Explain your changes.
Implement a function that were not wrapped:
frequency multiply: To multiply all frequencies by a certain factor, helpful with some stuff that shifting by semitones cannot achieve. Is the -f option in the CLI
tempo shift: Instead of calculating manually the ratio conversion for the tempo, use the function already included in rubberband