Wrong implementation of the 'AutoCorrelation' algorithm

[andresbrocco]

I'm currently studying the library, and I've found out that the implementation of the 'AutoCorrelation' output is not consistent between its options:

If you use 'generalized=false', it calculates the autocorrelation correctly. If you use 'generalized=true', the overall magnitude of the autocorrelation will be scaled by 1/sizeFFT^k, which is extremely bizarre (its scale should not depend on the size of the FFT).

Check out the current source code:

  for (int i=0; i<int(_fftBuffer.size()); i++) {
    if (!_generalized){
      _fftBuffer[i] = complex<Real>(_fftBuffer[i].real() * _fftBuffer[i].real() +
                                    _fftBuffer[i].imag() * _fftBuffer[i].imag(), // Generalized, todo 0.5 -> c
                                    0.0); // squared amplitude -> complex part = 0
    } else {
      // Apply magnitude compression
      _fftBuffer[i] = complex<Real>(
        pow(sqrt(pow(_fftBuffer[i].real() / sizeFFT, 2) + pow(_fftBuffer[i].imag() / sizeFFT, 2)), _frequencyDomainCompression),
        0.0); // squared amplitude -> complex part = 0
    }
  }

[dbogdanov]

Indeed, this normalization behavior is redundant nor it is following the original paper that the algorithm cites (Tolonen & Karjalainen, 2000), and I cannot find a reason this normalization was added in the first place. A pull request with a fix is welcome.

PercivalBpmEstimator is the only Essentia algorithm using generalized auto-correlation, so we should double check that its behavior is not affected.

Also test_autocorrelation.py is missing a test for the generalized case.

[andresbrocco]

@dbogdanov I don't think this issue should be labeled "algorithms QA". Instead, it should be on "bug".