Warn when Epochs events fall outside the raw data range (#12989)#14004
Warn when Epochs events fall outside the raw data range (#12989)#14004CedricConday wants to merge 12 commits into
Conversation
|
Thanks for opening this PR @CedricConday! I agree with the discussion in the issue that there should be an option to control whether a warning is emitted. That would also be in line with how we handle the similar situation for setting annotations in Lines 714 to 719 in 90b8cee |
Mirrors the on_missing pattern used for out-of-range Raw annotations (_on_missing helper): 'raise' | 'warn' (default) | 'ignore'. Addresses maintainer review on mne-toolsgh-14004.
|
Thanks @tsbinns! Added an |
tsbinns
left a comment
There was a problem hiding this comment.
Thanks for adding the extra control @CedricConday!
Just a few comments.
| # Warn when an event's *sample number* falls outside the recorded data | ||
| # (before ``first_samp`` or at/after the last sample). Such an event yields | ||
| # no epoch and is silently dropped, which is surprising and usually means | ||
| # the events were created at a different sampling frequency or precede | ||
| # ``first_samp``. This checks the event positions, not the epoch window — | ||
| # an in-range event whose window merely clips the edge is normal and quiet. | ||
| # See gh-12989. | ||
| if self._raw is not None and len(self.events) > 0: | ||
| lo = self._raw.first_samp | ||
| hi = lo + self._raw.n_times | ||
| n_oob = int(((self.events[:, 0] < lo) | (self.events[:, 0] >= hi)).sum()) | ||
| if n_oob: | ||
| _on_missing( | ||
| on_outside, | ||
| f"{n_oob} event{_pl(n_oob)} {'has' if n_oob == 1 else 'have'} a " | ||
| "sample number outside the recorded data; the corresponding " | ||
| f"epoch{_pl(n_oob)} will be dropped. This can happen if the " | ||
| "events were created at a different sampling frequency, or " | ||
| "contain sample numbers before first_samp.", | ||
| name="on_outside", | ||
| ) | ||
|
|
There was a problem hiding this comment.
I'd suggest including this within BaseEpochs, as we'd want this warning for both the Epochs and EpochsArray classes.
Also, might be worth only checking for this after selection has been applied in BaseEpochs.
| event_repeated="error", | ||
| on_outside="warn", | ||
| verbose=None, |
There was a problem hiding this comment.
| event_repeated="error", | |
| on_outside="warn", | |
| verbose=None, | |
| event_repeated="error", | |
| *, | |
| on_outside="warn", | |
| verbose=None, |
Should make sure this new param is kwarg only.
There was a problem hiding this comment.
Likewise, when the param is added to EpochsArray, it should be as kwarg-only (like some others there).
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("error") | ||
| mne.Epochs(raw, oob, tmin=-0.2, tmax=0.5, baseline=None, on_outside="ignore") | ||
| # an in-range event whose epoch window merely clips the edge must NOT warn | ||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("error") | ||
| mne.Epochs(raw, np.array([[990, 0, 1]]), tmin=0, tmax=0.5, baseline=None) |
There was a problem hiding this comment.
Warnings count as errors in our tests, so we don't need to catch them.
|
@CedricConday Please also link your GitHub account to CircleCI so that the documentation build will run, and we can verify that looks good. |
Events whose epoch window lies (partly) outside the raw data are silently dropped, which is surprising e.g. when events come from a different sampling frequency or contain samples before first_samp. Warn at construction listing how many events are out of bounds. Adds a regression test.
Warn only when an event's sample number falls outside the recorded data ([first_samp, first_samp + n_times)) — the issue's actual scenario (events at a different sampling frequency, or before first_samp). The earlier check on the epoch *window* also fired for in-range events whose window merely clips the edge (wide tmax / negative tmin on short data), which is normal and broke several existing tests. Full test_epochs.py is green under -W error.
for more information, see https://pre-commit.ci
Mirrors the on_missing pattern used for out-of-range Raw annotations (_on_missing helper): 'raise' | 'warn' (default) | 'ignore'. Addresses maintainer review on mne-toolsgh-14004.
…x test - on_outside made keyword-only in both Epochs and EpochsArray - Warning logic moved from Epochs to BaseEpochs._oob_check for reuse - EpochsArray now accepts and forwards on_outside to BaseEpochs - Test uses pytest.raises instead of pytest.warns per mne warnings-as-errors policy
…-as-errors, add missing BaseEpochs on_outside doc
…event file (mne-toolsgh-12989) test_warnings and test_n_components_none pass the complete event file against a shorter/cropped raw, so some events fall past the data and are dropped — expected here. Pass on_outside='ignore' at those call sites so the new default warning does not trip warnings-as-errors. No behavior change to the tests' actual assertions.
ed3a816 to
11f6a3b
Compare
for more information, see https://pre-commit.ci
|
Thanks for the review @tsbinns — I've addressed all of it: |
Fixes #12989
What was wrong
When
mne.Epochsis created from aneventsarray containing sample numbers outside the data, the corresponding epochs are silently dropped with no indication that the events were out of bounds. This is easy to hit wheneventswas generated at a different sampling frequency, or contains samples beforefirst_samp.Fix
At construction, compute each epoch's sample window (the same
start/stopmath used by_get_epoch_from_raw) and warn if any fall before the start or past the end of the raw data, reporting how many.Test
Adds
test_epochs_warn_out_of_bounds_events: an event past the end of the data now raises aRuntimeWarning. Verified it does not warn onmain(the silent-drop behavior) and warns with this change.AI-assisted, human-reviewed — I'm an AI engineer; I find, fix, and test with AI (Claude Code), then review and verify before opening.