adamstark / BTrack

A Real-Time Beat Tracker
GNU General Public License v3.0
384 stars 79 forks source link

Fix ComplexSpectralDifference formula, add CSD-Squared #1

Closed zbanks closed 8 years ago

zbanks commented 9 years ago

It looks like there was a mistake in the formula used in calculating the Onset Detection Function, in complexSpectralDifference and complexSpectralDifferenceHWR.

The previous formula computed the difference between two complex numbers x and y as: |x - y| = sqrt((|x| - |y|)^2 + (|x| sin(angle(x, y))^2 )

I changed this to: |x - y| = sqrt(|x|^2 + |y|^2 - 2 |x| |y| cos(angle(x, y)))

I also added complexSpectralDifferenceSquared and complexSpectralDifferenceSquaredHWR, making the latter the default, following the description in the Davies & Plumbley 2007 and 2004 papers. Testing a few problematic songs, it seemed to work better, but I haven't tested it extensively.

I also cleaned up trailing tabs -- sorry for the diff noise.

adamstark commented 8 years ago

(Just a note that I am looking at this - has taken me a while to get around to it and I'll get back to you soon).

adamstark commented 8 years ago

Hi Zach,

I've taken some time to look at this now. You are right about the implementation of complex spectral difference, and that should be updated. It must have been some error I made forever ago during my PhD and I just missed it. The change doesn't affect the performance of the algorithm particularly (it was 73.9% and increases to 74.2% with the fix) but it is closer to the intended implementation (and the implementation used in the original MATLAB version).

The squared versions of the onset detection functions I would rather leave out however - they seriously reduce performance (down to around 61%) on my dataset.

Also, I do plan to improve the code style of the implementation, but i will review that fully in the next update. Just for this amendment, if you could remove the edits to the trailing tabs too, that'd be good.

So perhaps would you be able to tweak the pull request so that it was just the amendments to the equations in the existing complexSpectralDifference and complexSpectralDifferenceHWR functions?

Alternatively - i'm happy to make these changes to the repo myself?

Let me know,

Adam

adamstark commented 8 years ago

I've now made these changes and mentioned you in the commit and README. Thanks for pointing these out!

Adam

zbanks commented 8 years ago

Sorry I missed your post earlier. Thanks for merging it in!

Do you have the dataset for evalutating performance? Is this the code you use to test it? -- https://github.com/adamstark/Beat-Tracking-Evaluation-Toolbox

adamstark commented 8 years ago

Yes, that's the code I use. I do have databases for testing, but I was only given them under the condition that I wouldn't distribute them (unfortunately - because it would be better for everyone if people were testing on the same datasets!). I'll ask around and see what I can do. :)