essentia
essentia copied to clipboard
Wrong implementation of the 'DerivativeSFX' algorithm
I'm currently studying the library, and I've found out that the implementation of the 'derAvAfterMax' output is not what it says!
As per the documentation: "the weighted average of the derivative after the maximum amplitude".
But I've read the source code and it seems to be wrongly implemented. It is currently accumulating the nominator and the denominator and then dividing them at the end. This is wrong because the denominator should be different for each time instant. The correct implementation would be to divide the nominator by the denominator at each time instant and then take the average of the resulting vector.
The current implementation gives us the following result:
$$ \frac{x_N-x_{imax-1}}{\sum_{i=imax}^{N} x_i} \qquad [eq.1] $$
While the description at the documentation says it would give us:
$$ \frac{1}{N-imax} \sum_{i=imax}^{N} \frac{x_i-x_{i-1}}{x_i} \qquad [eq.2] $$
What makes me conclude that the implementation is wrong is the fact that if the eq.1 is indeed the desired result, the implementation is not efficient, because it is taking the derivative at each time instant, which is not necessary: the nominator becomes only the difference between the last and the 'max-1' time instant, afterall.
@dbogdanov I don't think this issue should be labeled "algorithms QA". Instead, it should be on "bug"