search

craffel / mir_eval

Evaluation functions for music/audio information retrieval/signal processing algorithms.
MIT License
588 stars 109 forks source link

fix raw chroma accuracy for unvoiced estimates #312

Closed kukas closed 5 years ago

kukas commented 5 years ago

Fix #311

craffel commented 5 years ago

Thanks for catching this. Happy to merge after @rabitt or @justinsalamon confirm this is correct behavior.

justinsalamon commented 5 years ago

Gave it a quick look (it's late in Europe), in principle fix looks correct, but will try to give a second look tomorrow to confirm.

justinsalamon commented 5 years ago

Didn't have time today, but note that the regression tests fail (due to the change in the chroma metric). I imagine that, assuming the fix is confirmed to be correct, the regression data will have to be updated as part of this PR before it can be merged.

rabitt commented 5 years ago

See my comment above, but in summary: (1) the fix is almost correct, but doesn't work correctly when estimates are reported as negative frequencies. (2) the same bug exists for raw_pitch_accuracy, but in practice, we probably haven't hit any cases where the value of the metric was affected.

craffel commented 5 years ago

Good catch, @kukas do you mind implementing @rabitt's version and then we can merge?

kukas commented 5 years ago

Good catch, @kukas do you mind implementing @rabitt's version and then we can merge?

Sure, I will fix it on Thursday, if it can wait. Thanks @rabitt for the review!

justinsalamon commented 5 years ago

Thanks @rabitt and @juanjobosch for reviewing this and good catch on the neg case (sorry, totally swamped over here with on-boarding).

@kukas note @rabitt renamed correct_voicing to matching_voicing, which I think is a semantically more helpful name :)

kukas commented 5 years ago

Now it should be fixed, I used @rabitt `s patch and applied it also to raw pitch accuracy.

craffel commented 5 years ago

@kukas thanks, you'll need to fix the PEP-8 errors before Travis lets you pass though. https://travis-ci.org/craffel/mir_eval/jobs/490483158#L9358

@rabitt mind giving this a look and LGTM?

craffel commented 5 years ago

Thanks!