Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crackle in sonify.time_frequency #255

Merged
merged 3 commits into from
Jul 23, 2017

Conversation

rabitt
Copy link
Contributor

@rabitt rabitt commented Jul 17, 2017

This addresses @craffel's comments on #224.

The main changes are:

  1. Replaces @stefan-balke's asymmetric ramp between frames with an interpolator so the amplitudes will change smoothly (and symmetrically) over time.
  2. Fixes a second cause of noisiness/buzzing I found in _fast_synthesize. The line:
    n_samples = int(10.0 * fs / frequency)
    is meant to generate 10 periods of the wave at f0=frequency, and the generated wave is copied. For non-integer values of frequency, 10. * fs / frequency is not necessarily an integer, so casting it to an integer results in generating less than 10 periods of the wave, and then there is a discontinuity when the wave is copied. I did a hack to fix this problem and force n_samples to always be an integer.

If this is merged, #224 can be closed. cc @stefan-balke

@rabitt rabitt changed the title Sonify ramp Fix crackle in sonify.time_frequency Jul 17, 2017
@rabitt rabitt changed the title Fix crackle in sonify.time_frequency Fix crackle in sonify.time_frequency Jul 17, 2017
@rabitt
Copy link
Contributor Author

rabitt commented Jul 17, 2017

@craffel I think this is good to go (on my end).

frequency = float(frequency)
# hack so that we can ensure an integer number of periods and samples
# rounds frequency to 1st decimal, s.t. 10 * frequency will be an int
frequency = np.round(frequency, 1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this mean that only integer frequencies are sonifiable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No -- but i'm rounding each frequency to the nearest 10s place, so it might have an audible effect on higher frequencies.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

offline conversation summary: might be better to make the number of decimals a parameter n_dec, and maintain a table of size 10**n_dec * fs so that a user can control the precision of the approximation.

Copy link
Collaborator

@craffel craffel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up! Two small comments.


# Sum into the aggregate output waveform
output[start:end] += wave[start:end] * gram[n, m]
weights = gram_interpolator(np.arange(start, end))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like weights is unused? Did you mean to replace gram_interpolator(np.arange(start, end)) with weights below?

else:
# if there is only one point in time_centers, interpolator
# returns constant
gram_interpolator = _const_interpolator(gram[n, 0])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you replace all this logic with

gram_interpolator = interp1d(
    time_centers, gram[n, :],
    kind='linear' if len(time_centers) > 1 else 'nearest',
    bounds_error=False, fill_value=0.0)

or maybe 'zero' instead of 'nearest'? Alternatively if you can provide a little more of a comment for special casing, would be helpful for understanding it.

@rabitt
Copy link
Contributor Author

rabitt commented Jul 19, 2017

@craffel I ran into a snag in the interpolation when len(time_centers)=1.

Using kind='nearest':

gram_interpolator = interp1d(
    time_centers, gram[n, :],
    kind='linear' if len(time_centers) > 1 else 'nearest',
    bounds_error=False, fill_value=0.0)

it fails because when kind='nearest' it expects time_centers and gram[n, :] to have more than 1 point.

Using kind='zero':

gram_interpolator = interp1d(
    time_centers, gram[n, :],
    kind='linear' if len(time_centers) > 1 else 'zero',
    bounds_error=False, fill_value=0.0)

The interpolator always returns zero -- for example:

f = interp1d([1], [3], kind='zero', bounds_error=False, fill_value=0.0)
f(-1)
>> array(0.0)
f(0)
>> array(0.0)
f(1)
>> array(2.1904428788e-314)
f(2)
>> array(0.0)

What do you want the expected behavior to be? For a length n output, do you want the same set of pitches to be sustained, or something else?

@craffel
Copy link
Collaborator

craffel commented Jul 19, 2017

Hmm, those are frustrating behaviors of interp1d. Oh well, you can go back to what you had!

fix edge case where  has only one point
make n_dec a parameter
@craffel craffel merged commit 5dc8b1c into mir-evaluation:master Jul 23, 2017
@craffel
Copy link
Collaborator

craffel commented Jul 23, 2017

Thanks @rabitt !

craffel pushed a commit that referenced this pull request Jun 22, 2018
* linear interpolation between frames

* fix sinewave discontinuity; replace ramp with interpolator
fix edge case where  has only one point
make n_dec a parameter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants