Fix IrregularlySampledSignal.time_slice returning the whole signal for an empty window - #1899
Open
Yi-111-a wants to merge 1 commit into
Open
Fix IrregularlySampledSignal.time_slice returning the whole signal for an empty window#1899Yi-111-a wants to merge 1 commit into
Yi-111-a wants to merge 1 commit into
Conversation
`IrregularlySampledSignal.time_slice` builds a correct boolean mask but recovers the integer slice bounds by walking it with `id_start`/`id_stop` initialised to `None`. Those `None`s serve two purposes at once: a "no match found yet" flag inside the loop, and an unbounded slice bound afterwards. When no sample falls inside the window both stay `None`, so `self[None:None]` evaluates to `self[:]` and the entire signal is returned. Return an empty signal in that case. That matches `Event.time_slice` and `Epoch.time_slice`, and it is already a state this class supports: the constructor accepts an empty signal, and `test_time_slice_empty` asserts that slicing one returns an empty one. Windows that do contain samples are unaffected. Add a regression test covering a window inside a gap between samples, a window entirely after the last sample, and a window entirely before the first one. Fixes NeuralEnsemble#1888
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.
Fixes #1888.
The bug
IrregularlySampledSignal.time_slicecomputes a correct boolean mask, but thenrecovers the integer slice bounds by walking it:
Nonecarries two meanings here: "no match found yet" inside the loop, and"unbounded" in the slice afterwards. When no sample falls inside the window both
stay
None,self[None:None]evaluates toself[:], and the entire signal isreturned.
Segment.time_sliceappends the result unconditionally, so a segment sliced to awindow in which one irregularly sampled channel has no samples comes back holding
that channel's whole recording — timestamped outside the window that was asked
for — while its events and epochs for the same window are correctly dropped.
Reproducer against current
masterThe fix
Return an empty signal when nothing matched:
I chose "empty" over raising
ValueErrordeliberately — as the issue says, thesibling classes split on this and it is a real design question. I went with empty
because:
Event.time_sliceandEpoch.time_slicereturn empty for the same situation;IrregularlySampledSignalis already a state this class supports. Theconstructor accepts one, and
test_time_slice_emptyalready asserts thatslicing one returns an empty one. The current code satisfies that test only by
accident, because its input is empty to begin with;
AnalogSignal.time_sliceraises, andSegment.time_slicepropagates that, soraising here would widen what can fail inside a segment slice. Returning empty
changes behaviour only in the case that is currently wrong.
If you would rather raise here, I am happy to rework it — the mask is already
correct, so it is a one-line change either way.
I left
Segment.time_slicealone. It still appends the now-empty signalunconditionally; whether a channel with no samples should be dropped like an empty
Eventseems worth deciding separately, and it is not needed to fix the reportedbehaviour.
Verification
pytest neo/test/coretest/test_irregularysampledsignal.py— 70 passed, 1 skippedpytest neo/test/coretest/— 621 passed, 11 skippedtest_time_slice_window_without_samplesfails onmasterwithAssertionError: [ 1. 2. 30. 40.] s != [] sand passes with the fix.time_slice(None, None),time_slice(3, None),time_slice(None, 3), and the existingtest_time_slice_out_of_boundriesall still pass.