Adding saving of sources, take 3

[pseeth]

I closed my old PR #43 and just implemented the source separation functionality using the latest code. I know #52 also exists but it was a bit easier to take some ideas from that PR and incorporate it into this one I was already working on.

This PR has test cases that test for isolated sources adding up to the mixture.

One issue is that the regression data has changed because the NamedTuple now has an additional field (audio_path). So I regenerated the regression data for the latest files from the recent PR #40.

Another issue is that the EventSpec has changed making comparing to the older regression data difficult. To fix this, I've edited _compare_scaper_jams to manipulate EventSpec tuples before comparing them to get rid of fields that are not in the regression data.

It appears that the off by 1 error is still sneaking around somewhere. The test right now compares the mixture like this:

        # Here's a very strict test (this FAILS with because of off by 1 errors)
        assert np.allclose(mix, sum(sources), atol=1e-4, rtol=1e-8) 

        # and a less strict test, which looks at the minimum length and only compares those. PASSES.
        min_length = min(s.shape[0] for s in sources)
        summed = 0
        for s in sources:
            summed += s[:min_length]
        assert np.allclose(mix[:min_length], summed, atol=1e-4, rtol=1e-8) 

---

This change is 

[coveralls]

Coverage remained the same at 100.0% when pulling 1e091cdd94c5e754d427ceab56553d44a315fcef on pseeth:separation into 7fb77a92cf9fda15aae7defb5a3d826f87877b4e on justinsalamon:master.

[justinsalamon]

@pseeth @bensteers thanks for both PRs, this one seems most advanced at this stage so lets consolidate efforts and work off this one moving forward. I will review shortly.

Sidenote: all changes/updates to the API should be discussed/agreed upon prior to any code commits. So for future reference the preferred order of events would be:

0. Have an idea / encounter a problem
1. Create an issue
2. Discuss issue to make sure it's worth a PR

3a. Reach consensus on how the issue should be addressed, especially if it involves changing/updating the API, then open a PR

OR

3b. Open a PR and discuss implementation details there to reach consensus (prior to any code commits)

4. Once consensus has been reached, implement the changes as agreed upon.
5. Ping for a code review once the PR passes all checks (unit tests and coverage).

A good example of reaching consensus prior to modification is issue #36 and the related PR #54.

Thanks!

[justinsalamon]

The off-by-1 error was caused due to an issue with how the duration of time-stretched events was calculated. A fix has since been merged into master, so you should be able to use the strict tests.

[justinsalamon]

Let's work on merging in the other 2 PRs (#54 and #53), and once those have been merged we can work on merging this one.

[pseeth]

Hey Justin, thanks for all the reviews today! Super appreciated. I'll read through all the comments soon and try to fix. For the last part, re: off by one errors, these are branched off of the master that had the fix, I think. The problem seems to still be sneaking around somewhere.

[justinsalamon]

Hey Justin, thanks for all the reviews today! Super appreciated.

👍 👍 (calling it a day for today, more soon)

I'll read through all the comments soon and try to fix. For the last part, re: off by one errors, these are branched off of the master that had the fix, I think. The problem seems to still be sneaking around somewhere.

Dayum, OK, we should try and pin those down.

[pseeth]

Let's work on merging in the other 2 PRs (#54 and #53), and once those have been merged we can work on merging this one.

Yeah this makes perfect sense. For reference, I've merged these into my own master just to see if merges would be clean and they have some complications. The easiest path was to merge #53, then #54, then #55.

[pseeth]

Oh so the test case I made for this kind of sucks in terms of making temporary files. I couldn't figure out how to use temporary files to make directories that the data generated by the test case could be stored in, so what I do is make data in the tests/ directory and then clean it up after the test is done or fails with a variant of _close_tmp_files. This is kinda meh but if anyone has any suggestions for how to use NamedTemporaryFile-like things for this, I'm all ears!

[justinsalamon]

The easiest path was to merge #53, then #54, then #55.

OK, let's work to merge these in this order then. I've added some comment on the first two.

I couldn't figure out how to use temporary files to make directories that the data generated by the test case could be stored in.

We should avoid writing data to disk and try and stick to temp files/folders. Is this what you're looking for by any chance? https://github.com/justinsalamon/scaper/blob/master/tests/test_core.py#L493

[pseeth]

Oh brilliant, that might work! Thanks! I'll try to update the test case with that soon.

[justinsalamon]

All remaining comments are just minor fixes to docstrings. I'll merge and then make these fixes myself. Awesome job @pseeth !!