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

signal.fftconvolve return a tuple including lag information #11020

Closed
benjaminr opened this issue Nov 5, 2019 · 4 comments
Closed

signal.fftconvolve return a tuple including lag information #11020

benjaminr opened this issue Nov 5, 2019 · 4 comments
Milestone

Comments

@benjaminr
Copy link
Contributor

I'd be surprised if this hasn't been raised before, but I think it'd be useful to provide amethod for returning lag information alongside the FFT convolution results.

For people new to cross-correlation and the FFT convolution method, it adds an extra step of complexity that can be frustrating.

I suggest passing back a numpy array with the lag information alongside the existing output in a tuple.

@e-q
Copy link
Contributor

e-q commented Nov 7, 2019

Strictly speaking, the indices of a convolution result aren't lags, so the interpretation of the indices change whether you're interpreting the result as a convolution or a cross-correlation (having time-reversed and complex-conjugated one of the input signals).

Can you be a little more specific about the confusion/complexity that you encountered?

FWIW, the example in the fftconvolve docs shows an application of the function to perform a cross-correlation, and also that the lags are given by np.arange(-len(sig)+1,len(sig)) in the case of mode='full'.

@benjaminr
Copy link
Contributor Author

benjaminr commented Nov 7, 2019

Strictly speaking, the indices of a convolution result aren't lags, so the interpretation of the indices change whether you're interpreting the result as a convolution or a cross-correlation (having time-reversed and complex-conjugated one of the input signals).

Sure, I recognise that; I think I was just making a comparison with Matlab's xcorr which does what I'm describing.

Can you be a little more specific about the confusion/complexity that you encountered?

FWIW, the example in the fftconvolve docs shows an application of the function to perform a cross-correlation, and also that the lags are given by np.arange(-len(sig)+1,len(sig)) in the case of mode='full'.

The docs example is good, but in the case of cross-correlation, getting the delay / lag out is more involved. See below for an example - it's as you mention above.

# Ahead example
>>> x = np.random.random(1000)
>>> x_ahead = x[100:]
>>> corr = signal.fftconvolve(x, x_ahead[::-1], 'full')
>>> lag = np.argmax(corr)
>>> lag
999
# Here the lag is fairly meaningless without an additional calculation
>>> lags = np.arange(-x_ahead.size + 1, x.size)
>>> lags[lag]
100
# Behind example
>>> x = np.random.random(1000)
>>> x_behind = np.pad(x, (100,0), 'constant')
>>> corr = signal.fftconvolve(x, x_behind[::-1], 'full')
>>> lag = np.argmax(corr)
>>> lag
999
>>> lags = np.arange(-x_behind.size + 1, x.size)
>>> lags[lag]
-100

Perhaps a solution is a separate xcorr style function that wraps fftconvolve, returning a tuple of the correlation output alongside the lags as I have above.

If you're trying to understand delay in time series data, I think this makes more sense in its own function where the extra range calculation is abstracted. It could be I'm just slow to figure this stuff out and probably should read the docs more carefully, but I'm sure others hit the same thing.

@e-q
Copy link
Contributor

e-q commented Nov 8, 2019

A correlation function exists in signal.correlate At the very least, the docs for that function could have some additional notes or an example for computing the lag vector. It looks like matplotlib.pyplot.xcorr actually returns the lag vector, so there is some precedent.

Searching around also reminded me of stalled efforts to add a maxlags argument to convolution and correlation functions in SciPy and NumPy, which would also perhaps motivate returning such a vector.

Would you be interested in putting together a PR to add a keyword argument to toggle returning a tuple of the lags along with the correlation result? The usual thing to do when proposing a feature is to send something out to the Scipy-dev mailing list to get people's opinions. It can be useful to already have a PR started to point to as well.

@benjaminr
Copy link
Contributor Author

Sure happy to have a look at this.

benjaminr added a commit to benjaminr/scipy that referenced this issue Nov 16, 2019
Addresses issue scipy#11020 by providing a way to provide an output
specific to cross-correlation, containing both correlation values
and the lags used to determine time offset.
benjaminr added a commit to benjaminr/scipy that referenced this issue Nov 16, 2019
Addresses issue scipy#11020 by providing a way to provide an output
specific to cross-correlation, containing both correlation values
and the lags used to determine timing offset.
benjaminr added a commit to benjaminr/scipy that referenced this issue May 25, 2020
Addresses issue scipy#11020 by providing a way to provide an output
specific to cross-correlation, containing both correlation values
and the lags used to determine timing offset.
larsoner added a commit that referenced this issue Jun 16, 2020
* ENH: Adds xcorr lags return to signal.fftconvolve

Addresses issue #11020 by providing a way to provide an output
specific to cross-correlation, containing both correlation values
and the lags used to determine timing offset.

* ENH: Adds return_lags kwarg to signal.correlate

Replaces suggested use of xcorr kwarg in fftconvolve with
return_lags in signal.correlate. This helps to ensure a valid
cross-correlation context to its usage.

Implementation infers use case and passes mode 'full' and method
'fft' to correlate call.

* Fixes accidental doc change.

* Fix to docstring multi-line statement.

* Ensures return_lags compatability for xcorr

Adds compatibility for returning lags from 'valid', 'same' and
'full' modes in signal.correlate.

Adds test for return_lags kwarg in signal.correlate.

* Fixed flake8 issue in test and removed old part of docstring.

* Updates test function for return_lags in correlate

Removes uneccessary parameterization of return_lags.
Adds seeded random number generation for test arrays.
Fixes PEP8 regression, adding two blank lines between funcs.

* Swapped RandomState call random() to rand().

* Swapped RandomState call to normal_standard for commonality with np.random.Generator.

* Fixes flake8 issue with two blank lines.

* Moves correlation lags functionality to own func

* Updates correlation_lags and docs

correlation_lags now takes arraay sizes as opposed to the arrays
themselves.

The docstring now cross references correlate and has an example
and notes section.

Updated __init__.py to include correlation_lags.

Updated test_signaltools to reflect changes.

* Adds signal import to doctest.

* Update scipy/signal/signaltools.py

Co-authored-by: peterbell10 <peterbell10@live.co.uk>

* Update scipy/signal/signaltools.py

Co-authored-by: peterbell10 <peterbell10@live.co.uk>

* Adds correlation_lags to correlate see also.

* Trigger CI

* Fixes newlines and tests for lags and correlation shape.

* Update scipy/signal/signaltools.py

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

* Adds odd length to correlation_lags testing.

Co-authored-by: peterbell10 <peterbell10@live.co.uk>
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@tylerjereddy tylerjereddy added this to the 1.6.0 milestone Jun 17, 2020
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

4 participants