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

[Bug] signal_processing.cross_correlation_function() scaling #265

Closed
jmbudd opened this issue Oct 30, 2019 · 7 comments · Fixed by #277
Closed

[Bug] signal_processing.cross_correlation_function() scaling #265

jmbudd opened this issue Oct 30, 2019 · 7 comments · Fixed by #277

Comments

@jmbudd
Copy link

jmbudd commented Oct 30, 2019

Potential Issue
The scaling of the cross correlation coefficient may not be correct at least for single pairs of AnalogSignals.

Reproduce
See notebook: https://github.com/jmbudd/misc/blob/master/ElephantCrossCorr.ipynb
(pdf of notebook: https://github.com/jmbudd/misc/blob/master/ElephantCrossCorr.pdf)

Expected Behaviour
The output from elephant code should match numpy/matlab version such that there is a stronger decay with time lag from peak correlation location.

Environment
CPython 3.5.2
IPython 7.8.0
numpy 1.17.3
scipy 1.3.1
neo 0.7.2
elephant 0.6.3
compiler : GCC 5.4.0 20160609
system : Linux
release : 4.4.0-165-generic
machine : x86_64
processor : x86_64
CPU cores : 8
interpreter: 64bit

@dizcza
Copy link
Member

dizcza commented Nov 4, 2019

Thank you for pointing this out! True, these functions should behave as numpy/scipy equivalent with the only difference that the input is AnalogSignal.
We're currently at Elephant workshop that will end on Wed, then I'll take a closer look.

@dizcza
Copy link
Member

dizcza commented Nov 11, 2019

First of all, thank you for the reproducible notebook with the explanations. Seems your suggestion is right, though first I need to understand why the scaling was put in the first place. Maybe, the author of this function wanted to deal with the decaying tails on purpose, I'm not sure. And so I need to look up for the equation in the book the function references to.
I'll let you know once I get it through.

@jmbudd
Copy link
Author

jmbudd commented Nov 11, 2019

Thanks, Danylo. I think this is a right approach although I could not find the book cited.

@dizcza
Copy link
Member

dizcza commented Nov 13, 2019

I cannot find the book either. But the comment at line 255

Correct for bias due to zero-padding

explains the behavior sheds light on the issue. Maybe, it'd make sense to add a corresponding argument parameter unbiased=True like we do in cross_correlation_histogram function.

The example you linked in the notebook does not mention bias correction at all. And I'm not sure what the behavior is expected by default in numpy/matlab equivalent - with or without bias correction?

@mdenker
Copy link
Member

mdenker commented Nov 18, 2019

Link to Matlab scaling options: https://de.mathworks.com/help/matlab/ref/xcorr.html#d117e1608028

@jmbudd
Copy link
Author

jmbudd commented Nov 18, 2019

Thanks, Danylo and Michael, this explains the difference - abs(tau) term provides for an unbiased estimate, which numpy function does not. I agree adding a corresponding argument would make this clear and be more consistent with cross_correlation_histogram.

@dizcza
Copy link
Member

dizcza commented Dec 4, 2019

Hello @jmbudd
I added the flag scaleopt in the function. If you pass scaleopt='coeff', it works the same as your numpy example. Feel free to check.

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

Successfully merging a pull request may close this issue.

3 participants