Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong implementation of the 'DerivativeSFX' algorithm #1372

Open
andresbrocco opened this issue Sep 8, 2023 · 1 comment
Open

Wrong implementation of the 'DerivativeSFX' algorithm #1372

andresbrocco opened this issue Sep 8, 2023 · 1 comment

Comments

@andresbrocco
Copy link

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.

@andresbrocco
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants