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
Open
Arthur031221 wants to merge 1 commit into
Arthur031221 wants to merge 1 commit into
Conversation
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
AnalogSignal.time_slice(None, t_stop)treatst_stopas a time measured from0 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_startisNonethe start index is 0, which is right, because index 0is the sample at
self.t_start. Butt_startis then replaced by0 * pq.sand used as the origin the stop index is measured from:
jbecomes the absolute index oft_stopcounted from 0 s, where the rest ofthe 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 asignal 20 s long:
Three of the four are silent. In row three
jgoes negative, soself[0:j]counts back from the end of the signal. The
ValueErrorin row four is notabout where the signal starts: it fires when
rint(t_stop * rate)exceedslen(self), sot_start = 5 son this same 20 s signal also raises fort_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, andon master:
The
Nonepath was correct until 2019.f900b356("fix for #530") changed thestop index from the absolute
j = self.time_index(t_stop)to an offset fromt_start, which made theNonebranch raise, and46c64546seven minuteslater added
t_start = 0 * pq.sto quiet it. Withi = 0the change belowreduces to
j = self.time_index(t_stop), the expression that was there before.That
Nonemeans "do not cut this end" is stated in #842, which taughtSegmentto accept it: "time_slicemethods of data objects accept None ast_startort_stopwhen the corresponding samples should not be cut, e.g.AnalogSignals", linking this branch. In #1259 JuliaSprenger puts the other half
of it directly: "
t_startandt_stopare absolute times extracted from thedata file".
Event,Epoch,SpikeTrainandIrregularlySampledSignalallsubstitute
-infand+inf.Solution
Use
self.t_startas the origin in place of0 * pq.s, and documentNoneinthe 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_startalready returns today:Only a signal starting at exactly 0 s is unchanged for every
t_stop. Asub-sample offset is not exempt: at
t_start = 0.001 switht_stop = 4.5005 sthe count goes from 5 to 4, because
rint(t_stop * rate)andrint((t_stop - t_start) * rate)fall on opposite sides of a bin edge. In everysuch case the new count is the one that matches the window asked for.
Testing
test__time_slice__no_explicit_start_measures_from_signal_startruns the fourrows above plus a sub-sample offset (0.4 ms at 1 kHz,
t_stop10.6 ms, 10samples where master gives 11), checking the count,
t_start, the values, andagreement with the explicit call. Each of the five fails on master on its own.
test__time_slice__no_explicit_start_refuses_what_explicit_start_refusespinsthe second row of the table above: master returns 1600 samples where the
explicit call raises.
test__time_slice__no_explicit_timealready covers(None, t_stop), but itsoffset is 10 ms against a 1 Hz sampling rate, so the error it would see is a
hundredth of a sample and
np.rintremoves it.pytest neo/test/coreteston Python 3.12.3 with numpy 2.5.3 and quantities0.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 --checkreports no changes.Other comments
AnalogSignalProxy._time_slice_indiceshas the same shape and is left alonehere. It never substitutes anything for a
Nonet_start, so the subtractionat
neo/io/proxyobjects.py:204fails andload(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 < 0is still unchecked, so at_stopearlier than the signal returns aslice counted from the end rather than raising. This change makes the
Nonepath agree with the explicit path there; it does not add the missing check.
This does not change which bin a non-aligned
t_startrounds to, so it is notthe behaviour discussed in #834.