Qm shift 1/3

[daschuer]

Testing your patches, I have noticed that the lower border frequency of C3 has no Gauss bell form, because the lower 1/3 bin is missing.
Now the detection starts one bin earlier. This makes the code also easier to understand IMHO.

[cannam]

The 1/3 bin shift seems like a sensible idea - I'll take a look, thanks.

With regard to style fixes, please refer to the codestyle-and-tidy branch, into which I am currently accumulating a large number of code style fixes.

[daschuer]

OK, the style fixes where all included in the codestyle-and-tidy branch.
I have removed the commit here.

[daschuer]

It was probably not too sensible.
Now my 440 Hz test wave is off by ~ -10 ¢
There must be an other rounding issue somewhere

[daschuer]

OK, I have only tricked myself and tested a version without the first commit.
This one fixes the off by ~ -10 ¢ issue:

only the first commit. The 440 Hz signal is almost centered.

raw chroma: 
2.1864e-06 1.98328e-06 2.3192e-06 
3.22382e-06 2.58403e-06 2.77546e-06 
3.48479e-06 4.99738e-06 2.26164e-06 
3.63347e-06 3.80177e-06 1.71302e-06 
2.82505e-06 4.37589e-06 2.38337e-06 
3.37997e-06 4.08856e-06 2.25101e-06 
2.07105e-06 3.23208e-06 4.72169e-06 
4.41947e-06 5.44775e-06 3.95811e-06 
7.21986e-06 0.0036204 0.432004 
1 0.44585 0.00541741 
3.78441e-06 5.83631e-06 3.84269e-06 
3.64386e-06 2.46435e-06 2.48727e-06

Both commits. The 440 Hz is shifted by one bins and still almost centered.

raw chroma: 
2.08649e-06 2.18636e-06 1.98326e-06 
2.31919e-06 3.22381e-06 2.58403e-06 
2.77548e-06 3.4848e-06 4.99737e-06 
2.26162e-06 3.63351e-06 3.80177e-06 
1.71299e-06 2.82514e-06 4.37597e-06 
2.38334e-06 3.38004e-06 4.08852e-06 
2.25108e-06 2.07109e-06 3.23214e-06 
4.72168e-06 4.4196e-06 5.44783e-06 
3.95839e-06 7.22041e-06 0.0036207 
0.432011 1 0.445844 
0.0054171 3.78408e-06 5.83602e-06 
3.84263e-06 3.64383e-06 2.46433e-06

First commit removed: The 440 Hz is off pitch.

raw chroma: 
1.34152e-06 2.23106e-06 2.05634e-06 
1.92096e-06 3.04384e-06 2.58473e-06 
3.41087e-06 3.61391e-06 5.37381e-06 
2.91883e-06 3.62912e-06 3.14179e-06 
1.5454e-06 2.21627e-06 4.28804e-06 
2.5081e-06 3.36046e-06 3.84076e-06 
2.26447e-06 1.44288e-06 3.10097e-06 
3.92471e-06 4.09539e-06 3.42547e-06 
2.19289e-06 6.30409e-06 0.00293616 
0.430991 1 0.501545 
0.0164639 7.53878e-06 2.99073e-06 
3.81511e-06 2.4753e-06 2.66593e-06 

[cannam]

An update about this fix:

I submitted two versions of the QM Key Detector Vamp plugin to the MIREX 2019 evaluation of music analysis methods. The first was the same version as had been submitted in previous years. The second was "v5" of the plugin, which is not yet an officially released version of the plugin, but which was built against the current qm-dsp code with this fix in it (rev 9b18a22). I acknowledged your fix in the submission notes.

The results have just been published, and can be found here: https://www.music-ir.org/mirex/wiki/2019:Audio_Key_Detection_Results. The two versions are labelled as submissions CN1 and CN2 respectively in that page. As you can see, the version with your fix performs substantially better than the previous one in all datasets - a nice real-world validation. Thank you again for reporting and working on this.

[daschuer]

Good news. Thank you for the update.

Did the comment and close button strikes you? I think this PR is still relevant.

[cannam]

Oh sorry - I did close it, but I think I had probably lost track of what was in the PR. Re-opening, will try to take another look.

[Holzhaus]

Oh sorry - I did close it, but I think I had probably lost track of what was in the PR. Re-opening, will try to take another look.

Ping. We already hoped we could drop our custom patches when 1.8.0 landed, but then we saw that this has not been merged yet.

[JoergAtGithub]

@cannam It would be nice to get an official bugfix release of qm-dsp containing the PRs #3 and #6.

[cannam]

There are two problems here - neither insurmountable but both real.

The first is that I left QM in 2020, and so can't really make an official release. I have continued to maintain some stuff voluntarily, e.g. things you can find within the sonic-visualiser Github organisation, most of which I wrote. But this library is something that I worked on maintaining while at QM but that was largely written by other people, so it feels rather different.

That said, I am fairly sure that nobody else is about to make an official release either, and I can of course commit to this repo, so I could create a not-quite-an-official-release tag here if that would be good enough for your purposes.

The second problem - and the reason I didn't get around to merging these before - is testing. The other (more substantial) set of fixes from Daniel that preceded this PR did get merged and tested, but when it came to looking again at this one I could no longer remember what its test status was.

I'm wary (having made mistakes before) of merging even obviously correct patches without checking that they don't make the results worse - especially of course when it's not my name on the code. Sometimes a patch will fix something in one place that another patch already fixed somewhere else, undoing the fix. Sometimes (as was my concern for #6 in the tempo tracker) the code works by accident and a fix breaks it, etc. So while acknowledging that these patches look sensible and are "probably right", it's sadly not just a case of hitting the merge button.

I would like to get this dealt with though... Do you still happen to have test cases for the key detector lying around that you can use to check that this PR (when applied against the current head, rather than the code it was originally written against) improves matters?

[JoergAtGithub]

That said, I am fairly sure that nobody else is about to make an official release either, and I can of course commit to this repo, so I could create a not-quite-an-official-release tag here if that would be good enough for your purposes.

Merging these commits would be a first step, but the problem is, that all the package managers (Linux-Distributions, VCPKG, Homebrew, etc.) ship always the last official release. And until then, a project that uses qm-dsp needs to use a vendored local copy with these bug-fixes.