BUG: Fix annotation rejection idx

[richardkoehler]

Reference issue (if any)

Addresses #12893

What does this implement/fix?

As discussed in the issue above, I implemented a fix that implements the last sample of "bad" annotations in raw.get_data being rejected now as well.
However, I noticed that some other functions, for example filter, have a similar behavior as get_data before, where the last sample of the "bad" annotation is not rejected. This means that for consistent behavior, we should also adapt these functions accordingly, right?
I'm happy about any other feedback and comments.

Changes

• Keep bad annotations that end at the start sample (annotations that are potentially one sample long): end = start
• Do not discard bad annotations that are exactly one sample long: onset = end
• Also discard last sample of bad annotation: end - start + 1

[mscheltienne]

Thanks for the PR!

However, I noticed that some other functions, for example filter, have a similar behavior as get_data before, where the last sample of the "bad" annotation is not rejected. This means that for consistent behavior, we should also adapt these functions accordingly, right?

Indeed, but that's where this private helper function comes in:

onsets, ends = _annotations_starts_stops(self, ["BAD"])

I suspect you will find _annotations_starts_stops used throughout the entire code base to select the samples to retain or not. You should modify this function to correctly include the last sample instead of modifying each individual function which uses _annotations_starts_stops to find the ends indexes.

[richardkoehler]

Thank you for the helpful comment!

So, I have been thinking about the function _annotations_starts_stops a little bit. I think we just need to get the desired behavior straight first, and then I am happy to implement everything. I realized that some people (also contributors/maintainers) may have a different mental model of what the start and stop indices means. Per se, the function _annotations_starts_stops does what it is supposed to: If I have a Raw object with a sampling rate of 10 Hz and have a "Bad" annotation from 0 to 10 s, it returns onsets = [0] and end = [100]:

sfreq = 10
info = mne.create_info(1, sfreq, "eeg")
raw = mne.io.RawArray(np.random.RandomState(0).randn(1, 30 * sfreq), info)
annotations = Annotations([0], [10], ['BAD'], raw.info['meas_date'])
raw.set_annotations(annotations)
starts, stops = _annotations_starts_stops(raw, 'BAD')
print(starts, stops)

100 is in fact the last sample of the bad segment, so if I do raw.get_data()[..., ends], I can get the last sample of the annotation. However, if I do slicing, the last sample is not included (as per convention in python): raw.get_data()[..., [onsets[0], ends[0].
Thus how the start and stop indices are used depends on what the user intends to do.
In brief, I had implemented the change in _annotations_starts_stops, and some of the tests break, for example as observed in the function test_annotation_filtering. With the changes, filtering Raw objects that have been concatenated does not work as intended anymore - the "EDGE" annotation (a single sample at the start of each concatenated segment) that was added during concatenation is not filtered at all anymore.

The question is if we still want to go ahead with the change in _annotations_starts_stops, or make the individual changes to the functions where the change is desired, like in get_data(). Looking forward to hearing your opinion.