Adding support for single valued rbargs

[adityatb]

Reference Issue

Fixes Issue #21

What does this implement/fix? Explain your changes.

Arguments to CLI, parsed through rbargs can be single or dual valued, but currently breaks if it's single valued. for e.g. pyrb.pitch_shift(wav_data, n_semitones, rbargs={'--formant'}) won't work.

This current implementation allows us to restore that functionality by parsing an empty value '' to the CLI.

This fix allows us to parse flags through in the form of rbargs={'--flag' : ''} while retaining the kwargs format.

Any other comments?

I do feel that this leaves us a bit wanting for something nicer but it's just a step forward to allow some functionality for now. We can explore making the extra argument as None instead of '', but I figured this is probably just as intuitive for these occasional use-cases.

[adityatb]

Hey there! Just your usual internet stranger here, practicing some foo. I'm not sure how the review process works here. I just submitted one to get this little fix moving, but please let me know if I should be doing something different... appreciate your thoughts/inputs, and thanks for a great repo! Cheers! @bmcfee @faroit @waldyrious

[bmcfee]

Thanks @adityatb ! Sorry to be slow on this, but I'll try to give it a review in the next week or so. I have a lot of dev maintenance work queued up for the near future, but I appreciate your patience.

[adityatb]

Thanks @adityatb ! Sorry to be slow on this, but I'll try to give it a review in the next week or so. I have a lot of dev maintenance work queued up for the near future, but I appreciate your patience.

Sure, no worries, I understand. Thanks for the update! 🎉

[bmcfee]

Ok, gave it a look. Seems basically okay to me. I agree that it's not particularly elegant, but it gets the job done. Since @justinsalamon raised the original issue, maybe he could chime in on whether this sufficiently fixes it for him?

[adityatb]

Oh great! Thanks a bunch!

[justinsalamon]

Yes this fixes my issue (and yes it’s not the most elegant but a good interim solution).

Probably missing:

• version bump
• Unit test
• Update docs to document functionality!

thanks!

[bmcfee]

Probably missing:

* version bump

* Unit test

I'll handle version bumps separately; that should be the job of a release PR.

Unit tests: yeah probably :)

[adityatb]

Hey @bmcfee @justinsalamon

Sorry to be MIA. The warm weather here means I'm finally out of hibernation.
I just added in some unit tests and updated the docs. I haven't written a lot of tests before, so I'm not sure if they're up to spec here. Let me know if they're good to go!

A small note, the case {'-c-': '5'} is the default case according to the Rubberband docs, so I've included that with some other arbitrary cases.

[nullmightybofo]

I just stumbled over this as well, great that this is being addressed!
(Maybe splitting options into 2 variables, i.e., passing key:value options using a dict, and passing options without parameter as a list would also be an option? Not sure it would be a better alternative, and more code change necessary)

Would changes here "automatically" move over to librosa as well? (Not that important oc, as perfectly fine to use this package directly).

BR

[bmcfee]

Thanks @nullmightybofo for nudging this - it had fallen off my radar.

@adityatb the tests look good to me! I agree that packing flags and valued arguments into a dictionary structure is a little inelegant, but I can live with it.

Would changes here "automatically" move over to librosa as well?

No; librosa does not use or depend on pyrb. It has its own completely different (and, I dare say, much worse) implementation of pitch shift and time stretch.

[nullmightybofo]

Thanks @nullmightybofo for nudging this - it had fallen off my radar.

@adityatb the tests look good to me! I agree that packing flags and valued arguments into a dictionary structure is a little inelegant, but I can live with it.

Would changes here "automatically" move over to librosa as well?

No; librosa does not use or depend on pyrb. It has its own completely different (and, I dare say, much worse) implementation of pitch shift and time stretch.

Well, thanks first of all for this handy tool, certainly makes using rubberband easier!

I saw the link to this package's time-stretch function in the librosa doc ("sse also" section here https://librosa.org/doc/main/generated/librosa.effects.time_stretch.html#librosa.effects.time_stretch) and didn't realize it leads to this package here! Instead I thought that it is used in librosa as a submodule as well, hence my question above.

[bmcfee]

This totally fell off the radar, but now seems like a good time to do some maintenance merging. Thanks again for the PR!