fixing some alignment issues in time-frequency sonification

[bmcfee]

Fixes #408

It appears that fixing the interval padding issue resolved the error with single-interval sonification.

[codecov]

Codecov Report

Attention: Patch coverage is 98.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
mir_eval/sonify.py	98.00%	1 Missing ⚠️

Flag	Coverage Δ
unittests	85.67% <98.00%> (+0.20%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
mir_eval/sonify.py	99.12% <98.00%> (+3.47%)	⬆️

[craffel]

Looks good but get those coverage numbers up to 100%!! 📈

[bmcfee]

Will add tests on this once the dependent stops oozing everywhere and I have the brain cycles to put into it. For now it'd be good if @giovana-morais could kick the tires on this with the downstream jams test that revealed the issue originally.

[giovana-morais]

hey there! most tests are fixed, but there seems to be another corner case. this function breaks whenever we provide a duration=1.0 for the signal (which in mir_eval means length=sr))

import mir_eval
intervals = np.array([[3., 4.]]) 
# works
mir_eval.sonify.chords(["C"], intervals, fs=8000, length=None)  
# breaks
mir_eval.sonify.chords(["C"], intervals, fs=8000, length=8000)
# also breaks
mir_eval.sonify.chords(["C"], intervals, fs=8000, length=8000*2)
# works???
mir_eval.sonify.chords(["C"], intervals, fs=8000, length=8000*3)  

[bmcfee]

That's interesting. I'd think either all of the last 3 should break or they should all work. But I guess we don't have much in the way of intelligent bounds processing here. Maybe this function really just does need a whole rewrite.

[bmcfee]

Ok, I see what's going on here, and the length parameters are actually just surfacing different variations of the same bug as before. Specifically, this line:

    times, _ = util.adjust_intervals(times, t_max=last_time_in_secs)

simultaneously does the expansion and truncation of the time intervals array to fully cover [0, length]. My fix for the initially reported bug hits the case where we needed to also expand the gram array to have the same number of time steps as times (we hadn't been doing this previously!). Where it's failing now is when we have to fully clip out one or more time intervals.

The length=8000 and length=8000*2 cases fail because they are both less than the start of an interval ([80003, 80004]), so adjust_intervals removes an interval from the array and we have a shape mismatch between gram and times. The length=8000*3 case doesn't fail because it just barely touches that last interval, so even though the interval's duration is clipped down to 0, the times array still has the same shape as gram.

[bmcfee]

Ok, I think this PR is good to go, at least for the sake of getting the sonifier to behave sanely.

That said, I think there are some pretty serious issues with the interpolator here:

mir_eval/mir_eval/sonify.py

Lines 190 to 196 in a0a8672

	gram_interpolator = interp1d(
	time_centers,
	gram[n, :n_times],
	kind="linear",
	bounds_error=False,
	fill_value=(gram[n, 0], gram[n, -1]),
	)

we're doing linear interpolation between interval midpoints to sample the amplitudes applied to the synthesized signal. This is fine when intervals are short and equally spaced, but for long and irregular intervals (eg chord annotations), this leads to long and irregular ramps that put the sonified signal well outside the bounds of the original annotation. For example, the motivating use case in 408 had a single annotation spanning [3, 4]. When we prepend a silence from [0, 3], we now end up with a linear ramp up from 1.5 to 3.5, and down from 3.5 to 4. When the length= parameter is used, this can result in divergent outputs for different length values because the fade-out will have varying length.

I think we should replace this with a nearest-neighbor interpolation, which would A) be consistent across different length= settings, and B) more clearly conform to the annotation.

We can take this in a separate issue, but as it's only a 1-liner fix here, I could easily merge it into this PR as well.

[bmcfee]

Since my head is in this code and i have a few minutes now, I went ahead and implemented the nearest neighbor interpolation mode. Since we're now sonifying over the entire time range and then using interpolated gram values to mask out, this leads to transients at interval boundaries. I've put in a hack around this that convolves each nn-interpolated row of gram by an averaging filter with order set to match two cycles of the frequency in question. This gives us a smooth ramp up and down at the boundaries, and constant values within the interval.

Results sound good and seem just as fast as the older implementation. That said, I'm now seeing a bunch of opportunities to vectorize and speed this up further, eg by doing all frequency interpolations at once instead of over a loop. That can definitely be a separate PR.

[bmcfee]

One last couple of updates here:

• nearest neighbor interpolation over interval centers isn't correct, since the beginning of one interval may be closer to the center of a different interval than to its own.
• Using previous interpolation, with times[0, :] as the anchor points does the job correctly.
• This now means that the _const_interpolator is completely unnecessary (and has been removed)
• I lifted the interpolation bits out of the main loop, and the whole thing looks a bit simpler now.

[bmcfee]

It appears that removing _const_interpolator to rely on 1-sample interpolation in previous mode is only supported from scipy 1.10 and up.

I'm personally okay with this, but scipy 1.10 would bump our min python up to 3.8. I think that would probably lift this PR up from a 0.8.1 to a 0.9 feature. In the interest of making maintenance a little easier, I'll revert the const_interpolator hack for now, but leave a comment that it will not be necessary going forward.

[craffel]

Sounds good to me! Thanks for working on this.

[bmcfee]

👍 I've got one more optimization I want to put into this, but don't have time this minute. Will come back to it.