implemented pitch contour sonification #195
Conversation
042f9a2 to
6352d2b
Compare
|
@craffel @justinsalamon ready for CR I think. |
|
|
||
| # Clamp the extrapolated values | ||
| x[:int(times.min() * fs)] = 0 | ||
| x[int(times.max() * fs):] = 0 |
There was a problem hiding this comment.
I see why you did this from the test below, but this must create an actual click, right? Is it really that bad to have a small ramp-down/ramp-up at the beginning and/or end (because that's all your avoiding here, right)?
There was a problem hiding this comment.
Actually it's worse than that -- the interpolator seems to be constant-padding instead of using fill_value for extrapolation. Not sure why that is, but it seems incorrect based on my reading of the scipy docs, and it happens in both 0.16.1 and 0.17. This workaround just forces the output to be zero at extrapolation points.
At any rate, the discontinuity here doesn't bother me too much. I can add a ramp if you think it's necessary, but I'd rather keep it simple.
There was a problem hiding this comment.
Actually it's worse than that -- the interpolator seems to be constant-padding instead of using fill_value for extrapolation.
Hmm, seems ok to me:
In [1]: import numpy as np
In [2]: import scipy.interpolate
In [3]: x = np.linspace(.2, 1.1, 10)
In [4]: y = np.random.random_integers(2, 5, 10)
In [5]: f = scipy.interpolate.interp1d(x, y, kind='slinear', fill_value=10., bounds_error=False)
In [6]: f(.15)
Out[6]: array(10.0)Is that not what you'd expect?
At any rate, the discontinuity here doesn't bother me too much.
Well, not explicitly zeroing out doesn't sound worse, right? Unless there is some nasty behavior I am not seeing, it seems like interpolate will already be zeroing out things.
There was a problem hiding this comment.
Is that not what you'd expect?
... that is what i'd expect, and i get the same results when i run your code locally. NFC what's going on inside the synthesizer then; i'll investigate.
Well, not explicitly zeroing out doesn't sound worse, right? Unless there is some nasty behavior I am not seeing, it seems like interpolate will already be zeroing out things.
It should be, and it isn't, unless my tests are really broken. I'll report back shortly.
There was a problem hiding this comment.
Aha! The problem is that testing for zeros at the end of the signal is not correct, since the interpolated frequencies are accumulated through cumsum. This is easily fixable by testing that they're constant though.
There was a problem hiding this comment.
Fixed. Thanks for catching that.
|
One minor comment otherwise LGTM. |
|
|
||
| # Test with an explicit duration | ||
| # This forces the interpolator to go off the end of the sampling grid, | ||
| # which should put zeros at the end of the signal |
There was a problem hiding this comment.
Are these comments still correct?
There was a problem hiding this comment.
If by "signal" we mean "instatnaneous frequencies" then yes; I can clarify though.
39a5282 to
ef94a37
Compare
stupid pep8 minor efficiency improvements in pitch_contour sonification fixed the silly test case in pitch_contour sonify updated test comments in pitch_contour sonification pep8 is stupid
ef94a37 to
2667602
Compare
|
Merging, thanks for being so quick on this! |
|
I think something got screwed up in the git history here? This merge is no longer showing up in the master timeline. The network graph shows the PR still dangling. O_o |
|
Weird, I have no idea why that happened. Just re-merged in #198. Looks ok? Github was having some hiccups earlier this week, maybe it has to do with that? I dunno. |
|
Looks good now, thanks! It's possible that when I ff-pushed to my own fork on the display branch, something got out of sync and/or fat-fingered to upstream (ie craffel/mir_eval). At any rate, all is well now. |
resolves #194