Skip to content

Cut the lag axis together with the values in cross_correlation_function - #707

Open
adityasingh2400 wants to merge 1 commit into
NeuralEnsemble:masterfrom
adityasingh2400:fix/cross-correlation-n-lags-time-axis
Open

Cut the lag axis together with the values in cross_correlation_function#707
adityasingh2400 wants to merge 1 commit into
NeuralEnsemble:masterfrom
adityasingh2400:fix/cross-correlation-n-lags-time-axis

Conversation

@adityasingh2400

Copy link
Copy Markdown

When n_lags is given, cross_correlation_function returns correct correlation values attached to a wrong lag axis.

This is the plotting example from the function's own docstring:

import neo
import numpy as np
import quantities as pq
from elephant.signal_processing import cross_correlation_function

dt, N, f = 0.02, 2018, 0.5
t = np.arange(N) * dt
x = np.zeros((N, 2))
x[:, 0] = 0.2 * np.sin(2. * np.pi * f * t)
x[:, 1] = 5.3 * np.cos(2. * np.pi * f * t)
signal = neo.AnalogSignal(x, units='mV', t_start=0. * pq.ms,
                          sampling_rate=1 / dt * pq.Hz, dtype=float)

rho = cross_correlation_function(signal, [0, 1], n_lags=150)
print(rho.t_start.rescale('s'), rho.times[-1].rescale('s'))
print(rho.times[rho.shape[0] // 2].rescale('s'))

On current master:

-20.18 s -14.18 s
-17.18 s

The returned lag range is [-20.18 s, -14.18 s]. It does not contain zero lag at all, and the sample that actually is zero lag is labelled -17.18 s. The expected range for n_lags=150 at dt=0.02 s is [-3.0 s, 3.0 s] with zero in the centre.

The cause is that only the values are sliced:

    if n_lags is not None:
        tau0 = np.argwhere(tau == 0).item()
        xcorr = xcorr[tau0 - n_lags: tau0 + n_lags + 1, :]

    cross_corr = neo.AnalogSignal(xcorr,
                                  units='',
                                  t_start=tau[0] * signal.sampling_period,

tau keeps the full range, so t_start is taken from the uncut vector and the whole axis is shifted by nt // 2 - n_lags samples, which is 859 samples or 17.18 s here. The magnitudes are right, I verified they match the corresponding slice of the un-cut result exactly, so this shows up as a silently mislabelled plot rather than a crash. The docstring's own example does plt.plot(rho.times, rho) with n_lags=150. The fix slices tau with the same indices.

The same expression also wrapped silently when n_lags exceeded the available lag range, because tau0 - n_lags went negative. For a 100 sample signal, n_lags=50 returned 100 rows and n_lags=60 returned 10 rows, both contradicting the documented output shape [2*n_lags+1, n]. That now raises ValueError naming the largest usable value, and the Raises section gains the matching bullet.

Two tests are added. test_cross_correlation_nlags_time_axis checks that the lag axis runs from -n_lags to +n_lags with zero in the centre, and applies the same sine of the lag identity that test_cross_correlation_freqs already uses for the un-cut case, which only holds if the axis matches the values. test_cross_correlation_nlags_too_large pins the boundary at 49 for 100 samples and the rejection above it.

Reverting signal_processing.py to master fails with Mismatched elements: 61 / 61 (100%), [0]: -20.18 (ACTUAL), -0.6 (DESIRED). After, 52 passed and 2 subtests passed. signal_processing.py and its test module are pycodestyle clean before and after.

I have not added myself to doc/authors.rst, since it is a numbered institutional affiliation list. Happy to add an entry if you would like one.

Disclosure: this change was prepared with AI assistance. I have reviewed and tested it.

When 'n_lags' was given, only the correlation values were sliced. The
lag vector 'tau' kept the full range, and 't_start' of the returned
neo.AnalogSignal was taken from the uncut 'tau', so the whole lag axis
was shifted by nt // 2 - n_lags samples. For the example in the
function's own docstring (2018 samples, dt = 0.02 s, n_lags = 150) the
returned lags ran from -20.18 s to -14.18 s, zero lag was labelled
-17.18 s, and the axis did not contain zero at all. The values were
correct, only the times they were attached to were wrong, which makes
every plot of 'rho.times' against 'rho' mislabelled.

Slice 'tau' with the same indices so 't_start' follows the cut.

The same slice also silently wrapped when 'n_lags' exceeded the
available lag range, because 'tau0 - n_lags' went negative. A signal of
100 samples returned 100 rows for n_lags=50 and 10 rows for n_lags=60,
both contradicting the documented shape of 2 * n_lags + 1. That case
now raises ValueError with the largest usable value.
@adityasingh2400

Copy link
Copy Markdown
Author

The docs jobs here are failing on a remote fetch, not on this branch.

The sphinx build dies executing a notebook that pulls sample data over HTTPS, ending in HTTPError: HTTP Error 403: Forbidden. That is the download being refused, so the same build should pass once the remote is serving normally again.

The other elephant PR I have open, #706, is red on the same theme from the other direction, with SSLCertVerificationError on the TestDownloadDatasets cases. Neither of these branches touches anything involved in fetching data.

@coveralls

Copy link
Copy Markdown
Collaborator

Coverage Status

coverage: 45.672% (-42.9%) from 88.605% — adityasingh2400:fix/cross-correlation-n-lags-time-axis into NeuralEnsemble:master

@adityasingh2400

Copy link
Copy Markdown
Author

Following up on my note above, which only covered the docs job. The pip, mpi and conda jobs have since gone red too, and they are the same story.

On the latest run here the only failures are test_datasets.py::TestDownloadDatasets::test_valid_data_with_integrity_check and test_valid_data_with_failed_integrity_check, with URLError: [Errno 110] Connection timed out, against 773 passed and 5 skipped. Across my three open PRs the same two tests have now failed with three different network errors in about an hour, a certificate verification failure, a 403, and now a timeout, which points at the data host rather than at any branch.

Worth flagging one knock-on effect: Coveralls reports coverage dropping to roughly 45.7 percent from 88.6. That looks alarming but is consistent with the failing jobs never uploading their coverage, leaving only a partial report merged. I would not read it as a real coverage regression from these changes.

@CozySocksAlways
CozySocksAlways self-requested a review August 5, 2026 16:54
@CozySocksAlways

Copy link
Copy Markdown
Contributor

Hi again,
It's the same story here with the unit tests. Will review soon :)

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

Labels

bugfix Fix for an indentified bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants