Skip to content

Preserve sample timestamps when slicing AnalogSignal - #1919

Open
antonsoo wants to merge 4 commits into
NeuralEnsemble:masterfrom
antonsoo:fix/analog-slice-timestamps
Open

antonsoo wants to merge 4 commits into
NeuralEnsemble:masterfrom
antonsoo:fix/analog-slice-timestamps

Conversation

@antonsoo

@antonsoo antonsoo commented Oct 18, 2026 •

Copy link
Copy Markdown

signal[::2] currently keeps the original sampling period, and negative slice starts are treated as literal time offsets. For example, a ten-sample signal starting at 2 s at 4 Hz reports signal[-3:].t_start as 1.25 s instead of 3.75 s.

Normalize the start against the signal length and adjust the sampling rate by direct division for both bare and tuple slices. Add regressions for negative/clipped starts, forward/reverse strides, nested slices and exact sampling-rate preservation. The separate author-list commit follows the first-contribution instructions.

Validation: 634 core tests and 51 subtests pass; Black passes on the changed Python files. On recorded EEGMMIDB S001/R04 data, all 30 slicing configurations match absolute sample-index timestamps after the fix, correcting 18 configurations and 227 event-window selections. All sample values and event-window selections remain correct. Direct rate division avoids the reciprocal-round-trip drift reported in review. This changes __getitem__; the time_slice change in #1905 remains separate.

Normalize slice starts against the signal length and apply the stride to the sampling period in both bare and tuple indexing. Negative or clipped starts then refer to the samples actually selected, and a bare stride has the same timing as its tuple equivalent.
@zm711

zm711 commented Oct 18, 2026

Copy link
Copy Markdown
Contributor

I have requested @apdavison to help review this.

@apdavison apdavison left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, many thanks for catching and fixing this.

I have one suggested improvement - see the inline comments.

Comment thread neo/core/analogsignal.py Outdated
Comment thread neo/core/analogsignal.py Outdated
@antonsoo

Copy link
Copy Markdown
Author

Changed both slice paths to set sampling_rate = self.sampling_rate / step. The new exact-rate checks cover Hz/kHz units, forward/reverse strides and all three indexing forms. The core suite passes 634 tests and 51 subtests; all 30 recorded EEG slicing cases still match the independent sample-index timestamps and event windows.

@antonsoo
antonsoo force-pushed the fix/analog-slice-timestamps branch from 68e405f to 5cb7efd Compare October 26, 2026 23:21

This branch has not been deployed

No deployments
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 this pull request may close these issues.

3 participants