Adding saving of sources, take 3 #55

pseeth · 2019-04-03T06:58:14Z

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 · 2019-04-03T07:10:39Z

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

justinsalamon · 2019-04-05T23:56:09Z

@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:

Have an idea / encounter a problem
Create an issue
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)

Once consensus has been reached, implement the changes as agreed upon.
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 · 2019-04-06T01:44:38Z

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 · 2019-04-06T01:45:49Z

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 · 2019-04-06T02:08:58Z

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 · 2019-04-06T02:29:39Z

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 · 2019-04-06T02:53:18Z

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 · 2019-04-06T03:14:15Z

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 · 2019-04-08T04:12:49Z

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 · 2019-04-08T04:18:03Z

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

…the stricter test case is commented out

justinsalamon

OK @pseeth I've given it a first CR pass. Here are the key issues we need to tackle:

Nomenclature

We can't use "sources" for event audio files, since "source_file" is already used to refer to the audio file from which an event is taken. My recommendation is to replace "source(s)" everywhere in the code and docstrings with "isolated_event(s)"
"audio_path" is way too generic. But since we'll be moving it out of the event spec, I'll comment on that further down.
there are a few other variables/functions (e.g. "generate_scaper_scene") whose names should be updated to something more specific & meaningful.

API design

Right now the path for the isolated event audio output is stored in the event_spec, but I don't believe that's the right place for it. Conceptually, the event spec should store all and only that information needed to generate an event. In the same way generate() and generate_from_jams() take user input for setting the path for output files, I think the path for the output isolated event audio files should be specified at generation time and not stored in the event specification. These values can be added to the JAMS files for logging purposes.
So, basically my proposal is that when calling generate or generate_from_jams, the user also specifies a single folder for the isolated outputs, e.g. isolated_event_folder. The code then creates two subfolders in that path ("foreground" and "background") and stores all the isolated events into these folders. We then add "isolated_event_folder" to ann.sandbox.scaper, much like fg_path and bg_path etc.
The other question is how to name the isolated event audio files. From the code it looks like right now it just keeps the random tempfile names? Would it make sense for the isolated events to follow a naming convention? E.g., it could be the event label (e.g. "siren"), followed by an incremental index number (so "siren_0.wav", "siren_1.wav", "siren_2.wav" etc.). Whatcha think?

Docs

Once we finalize the above, we'll have to write a tutorial for using this feature. But let's figure out 1 and 2 first :)

justinsalamon · 2020-02-05T00:31:06Z

.gitignore

