-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
Calculation in katz_fd seems to be incorrect #34
Comments
Hi @Khaktos, Thank you for the deep dive and catching this issue ! My opinion is that if the current implementation is incorrect, then we should just go ahead and release a new version of antropy with the correction. Users can decide to downgrade (or not upgrade) their version if they really want to maintain to the old behavior. If that sounds ok, could you please submit a PR, ideally including unit tests against the Matlab implementation (assuming that it is correct)? Thanks again, |
The current implementation of katz_fd in antropy/fractal.py looks like the following:
However, both in the documentation and in the original paper the distance (
dist
andd
) is defined as the Euclidean distance between the given points, which is calculated differently.A BASIC implementation was also provided in the Appendix of the original paper:
The important differences are in lines 130 and 150. (Note: the exponential operator
^
probably missing due to scanning/printing issues)With this and by looking at another implementation in MATLAB, I think the above code for katz_fd should be changed to represent the original definition. However, I do not know if this change would break existing works using the current (apparently incorrect) functionality, so I propose the following change:
dists
andd
calculations to something like the following (Note: this was only tested with single channel data)If there are any comments or suggestions, please let me know.
(Also this is my first contribution to an open source project, so I am a bit unsure about the etiquette/conventions)
The text was updated successfully, but these errors were encountered: