bmcfee / pyrubberband

python wrapper for rubberband
ISC License
154 stars 20 forks source link

Added timemap stretch option #15

Closed marcsarfa closed 6 years ago

marcsarfa commented 6 years ago

I added a function that allows users to perform the timemap stretch option of rubberband (see doc here). It is largely based on the current code.

I've also added a test in the tests file.

Here is an example of time map that stretches a 5 seconds audio file by: Leaving second 0 to 1 as is Speeding up seconds 1 to 3 by a factor 2. (it corresponds to seconds 1 to 2 in the stretched audio) Leaving the rest of the audio as is.

time_map = [(0, 0), (sr, sr), (3*sr, 2*sr), (5*sr, 4*sr)]

bmcfee commented 6 years ago

Thanks for implementing this!

It looks like there are some problems with the tests, but I'll give it a code review once they pass.

marcsarfa commented 6 years ago

Yes I couldn't run the tests on my machine. How exactly do you run the tests? Thanks

bmcfee commented 6 years ago
  1. Install pytest
  2. Make sure that your installed rubberband is development, and not the package. That is, pip uninstall pyrubberband and then pip install -e . from the pyrb source directory
  3. Run pytest -v from the pyrb source directory
marcsarfa commented 6 years ago

Yes, that's what I tried but I keep getting the following error:


$ pytest -v
usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --cov-report --cov pyrubberband --pep8
  inifile: /Users/marcs/Documents/pyrubberband/setup.cfg
  rootdir: /Users/marcs/Documents/pyrubberband```
bmcfee commented 6 years ago

Ah, sorry -- you also need the pytest-cov package.

I'll make a note to add contributor guidelines and streamline the test setup.

marcsarfa commented 6 years ago

It's now working

marcsarfa commented 6 years ago

The current version is much cleaner and passes Travis tests successfully. However it fails the coveralls tests because the assertions in timemap_stretch are not tested.

I don't know how to create a @pytest.mark.parametrize that takes a fixture parameter (num_samples in this case) into account.

bmcfee commented 6 years ago

Thanks for the quick turnaround on this!

However it fails the coveralls tests because the assertions in timemap_stretch are not tested.

I don't know how to create a @pytest.mark.parametrize that takes a fixture parameter (num_samples in this case) into account.

I think I don't understand yhat last part -- what exactly do you want it to do with num_samples?

The simplest way to test the value error assertions is to write a separate test function that is marked to fail, and give it a invalid time map fixtures (one with a negative time, one with non-monotonic values, etc).

marcsarfa commented 6 years ago

Everything is working now

bmcfee commented 6 years ago

Looks pretty good! My only remaining quibble is that the os.unlink line is not error-safe. It should be in the finally clause of a try block, so that the file is removed properly if an exception is raised.

marcsarfa commented 6 years ago

All set!

marcsarfa commented 6 years ago

Removed the except clause.

bmcfee commented 6 years ago

Lgtm! Thanks again for implementing this. I'll merge and push out a new release soon.