Skip to content

IrregularlySampledSignal.time_slice returns the whole signal when the window contains no samples #1888

Description

@Arthur031221

Describe the bug

IrregularlySampledSignal.time_slice() returns the whole signal when the
requested window contains no samples, and Segment.time_slice() then carries
that whole signal into the sliced segment.

time_slice builds a correct boolean mask, then walks it to recover integer
slice bounds. When nothing matches, id_start and id_stop are both left at
None, and self[None:None] is self[:]. The None sentinel means "not found
yet" inside the loop and "unbounded" in the slice, and the two meanings collide
in exactly the case where no sample matched.

To Reproduce

import numpy as np
import quantities as pq
from neo.core import IrregularlySampledSignal, Event, Epoch, Segment

times = np.array([1.0, 2.0, 3.0, 4.0, 5.0]) * pq.s
sig = IrregularlySampledSignal(times, np.arange(5.0).reshape(-1, 1) * pq.mV)

print(sig.time_slice(10 * pq.s, 20 * pq.s).times)                       # all five
print(Event(times=times).time_slice(10 * pq.s, 20 * pq.s).times)        # empty
print(Epoch(times=times, durations=np.ones(5) * pq.s)
      .time_slice(10 * pq.s, 20 * pq.s).times)                          # empty

seg = Segment()
seg.irregularlysampledsignals.append(sig)
seg.events.append(Event(times=times))
sliced = seg.time_slice(10 * pq.s, 20 * pq.s)
print(len(sliced.events))                                               # 0
print(sliced.irregularlysampledsignals[0].times)                        # all five

A window entirely before the data, time_slice(-20 * pq.s, -10 * pq.s), does
the same. So does a window falling in a gap: samples at [1, 2, 30, 40] s sliced
to [5, 20] s returns all four.

The segment case is the one that costs something. Segment.time_slice appends
the sliced signal unconditionally (segment.py:269) while events and epochs are
guarded by if len(...): (lines 290 and 302). A segment sliced to a window in
which an irregularly sampled channel has no samples comes back holding that
channel's entire recording, timestamped outside the window that was asked for,
with no error. The segment's own events for that window are dropped correctly,
so the result is internally inconsistent. A slow channel that stopped early, a
pupil trace or a temperature probe, is exactly the shape this happens to.

Expected behaviour

Either an empty signal or a ValueError, and I don't think it is my call which.

The four sibling classes split evenly on the surface but not by accident:

has t_start / t_stop window with no samples
AnalogSignal yes raises ValueError
SpikeTrain yes raises ValueError
Event no returns empty
Epoch no returns empty
IrregularlySampledSignal yes (lines 325, 334) returns everything

By that reading it belongs with the classes that raise, and #831 is the same
question already answered: spiketrain.py:786 carries the comment "the
alternative to raising an exception would be to return a zero-duration spike
train", and 3128225 added it as the fix for #831. The reporter there wanted
this for the same reason I ran into it: "not all spiketrains 'exist' at all
times ... the slicing function as it is now assumes silence. It can also be
especially useful for slicing whole segments as well".

Two things make me hesitate to just copy that guard. SpikeTrain's test is
if _t_start > self.t_stop or _t_stop < self.t_start, and
IrregularlySampledSignal.t_start is self.times[0], which raises IndexError
on an already-empty signal, so it needs a length check first or
test_time_slice_empty stops working. And a ValueError from
Segment.time_slice is a bigger change than it looks, since a segment can hold
many signals and only one of them needs to miss the window.

Returning empty, matching Event and Epoch, is a three-line change and is
what I would write if the choice were mine. I would rather ask than guess,
given #831.

Environment:

  • OS: Linux 6.17, Ubuntu
  • Python version: 3.12.3
  • Neo version: 0.15.dev0 at 35cbce7
  • NumPy version: 2.5.1

Additional context

fc4f4ba7 (2017-03-08) added time_slice to Epoch, Event and
IrregularlySampledSignal in one commit. The first two index with the mask
directly, deepcopy(self[indices]), and come back empty. Only the third
converts the mask into None-initialised integer bounds. Mask indexing isn't
available here as a drop-in, since
IrregularlySampledSignal.__getitem__ raises TypeError on a boolean array.

The existing test_time_slice_empty cannot catch this. It builds its signal
from [] * pq.mV and [] * pq.ms, so the signal is already empty before
time_slice is called, and self[None:None] on an empty signal is
indistinguishable from a correct empty result. test_time_slice_out_of_boundries
uses t_start=0, t_stop=2500000, a window that contains every sample.

Two other things I noticed while testing and did not want to fold in silently.
Inverted windows, t_start > t_stop, currently return the whole signal too, and
AnalogSignal and SpikeTrain both raise on those. And the empty result raises
IndexError on .t_start, .t_stop and .duration, which is pre-existing for
any empty IrregularlySampledSignal but would become reachable through
time_slice for the first time.

I have the return-empty version written with regression tests, covering a window
after the data, one before it, one inside a gap, agreement with the Event and
Epoch siblings, and the segment case. Against 35cbce7 they fail before and
pass after, and neo/test/coretest goes from 619 passed 12 skipped 0 failed to
623 passed 12 skipped 0 failed, with no existing test disturbed. Say which
behaviour you want and I'll open the PR, including the raise version if that's
the one.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions