Adding SalientExcerptMixSourceFolder object #228
Conversation
…ied MixSourceFolder object to increase versatility. Simplified a few lines from utils.
…ntent to testcase.
…m the segment_mode.
Codecov Report
@@ Coverage Diff @@
## master #228 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 72 72
Lines 5209 5280 +71
=========================================
+ Hits 5209 5280 +71
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for doing this! I've added inline comments, but I think the main takeaways for me are that I would expect the changes to be contained within the SalientExcerptMixSourceFolder object and related tests, but it seems like there's a bit of spillover. Overall, great work Boaz!
| hop_dur = int(dur * hop_ratio) | ||
| threshold = np.power(10.0, threshold_db / 20.0) | ||
| # adjust the shape of a mono track | ||
| if audio.shape[0] == 1: |
There was a problem hiding this comment.
Does this work with stereo?
There was a problem hiding this comment.
I'll double check, but I believe that the stereo worked fine, but for some reason mono needed to be formatted differently hence the special check.
… from base_dataset, adding padding code to hooks, reverting commit of multitrack, cleaning asserts in tests.
There was a problem hiding this comment.
Overall looks good again. Getting closer. Keep iterating. I think we should nix the padding, as discussed below.
One nit pick is to make sure your code is following PEP8, specifically limiting the maximum line length to 79 chars: https://pep8.org/#maximum-line-length
There was a problem hiding this comment.
Looking really good @boazcogan! Most of my comments are nit picks. I do have some opinions about how you should structure your code & logic for the balancing, but I'll pop into your stand up tomorrow to discuss.
nussl/datasets/hooks.py
Outdated
| folder (str): Location that should be processed to produce the | ||
| list of files. | ||
| salient_src (str): The name of the source that will be used to identify | ||
| salient samples in the dataset. e.g. ('drums') |
There was a problem hiding this comment.
Make sure all of the wrapped lines have the same indent. Some have 4 spaces (or 1 tab) others have 2. Follow the docstrings in the rest of this file. This will be important when we have to build the documentation website.
nussl/datasets/hooks.py
Outdated
| segment_dur (float, optional): the duration of the desired audio clips | ||
| in seconds. Defaults to 4.0. | ||
| hop_ratio (float, optional): size of the hops to use when computing | ||
| the RMS. Defaults to 0.5. |
There was a problem hiding this comment.
Not just the RMS, but more importantly the Segment window.
nussl/datasets/hooks.py
Outdated
| in seconds. Defaults to 4.0. | ||
| hop_ratio (float, optional): size of the hops to use when computing | ||
| the RMS. Defaults to 0.5. | ||
| verbose (bool, optional): suppress progress bar and logging for the |
There was a problem hiding this comment.
This language is a bit ambiguous. Does "Suppress progress bar..." mean that if this arg is True it will suppress the progress bar? For bools, I like to be explicit in the docs: If True, this flag will blah blah. Else, blah blah.
nussl/datasets/hooks.py
Outdated
| self.hop_ratio, | ||
| mix.sample_rate, | ||
| self.threshold_db) | ||
| # this is a fairly cheap operation, no need to embed it within a |
There was a problem hiding this comment.
I don't think you need this comment
nussl/datasets/hooks.py
Outdated
| 'mixsrc_item': item, | ||
| 'start': start / mix.sample_rate | ||
| }) | ||
| # if the balance flag is set then balance the metadata |
There was a problem hiding this comment.
Same with this comment. In general, use inline comments to explain code excerpts that might be difficult to parse like complex logic, a seemingly unorthodox decision, or the expected contents of a data structure. Adding unnecessary comments can clutter up the code.
A more effective comment here would be like # self._balance_set() will take care of the balancing logic. But I'm still not convinced you need a comment here though...
nussl/datasets/hooks.py
Outdated
| mixsrc_item = item['mixsrc_item'] | ||
| # Note that the onset needs to be passed to the AudioSignal | ||
| # class as the kwarg offset. | ||
| onset = item['start'] |
There was a problem hiding this comment.
Why not rename this variable to start = item['start']? That way the code is clearer and we don't need this comment to explain what's happening.
There was a problem hiding this comment.
As it is right now, this variable is referred to by three names in just 3 lines of code: 'start' (in item['start']), onset, and offset (in the process_item kwarg). I'm not sure if there's enough context to name the variable offset here because there are a few layers of abstraction before the offset arg is used directly, so I think start conveys what's happening here well.
Sorry for the long winded explanation; I'm walking you through my thought process.
nussl/datasets/hooks.py
Outdated
| # others | ||
| avg_length = np.mean([elem[0] for elem in sample_counts]) | ||
| metadata = [] | ||
| for count, song, starts, sample_rate in sample_counts: |
There was a problem hiding this comment.
Nit: the AudioSignal might not be a song, so perhaps this is a confusing variable name. item would be better and more consistent with the rest of the object.
nussl/datasets/hooks.py
Outdated
| list of files. | ||
| salient_src (str): The name of the source that will be used to identify | ||
| salient samples in the dataset. e.g. ('drums') | ||
| sample_rate (int, optional): the sampling rate for the audio files. |
There was a problem hiding this comment.
Nit: Make sure to use proper grammar in docstrings, including capitalizing the first word of each sentence.
Fixes #225