Skip to content

Fix AnalogSignal.time_slice(None, t_stop) for signals that do not start at 0 s - #1905

Open
Arthur031221 wants to merge 1 commit into
NeuralEnsemble:masterfrom
Arthur031221:fix/analogsignal-time-slice-none-t-start
Open

Arthur031221 wants to merge 1 commit into
NeuralEnsemble:masterfrom
Arthur031221:fix/analogsignal-time-slice-none-t-start

Conversation

@Arthur031221

Copy link
Copy Markdown

Problem

AnalogSignal.time_slice(None, t_stop) treats t_stop as a time measured from
0 s rather than from the signal's own start, so on a signal that does not start
at 0 s it returns the wrong window.

When t_start is None the start index is 0, which is right, because index 0
is the sample at self.t_start. But t_start is then replaced by 0 * pq.s
and used as the origin the stop index is measured from:

if t_start is None:
    i = 0
    t_start = 0 * pq.s
...
delta = (t_stop - t_start) * self.sampling_rate
j = i + int(np.rint(delta.simplified.magnitude))

if (i < 0) or (j > len(self)):
    raise ValueError(...)

j becomes the absolute index of t_stop counted from 0 s, where the rest of
the method wants an offset from the start of the signal, and the guard then
compares that absolute index against len(self). Twenty samples at 1 Hz, so a
signal 20 s long:

 t_start   t_stop   correct                  time_slice(None, t_stop) returns
   5 s      7 s     2 samples, 5 to 7 s       7 samples, 5 to 12 s
  -2 s      1 s     3 samples, -2 to 1 s      1 sample, -2 to -1 s
  -2 s     -1 s     1 sample, -2 to -1 s      19 samples, -2 to 17 s
  30 s     33 s     3 samples, 30 to 33 s     ValueError

Three of the four are silent. In row three j goes negative, so self[0:j]
counts back from the end of the signal. The ValueError in row four is not
about where the signal starts: it fires when rint(t_stop * rate) exceeds
len(self), so t_start = 5 s on this same 20 s signal also raises for
t_stop = 24 s, which is inside the signal.

At realistic sizes the silent rows are the ones that bite. A 30 kHz recording
starting at 0.5 s, asked for everything up to 10 s, returns 300000 samples
spanning 0.5 s to 10.5 s instead of 285000 spanning 0.5 s to 10.0 s.

No synthetic signal is needed. ExampleIO's second segment starts at 15 s, and
on master:

sig = ExampleIO('').read_block(lazy=True).segments[1].analogsignals[0].load()
sig.time_slice(None, 16 * pq.s)          # ValueError
sig.time_slice(sig.t_start, 16 * pq.s)   # 10000 samples, 15.0 s to 16.0 s

The None path was correct until 2019. f900b356 ("fix for #530") changed the
stop index from the absolute j = self.time_index(t_stop) to an offset from
t_start, which made the None branch raise, and 46c64546 seven minutes
later added t_start = 0 * pq.s to quiet it. With i = 0 the change below
reduces to j = self.time_index(t_stop), the expression that was there before.

That None means "do not cut this end" is stated in #842, which taught
Segment to accept it: "time_slice methods of data objects accept None as
t_start or t_stop when the corresponding samples should not be cut, e.g.
AnalogSignals", linking this branch. In #1259 JuliaSprenger puts the other half
of it directly: "t_start and t_stop are absolute times extracted from the
data file". Event, Epoch, SpikeTrain and IrregularlySampledSignal all
substitute -inf and +inf.

Solution

Use self.t_start as the origin in place of 0 * pq.s, and document None in
the docstring as the other four classes already do.

This moves the boundary in both directions. Both new results agree with what
passing the signal's own t_start already returns today:

t_start=5 s, 20 samples at 1 Hz, t_stop=24 s     ValueError -> 19 samples
t_start=-0.5 s, 2 s at 1 kHz, t_stop=1.6 s       1600 samples -> ValueError

Only a signal starting at exactly 0 s is unchanged for every t_stop. A
sub-sample offset is not exempt: at t_start = 0.001 s with t_stop = 4.5005 s
the count goes from 5 to 4, because rint(t_stop * rate) and
rint((t_stop - t_start) * rate) fall on opposite sides of a bin edge. In every
such case the new count is the one that matches the window asked for.

Testing

test__time_slice__no_explicit_start_measures_from_signal_start runs the four
rows above plus a sub-sample offset (0.4 ms at 1 kHz, t_stop 10.6 ms, 10
samples where master gives 11), checking the count, t_start, the values, and
agreement with the explicit call. Each of the five fails on master on its own.
test__time_slice__no_explicit_start_refuses_what_explicit_start_refuses pins
the second row of the table above: master returns 1600 samples where the
explicit call raises.

test__time_slice__no_explicit_time already covers (None, t_stop), but its
offset is 10 ms against a 1 Hz sampling rate, so the error it would see is a
hundredth of a sample and np.rint removes it.

pytest neo/test/coretest on Python 3.12.3 with numpy 2.5.3 and quantities
0.16.4 on Linux: 621 passed and 12 skipped, against 619 passed and 12 skipped on
master. With the fix reverted and the tests kept, 2 failed and 619 passed.
black --line-length 120 --check reports no changes.

Other comments

AnalogSignalProxy._time_slice_indices has the same shape and is left alone
here. It never substitutes anything for a None t_start, so the subtraction
at neo/io/proxyobjects.py:204 fails and load(time_slice=(None, t_stop))
raises for every proxy signal, including one starting at 0 s. That is a
separate fix and I am happy to send it if you would like it.

j < 0 is still unchecked, so a t_stop earlier than the signal returns a
slice counted from the end rather than raising. This change makes the None
path agree with the explicit path there; it does not add the missing check.

This does not change which bin a non-aligned t_start rounds to, so it is not
the behaviour discussed in #834.

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.

1 participant