+tests/separation/*/foreground/*.wav
+tests/separation/*/background/*.wav
+tests/separation/mix_sources/foreground/*.wav
+tests/separation/mix_sources/background/*.wav


Are these audio files part of the test suite for source separation? If so, should they not be part of the repo?

this was leftover from an early version - deleted.

justinsalamon · 2020-02-05T00:32:30Z

scaper/core.py

@@ -38,7 +38,7 @@
 EventSpec = namedtuple(
    'EventSpec',
    ['label', 'source_file', 'source_time', 'event_time', 'event_duration',
-     'snr', 'role', 'pitch_shift', 'time_stretch'])
+     'snr', 'role', 'pitch_shift', 'time_stretch', 'audio_path'])


audio_path seems too vague (first line I'm reviewing in core.py so not even sure what this will be used for). Whatever it is it needs a more meaningful name :)

justinsalamon · 2020-02-05T00:36:03Z

scaper/core.py

@@ -47,7 +47,7 @@


 def generate_from_jams(jams_infile, audio_outfile, fg_path=None, bg_path=None,
-                       jams_outfile=None):
+                       jams_outfile=None, save_sources=False):


Generally in Scaper, since we're composing the scene, I'm a little unsure about using the term sources here to refer to the audio of the individual sound events. Would be curious to hear your take here.

Perhaps a more descriptive name for this flag would be save_isolated_events ?

justinsalamon · 2020-02-05T00:38:41Z

scaper/core.py

+    save_sources : bool
+        If True, this will save the sources in a directory adjacent to the generated
+        mixture. The sources sum up to the mixture. Sources can be found at 
+        `[audio_path]_sources/foreground` and `[audio_path]_sources/background'. 
+        Source audio names follow the pattern: `[role]_[label][count]`, where count 
+        is the index of the source in self.fg_spec (this allows sources of the same 
+        label to be added more than once to the soudscape without breaking things).


Per the previous comment, we need to choose some nomenclature here. I'm wary of using "sources" because the term "source" is already used for "source_file" (i.e. the file from which an event will be taken), so using "source" both for input files and output files will be confusing. As per the previous comment, how about save_isolated_events?

Also, this docstring makes reference to audio_path but the function doesn't have an audio_path input argument? Is this a reference to the audio_path stored in the event spec? That doesn't seem like the right place for that?

justinsalamon · 2020-02-05T00:39:13Z

scaper/core.py

@@ -144,10 +151,11 @@ def generate_from_jams(jams_infile, audio_outfile, fg_path=None, bg_path=None,

    # Generate audio and save to disk
    reverb = ann.sandbox.scaper['reverb']
-    sc._generate_audio(audio_outfile, ann, reverb=reverb,
+    sc._generate_audio(audio_outfile, ann, reverb=reverb, save_sources=save_sources,


save_isolated_events or whatever alternative we decide on.

justinsalamon · 2020-02-05T01:47:02Z

tests/test_core.py

@@ -877,7 +930,8 @@ def test_scaper_instantiate_event():
                         snr=('uniform', 10, 20),
                         role='foreground',
                         pitch_shift=('normal', 0, 1),
-                         time_stretch=('uniform', 0.8, 1.2))
+                         time_stretch=('uniform', 0.8, 1.2),
+                         audio_path='foreground/0')


justinsalamon · 2020-02-05T01:47:09Z

tests/test_core.py

@@ -862,7 +914,8 @@ def test_scaper_add_event():
                                  snr=('uniform', 10, 20),
                                  role='foreground',
                                  pitch_shift=('normal', 0, 1),
-                                  time_stretch=('uniform', 0.8, 1.2))
+                                  time_stretch=('uniform', 0.8, 1.2),
+                                  audio_path='foreground/0')


justinsalamon · 2020-02-05T01:47:14Z

tests/test_core.py

@@ -832,7 +883,8 @@ def test_scaper_add_background():
                                  snr=("const", 0),
                                  role='background',
                                  pitch_shift=None,
-                                  time_stretch=None)
+                                  time_stretch=None,
+                                  audio_path='background/0')


tests/test_core.py

justinsalamon · 2020-02-05T01:58:48Z

tests/notebooks/create_test_data.ipynb

@@ -115,7 +115,7 @@
  },
  {
   "cell_type": "code",
-   "execution_count": 6,
+   "execution_count": 9,


Not sure what significant changes there are in this file, but to be revisited after redesign

This notebook is now gone, replaced with a script at tests/create_test_data.py.

justinsalamon

Overall it's mostly solid, various wording fixes required and a few code-related concerns to address. Tnx!

docs/tutorial.rst

scaper/core.py

tests/create_test_data.py

tests/test_audio.py

justinsalamon · 2020-02-21T23:15:44Z

tests/test_core.py

@@ -1371,6 +1425,114 @@ def _test_generate_audio(SR, REG_WAV_PATH, REG_BGONLY_WAV_PATH, REG_REVERB_WAV_P
        regwav, sr = soundfile.read(REG_BGONLY_WAV_PATH)
        assert np.allclose(wav, regwav, atol=atol, rtol=rtol)

+def create_scaper_scene():


…scape mixture audio. (+16 squashed commits) Squashed commits: [6313fce] caught a showstopper with seeding...strengthened the test and passed it [d7188ca] changing tut [d43438f] simplifying compare_scaper_jams [b56b3ee] making the test more robust [4b13c02] adding separation stuff to tutorial [1919a4a] matching source lengths with mix and adding pysoundfile as dep [15ae372] updating source -> isolated event in test [cb511d8] use slice [d48077f] stray audio_paths [98c1af9] adding a cast for the sandbox [0b62874] 100 cov with changes. moving test data creation to a script [061bd2c] adding save_sources to generate_from_jams [b3917fb] making this test case work in older python [ee900f1] commenting out funky test case [f453b01] fixing how scaper jams are compared when the EventSpec changes [be4e332] whoops, if conditional in wrong spot

justinsalamon

Code is good. All minor comments about docstrings. I'll probably fix these myself.

docs/tutorial.rst

scaper/audio.py

scaper/core.py

tests/create_regression_data.py

justinsalamon · 2020-02-22T01:27:01Z

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

Minor docstring fixes following #55 + version bump

justinsalamon mentioned this pull request Apr 6, 2019

Output sources as separate files alongside mixture #52

Closed

justinsalamon mentioned this pull request Apr 8, 2019

Support default values for distribution tuples #41

Open

works with a test case, but there are still strange off by 1 errors. …

bab0d38

…the stricter test case is commented out

pseeth force-pushed the separation branch from b742267 to 061bd2c Compare February 3, 2020 01:33

justinsalamon requested changes Feb 5, 2020

View reviewed changes

pseeth mentioned this pull request Feb 13, 2020

Time stretching causes source separation options to crash #72

Open

justinsalamon mentioned this pull request Feb 15, 2020

Sources are off by 1 sample. #73

Closed

pseeth mentioned this pull request Feb 19, 2020

ann.sandbox.scaper is sometimes a dictionary, sometimes a jams.Sandbox #77

Open

pseeth force-pushed the separation branch from d4a820a to d43438f Compare February 19, 2020 06:49

This was referenced Feb 19, 2020

Replace sox with in-memory processing #76

Closed

Unexpected warnings with dumping sources alongside mix #75

Open

Generating background from short segments #70

Closed

Putting code from the tutorial in a runnable script #78

Open

justinsalamon requested changes Feb 21, 2020

View reviewed changes

pseeth force-pushed the separation branch from f0da24a to 1e091cd Compare February 21, 2020 23:54

justinsalamon reviewed Feb 22, 2020

View reviewed changes

justinsalamon merged commit dc0d8f9 into justinsalamon:master Feb 22, 2020

justinsalamon added a commit that referenced this pull request Feb 22, 2020

Minor docstring fixes following #55

f633b0e

justinsalamon added a commit that referenced this pull request Feb 22, 2020

Merge pull request #79 from justinsalamon/docs55

62f7c30

Minor docstring fixes following #55 + version bump

justinsalamon mentioned this pull request Feb 27, 2020

Output audio format should match input format #87

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding saving of sources, take 3 #55

Adding saving of sources, take 3 #55

pseeth commented Apr 3, 2019 •

edited by justinsalamon

Loading

coveralls commented Apr 3, 2019 •

edited

Loading

justinsalamon commented Apr 5, 2019 •

edited

Loading

justinsalamon commented Apr 6, 2019

justinsalamon commented Apr 6, 2019

pseeth commented Apr 6, 2019

justinsalamon commented Apr 6, 2019

pseeth commented Apr 6, 2019

pseeth commented Apr 6, 2019

justinsalamon commented Apr 8, 2019

pseeth commented Apr 8, 2019

justinsalamon left a comment

justinsalamon Feb 5, 2020

pseeth Feb 19, 2020

justinsalamon Feb 5, 2020

pseeth Feb 19, 2020

justinsalamon Feb 5, 2020

justinsalamon Feb 5, 2020

justinsalamon Feb 5, 2020

justinsalamon Feb 5, 2020

justinsalamon Feb 5, 2020

justinsalamon Feb 5, 2020

justinsalamon Feb 5, 2020

pseeth Feb 19, 2020

justinsalamon left a comment

justinsalamon Feb 21, 2020

justinsalamon left a comment •

edited

Loading

justinsalamon commented Feb 22, 2020 •

edited

Loading

Adding saving of sources, take 3 #55

Adding saving of sources, take 3 #55

Conversation

pseeth commented Apr 3, 2019 • edited by justinsalamon Loading

coveralls commented Apr 3, 2019 • edited Loading

justinsalamon commented Apr 5, 2019 • edited Loading

justinsalamon commented Apr 6, 2019

justinsalamon commented Apr 6, 2019

pseeth commented Apr 6, 2019

justinsalamon commented Apr 6, 2019

pseeth commented Apr 6, 2019

pseeth commented Apr 6, 2019

justinsalamon commented Apr 8, 2019

pseeth commented Apr 8, 2019

justinsalamon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsalamon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsalamon left a comment • edited Loading

Choose a reason for hiding this comment

justinsalamon commented Feb 22, 2020 • edited Loading

pseeth commented Apr 3, 2019 •

edited by justinsalamon

Loading

coveralls commented Apr 3, 2019 •

edited

Loading

justinsalamon commented Apr 5, 2019 •

edited

Loading

justinsalamon left a comment •

edited

Loading

justinsalamon commented Feb 22, 2020 •

edited

Loading