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

sonify.time_frequency regression #371

Closed
bmcfee opened this issue Mar 21, 2024 · 3 comments · Fixed by #374
Closed

sonify.time_frequency regression #371

bmcfee opened this issue Mar 21, 2024 · 3 comments · Fixed by #374
Labels
Milestone

Comments

@bmcfee
Copy link
Collaborator

bmcfee commented Mar 21, 2024

@leighsmith looks like modernizing the tests #370 surfaced a bug in the optimization of time_frequency sonification #355

The bug only seems to show up in the chord sonification, which goes through chord→chroma and chroma→time_frequency. It works fine when length=None is used, but when length is explicitly passed and different from the end of the last time interval, we end up with a length mismatch at
https://github.com/craffel/mir_eval/blob/57f7c31b120f6135c31207295372e3b67848126d/mir_eval/sonify.py#L190-L191 .

We didn't have this problem before #355 because the length was always precisely determined to using matching start:end indices for each time interval.

Since I'm in the midst of revamping all the tests, we're not really in a good place to handle this bugfix as a separate PR yet. If there's an easy fix, I'd like to roll it into #370 so we can merge things in a passing state. Otherwise I might mark that test as a known fail for now and we can come back to it after merging 370.

@bmcfee bmcfee added the bug label Mar 21, 2024
@bmcfee bmcfee added this to the 0.8 milestone Mar 21, 2024
@bmcfee bmcfee mentioned this issue Mar 21, 2024
4 tasks
@bmcfee
Copy link
Collaborator Author

bmcfee commented Mar 21, 2024

Thought this over a bit, and I think I have a not-too-intrusive fix that I can PR after #370 merges.

The basic idea is to decouple the output array allocation length from the input / time grid length. Right now these things are implicitly tangled, and that's causing us trouble.

Unit tests will need to be expanded to cover a few cases:

  • Time indices vs time intervals
  • length inferred (None, should not be a problem)
  • length < max time (in either interval or index case)
  • length > max time (ditto)

@leighsmith
Copy link
Contributor

Yes, good call, totally agree that decoupling the allocation length from the time grid length is the right approach. Sorry, I'm snowed under with other work and can't get to it until the weekend.

bmcfee added a commit to bmcfee/mir_eval that referenced this issue Mar 25, 2024
@bmcfee
Copy link
Collaborator Author

bmcfee commented Mar 25, 2024

I found a cleaner solution here: pull out the sample indices for the interval in question, and index directly into the output array. (This is how it used to work, pre-#355.)

So instead of

output += wave[:length] * interpolator(...)

we do

t_min, t_max = ...
signal = interpolator(np.arange(t_min, t_max))  # len(signal) = t_max - t_min
output[t_min:t_max] += wave[:len(signal)] * signal

Tested out and it works, and we retain the speedups from #355.

This is implemented in #374 and should be good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants