ENH: allow Epochs to hold trials of different duration - #14210
ENH: allow Epochs to hold trials of different duration#14210snesmaeili wants to merge 11 commits into
Conversation
larsoner
left a comment
There was a problem hiding this comment.
Next step I would maybe add a new tutorial so we can see it working. If we need to add a new dataset we could, but maybe better would be to use openneuro-py in the example to download 1 subject's data and process it? MNE-BIDS does something like this and it seems to be okay. (Eventually we'll want to have CircleCI do this ahead of time so triage based on example content and modify _download_all_example_data, but we can do those steps later.)
| event_id: int | list[int] | dict | str | list[str] | None = None, | ||
| tmin: float = -0.2, | ||
| tmax: float = 0.5, | ||
| tmin: "float | np.ndarray" = -0.2, |
There was a problem hiding this comment.
Not sure why this would need to be string?
| tmin: "float | np.ndarray" = -0.2, | |
| tmin: float | np.ndarray = -0.2, |
|
|
||
|
|
||
| @fill_doc | ||
| class Epochs(BaseEpochs): |
There was a problem hiding this comment.
I see a lot of decorator mechanics etc. Would things get simpler if we added a EpochsRagged (or some better name) class instead of expanding Epochs itself? I think the decorator idea might have been mine but not sure if it's better or worse than a separate class... I'm thinking this isn't so bad, just want to make sure we thought about another option.
There was a problem hiding this comment.
I also spiked EpochsRagged(BaseEpochs) to check whether a separate class would actually simplify this. It doesn't avoid the two shared-module changes: with mixin.py and channels.py restored to upstream, epochs[0:2] fails in _getitem because it assumes ndarray storage, and pick() fails in _pick_drop_channels for the same reason. Avoiding those small shared branches would mean overriding the methods in the subclass and duplicating their selection / drop_log / metadata bookkeeping. Most of the remaining branches in epochs.py are construction and per-epoch-bound handling that would move into the subclass rather than disappear. So at the moment I think keeping one class is simpler, but I'm happy to switch if you prefer the stronger type separation.
The decorator mechanics are independent of that choice. I can replace the dynamic setattr wrappers with explicit variable-duration checks in the affected methods so the behaviour is visible where someone reading average(), filter(), etc. would expect to find it.
For the tutorial, sleep_physionet looks like a good fit and does not require a new dataset. Its hypnogram annotations already carry durations. The existing sleep tutorial intentionally uses chunk_duration=30. for the sleep-stage classification example; on SC4001 that converts 141 annotated sleep-stage bouts spanning 30–1890 s into 653 fixed 30-s events. That gives us a compact way to show the distinction between preserving annotation durations as variable-length epochs and explicitly converting them to a fixed-window representation. I can build the tutorial around one subject and show durations, get_times(), indexing, and as_fixed() with the contributing-count curve
There was a problem hiding this comment.
Okay sounds good to me! I think the decorators are simple enough and better than a bunch of repeated checks.
For the new example, CircleCI treats all warnings as errors so you probably need a verbose="error" (easiest way to suppress the warning) during raw read, see:
There was a problem hiding this comment.
(our CIs are under a heavy load so I'm going to kill the currently running ones to save some cycles under the assumption you'll fix this soon-ish and push!)
Just a quick note to say that |
e9808c4 to
0d3b433
Compare
Some experiments produce trials whose length is part of what is being measured: a gait cycle, a spoken word, a sleep stage. Cutting them to a common window either pads the short ones or truncates the rest, and both choices are made silently. `tmin` and `tmax` now accept an array with one entry per event, and `EpochsArray` accepts a list of (n_channels, n_times) arrays, deriving each epoch's `tmax` from its own length. Bounds that carry no actual variation collapse back to a single value, so nothing about the existing scalar path changes. The object reports itself through `variable_duration` and describes its trials with `durations` and `get_times(epoch)`. `times` refuses rather than inventing a shared axis, since returning the longest epoch's axis would leave `len(epochs.times) == data.shape[-1]` false while looking ordinary; `as_fixed()` returns the padded copy along with the number of epochs contributing at each sample, so the cost of padding is visible rather than implied. Reading from `Raw` gives each epoch its own length while keeping the drop bookkeeping intact. Discussed in mne-toolsgh-14206.
With trials of differing length the methods divide into three kinds, and guessing which one you are calling is how a wrong answer gets returned quietly. Selecting epochs, selecting channels, dropping and shifting the time origin do not care how long each trial is, so they work as they always did; the per-epoch bounds travel with the epochs they describe. That needs one branch each in GetEpochsMixin._getitem, shift_time and _pick_drop_channels, since those hold the data as one array. _pick_drop_channels replaces the list contents rather than the attribute, which keeps `_data` an ndarray for Raw, Evoked and the rest. Reductions across a shared time axis decline and say what they would need: padding makes the number of contributing epochs a function of time, which no single nave describes. Measuring it is what settled this - on 43 epochs spanning 2.0-3.6 s, average() returns an Evoked that is 44% NaN while nave reports 43 where 3 epochs remain. Per-trial operations with no ragged implementation decline too, rather than running on a padded copy and returning a wrong answer instead of a slow one. plot() is among them for now; the next commit implements it. to_data_frame keeps a warning fallback, since its result is only read.
The epochs browser already draws its trials as a pseudo-continuous strip,
concatenating them and ruling a line at each boundary, so ragged epochs
need the samples they actually hold rather than a padded copy. plot()
leaves the not-implemented table and becomes native.
The x axis is built from those samples:
lengths = per-epoch sample counts
boundary_samples = np.r_[0, np.cumsum(lengths)]
boundary_times = boundary_samples / sfreq
n_times = boundary_samples[-1]
_n_times_per_epoch returns len(times) when durations are equal, so this
is one code path and reproduces the previous uniform grid exactly while
never reading `times`, which variable-duration epochs refuse to provide.
A window of k epochs from index i spans boundary_times[i + k] minus
boundary_times[i]; _epoch_window computes that for both backends, and
_get_start_stop and _load_data share _get_epoch_ix_range so the sample
bounds and the concatenated array agree by construction. That is what
makes the existing shape assertions meaningful for ragged windows.
Arrow keys step whole epochs and shift steps whole windows, home and end
ask the boundaries how many seconds an epoch is worth, and the scrollbar
draws each epoch at its own width. Vertical lines mark a latency relative
to each epoch's own event and are omitted from epochs too short to reach
it, replacing arithmetic that took the remainder against one duration.
Events map through each epoch's own window; the fixed path keeps its
existing bounds, whose upper limit overshoots the last sample by |tmin|,
rather than have that copied.
_compute_scalings failed first of all, before any of the above, since it
reshaped _data as an array. ICA sources reach the same browser without
supplying the new per-epoch arrays, so those are derived from the
boundaries when absent.
Non-matplotlib backends decline with a message naming matplotlib until
mne-qt-browser can consume boundary_times, boundary_samples and n_times,
which the params dict now carries; the browser tests skip there for the
same reason.
Builds epochs straight from the Sleep Physionet hypnogram durations, so
no new dataset is needed and tools/circleci_download.sh already prefetches
it. Bouts over five minutes are set aside to keep the padded array small,
leaving 130 epochs from 30 to 300 s.
Walks through what the object holds, which operations are unaffected by
ragged trials, which ones refuse and why, browsing them at their own
lengths, and what as_fixed() reports: 130 epochs at t=0 falling to 1 by
300 s, which is the reason average() cannot return an ordinary Evoked.
The browsing section picks bouts by taking the first occurrence of each
distinct value in `durations`, so five different lengths are guaranteed
rather than hoped for; here that is 120, 30, 150, 60 and 90 s. It runs
under use_browser_backend("matplotlib"), as several other tutorials
already do, because the doc build exports MNE_BROWSER_BACKEND=qt and qt
is tried first, and the PyQtGraph backend does not handle ragged epochs
yet.
The closing section points at the sleep-staging tutorial, where fixed
30 s windows are the right representation, so the two are not read as
alternatives.
0d3b433 to
bed56d5
Compare
Cropping asks for a window in seconds, and that question has an answer for each trial on its own: keep the samples inside it. No epoch has to be padded, stretched or compared with any other, so crop leaves the not-implemented table. The requested window is applied to every epoch independently and clamped to that epoch's own bounds where it reaches past them, which is what the fixed path does against its single interval. Selections for every epoch are computed before anything is written, so a window that misses one epoch fails and leaves the object as it was rather than dropping it. That failure comes from _time_mask seeing an inverted interval once tmax has been clamped back, which is the same route the fixed path takes. Clamping is reported once per bound rather than once per epoch, and only when it happened. A clamped tmax keeps that epoch's last sample even when include_tmax is False, matching the fixed path. Bounds are taken from the samples that survived, never from the requested float, so len(get_times(i)) continues to describe the block. Cropping can also remove the variation: when every epoch ends up on the same axis the blocks are stacked and the object becomes an ordinary Epochs again, which is checked by sample index and length rather than by comparing floats. The reductions then return on their own, since the wrappers ask about _variable_duration when they are called. ExtendedTimeMixin is untouched. It is shared with Raw, Evoked and TFR, and this behaviour belongs to Epochs.
larsoner
left a comment
There was a problem hiding this comment.
I see the NOT_IMPLEMENTED list has shrunk, which is good! But it will make it harder to review 😓
I would suggest to stop at the current list, see if there are candidates for simplification, get this plus the mne-qt-browser bit working well, then I find the time to read test manually and merge.
In the meantime, since this implements browsing, if you have more LLM cycles, can you ask a fresh agent to try to find corner cases across multiple interactive use cases (can use qtbot and/or QTest), clicking around, setting channel counts, having lots of epochs, few epochs, vastly different durations, dropped vs not, etc.? This would help ensure that the code here is robust. Might make your laptop kind of unusable for a bit but it will be able to iterate much faster than me...
|
|
||
|
|
||
| def _check_variable_bounds(tmin, tmax, n_events): | ||
| """Normalize ``tmin``/``tmax``, which may be given per event. |
There was a problem hiding this comment.
The diff is quite big at +2,030 -109, so I'm hoping / looking for some way to make the diff smaller. I don't see a lot of ways, but one way would actually be to remove the docstrings from these private helpers (other than the first line). LLMs are great at generating a lot of content like this but it ends up needing to be checked and maintained by humans, so if the names and likely types and shapes etc. are already unambiguous from the surrounding context, we have been tending to omit them nowadays. They tend to go out of date quite quickly as well, since private function docstrings are not checked by automated tooling, and it's too easy to forget to update them.
Route variable-duration browsing on a backend capability flag rather than the backend name, so mne-qt-browser can opt in. Duck-typed the way BrowserBase._has_time_slice already is, so an older mne-qt-browser still declines. Six tests whose skip reason claimed matplotlib-only now run on both backends. Three of these fixes are regressions on the *equal-duration* path, not ragged-only: - _recompute_epochs_vlines computed an unclamped sample offset, so a click in an epoch's last half sample rounded one sample past its end, a latency no epoch holds. Every line was dropped while the readout still showed the out-of-range value. The Qt backend kept its old path behind a guard; the matplotlib rewrite had none, so fixed-duration data went through the new code. - _draw_traces rebuilt the visible-epoch list by searchsorting the time range, which drops the last epoch when it holds one sample, and raises when that is the only visible epoch. Ask the view instead. - The colour band mask excluded each epoch's own first sample, so a one-sample epoch was drawn in its neighbour's colour and the window's first sample was never painted at all. Ragged-only: - _create_epoch_histogram called np.ptp(..., axis=2) on a list of arrays. Peak-to-peak is per trial, so compute it per epoch. - _getitem moved the per-epoch bounds but never re-derived the union time axis, so as_fixed() kept padding out to epochs that had been dropped: epochs[0] of a 100-sample epoch returned (1, 3, 280) with 540 NaN and n_contributing == 0 at 180 time points. crop() already re-derives it. - drop_bad(reject=...) reached Epochs.times and raised an internal error with no classification; it now declines clearly. The no-arg call still short-circuits, which _concatenate_epochs relies on. Also silence two ty diagnostics in _crop_variable that predate this branch. Verified against the pre-PR commit across 7,697 recorded states and 377 figures per environment: all 32 fields that determine what the reader sees are bit-identical, and the only movement is the vline landing on a real sample instead of between two.
Harness, per-slice reports, screenshots and the draft reply for the sweep behind mne-tools/mne-python#14210 and mne-tools/mne-qt-browser#452. The harness builds every expectation from the source arrays rather than from the object under test, and the fuzzer carries a mutation self-test, so a clean result means something. Raw run logs and caches are ignored; the reports carry the numbers. Findings are triaged against the equal-duration path throughout: anything that also happens without ragged input is recorded as pre-existing rather than fixed. Three defects turned out to regress equal-duration browsing.
Reduce the private helpers this branch adds to one-line summaries, dropping the Parameters/Returns/Notes sections, and cut the comments that only restated the line below them. Kept as they were: `_get_epoch_ix_range` and `_check_variable_duration_backend` in viz/_figure.py, which record the mne-qt-browser flag contract and the single-source-of-truth invariant behind the shape assertion in `_update_data`. `_decim_slice` in `__init__` was commented as avoiding a densifying `decimate()`; `decimate` is in `_VARIABLE_NOT_IMPLEMENTED` and raises, so the comment now says what the assignment is actually for. No behaviour change: stripping docstrings leaves every touched file with an identical AST, and tests, tutorial and doc/ are untouched.
`get_data()` dispatched to `_get_variable_data(picks, item, copy)` for variable-duration epochs, so `units`, `tmin` and `tmax` were accepted and then discarded: `get_data(units="uV")` returned volts and `get_data(tmin=..., tmax=...)` returned whole epochs. Raise instead, naming the argument and pointing at `as_fixed()`, which supports all three. The docstring promised a 3D array on both paths; it and the return annotation now cover the list of one array per epoch that ragged epochs return. `save()` asserts the array case it already guarantees, since it refuses ragged epochs.
`_load_variable_from_raw` returns a list, which its one-line summary now says, since the call site comment that said so went with the sections. `# First pass:` labelled a pair whose second half was a bare `# Second pass: apply` and was removed; drop the label rather than restore it.
`test_crop_keeps_epoch_bookkeeping` called `pytest.importorskip("pandas")`
after `import pandas as pd`, so the hard import raised first and the guard
never ran. This failed the minimal build.
The variable-duration browser tests ran under both backends, but `plot()` raises for a qt backend that does not announce `_SUPPORTS_VARIABLE_DURATION`, so six of them failed the Ultraslow_PG build. They passed locally only because mne-tools/mne-qt-browser#452 was installed. Ask the guard rather than the backend name, so they run wherever the backend really does support ragged epochs and skip with its own message where it does not. `test_plot_variable_duration_refuses_old_backends` builds its epochs directly, since it must still run when the fixture would skip.
|
Thanks @drammock for your in-person input on this — I agree with the concern about being conservative here.
The problem is that its interpretation depends on why the epoch ended. because at late times the fast-response trials are already gone. There is no single honest value of: evoked.navefor the whole trajectory. The frequency axis can be common while the time axis remains ragged. rather than because stretching/compressing the raw signal changes its physical frequencies. compute_tfr(..., average=False)and compute_tfr(..., average=True)as scientifically different problems. gives every event the same influence. gives every observed second equal influence. then longer segments contribute proportionally more observations. where every epoch covariance gets equal weight. But then a 10-second segment contributes 10 times as many observations as a 1-second segment.
there is no mathematical requirement that every segment has the same number of time samples.
is fine. may also be meaningful. has the same scientific meaning across observations. where each type represents a different process. segments["movement"]versus segments["preparation"]rather than accidentally averaging all intervals together simply because they live in the same container. Both epochs have the same duration, but they do not have the same time axis. but something more like: or This is also relevant to #5794, where the core issue is different per-trial temporal anchors rather than necessarily different numbers of samples. The exact order can change, but I think small method-family PRs with explicit validation are much safer than trying to make the whole
|
Draft, following the design discussed in #14206.
tminandtmaxaccept(n_events,)arrays. Bounds that carry no actualvariation collapse back to the scalar path, so existing behaviour is unchanged.
EpochsArrayalso takes a list of(n_channels, n_times_i)arrays and deriveseach epoch's
tmaxfrom its own length.New:
durations,get_times(epoch),variable_duration,as_fixed().as_fixed()returns a paddedEpochsArraytogether with the number of epochscontributing at each time point.
timesraises when durations vary, rather than returning the union. Returning itwould leave
len(epochs.times) == data.shape[-1]false while looking ordinary.The union is still available as
as_fixed().times.Method behaviour. Three groups rather than a blanket fallback:
pick,drop,__getitem__,shift_time,plotas_fixed():to_data_frameaverage,standard_error,subtract_evoked,iter_evoked,compute_tfr,compute_psd), plus per-trial operations with noragged implementation yet (
filter,apply_function,apply_baseline,crop,decimate,resample,plot_image,plot_topo_image,save,export)I originally had the reductions fall back with a warning, as suggested in the
issue. Measuring it changed my mind. On 43 epochs spanning 2.0–3.6 s,
average()returns an Evoked that is 44% NaN from the first drop-out onward,while
navereports 43 where 3 epochs remain — one short epoch takes out thewhole time point.
compute_tfrwas worse: it padded and then transformed, whichis the reverse of the order argued for in the issue.
Browsing. Following @drammock's point that
Epochs.plotalready draws apseudo-continuous strip,
plot()is native rather than a padded fallback. Thebrowser's x axis is built from the samples the epochs really hold:
_n_times_per_epochreturnslen(times)when durations are equal, so this isone code path and reproduces the previous uniform grid exactly. A window of
kepochs starting at
ispansboundary_times[i + k] - boundary_times[i];_get_start_stopand_load_datashare one index range, so the sample boundsand the concatenated array agree by construction rather than by arithmetic.
Arrow keys step whole epochs, the scrollbar draws each epoch at its own width,
and a vertical line marks a latency relative to each epoch's own event, omitted
from epochs too short to reach it.
Validated on synthetic epochs of 100, 250, 75 and 180 samples: boundaries are
the cumulative real sample counts,
n_timesis their sum rather thann_epochs × max, the loaded window equalsnp.concatenateof the source blocksbyte for byte, no NaN appears anywhere, and
as_fixed()is never called.Browsing is Matplotlib-only for now — other backends decline with a message
naming it. The PyQtGraph companion is mne-tools/mne-qt-browser#452, which
consumes the same
boundary_times/boundary_samples/n_timesthis puts inthe browser params; relaxing the guard here is a follow-up once that is released.
What was validated. Construction and extraction, plus a per-epoch TFR
pipeline built on
get_data(). On all 24 ds004505 subjects (29,546 swingcycles), every epoch is byte-identical to the raw slice it came from, and
re-running an existing ERSP analysis through the container reproduces the
previously computed maps at 0.000e+00 dB with matching retained counts. Scripts
and per-subject reports:
https://github.com/snesmaeili/meta-mne-python-sprint/tree/main/validation
Tutorial.
tutorials/epochs/70_variable_duration_epochs.pybuilds epochsfrom the Sleep Physionet hypnogram durations, so no new dataset is needed and
tools/circleci_download.shalready prefetches it. It shows the container, theoperations that are unaffected, the ones that refuse, browsing bouts of 120, 30,
150, 60 and 90 s at their real widths, and the contributing-count curve from
as_fixed(). Its closing section points attut-sleep-stage-classif, wherefixed 30 s windows are the right representation, so the two are not read as
competing.
This does not validate any padded path — the methods that would need one raise.
mne/tests/test_epochs.pypasses unchanged; 336 tests pass across epochs,variable-duration and browser tests.
Open questions are in #14206: whether
tmin/tmaxshould stay scalar withper-epoch bounds under separate names, what the reductions should eventually do
about a contributing count that varies over time, and the FIF representation.
AI assistance: I designed the approach and ran the analyses it is validated
against; Claude Opus 5 wrote the implementation and the tests under my
direction, which I reviewed and tested.