diff --git a/docs/epoch-review-ui-plan.md b/docs/epoch-review-ui-plan.md index 20a0ee6..92cf78b 100644 --- a/docs/epoch-review-ui-plan.md +++ b/docs/epoch-review-ui-plan.md @@ -45,12 +45,18 @@ That environment has hard constraints: and (b) can't even cross the worker boundary — a `PyProxy`/Figure isn't `structuredClone`-able, so `postMessage` throws `DataCloneError`. -**This is not an MNE-version problem.** We are already on **MNE 1.12.1** (current). -The recent errors (`plot_psd` → `compute_psd`, `events=None` → `events=False`) are -just the code catching up to the modern MNE API; bumping MNE won't remove them. -The interactive-GUI problem is architectural: **native Matplotlib GUIs don't exist -in WASM, and no MNE version changes that.** The only path to interactive epoch -review is to build the UI ourselves — which is what this plan is for. +**This is not an MNE-version problem — it's architectural.** We install the latest +MNE from PyPI (`internals/scripts/InstallMNE.mjs` lists `mne` **unpinned** in +`PYPI_PACKAGES`, so `postinstall` grabs whatever is current — a modern 1.x). The +recent errors (`plot_psd` → `compute_psd`, `events=None` → `events=False`) were +just the code catching up to the modern MNE API; no MNE version brings back native +Matplotlib GUIs in WASM. The only path to interactive epoch review is to build the +UI ourselves — which is what this plan is for. + +> **Note (grounding):** MNE is unpinned today. If exact-reproducibility of the +> cleaned output matters (design goal 3), consider pinning the MNE version in +> `InstallMNE.mjs` so a silent upstream bump can't change epoching/averaging +> behavior out from under the tests. --- @@ -133,14 +139,56 @@ Four concerns. Each has real design choices (deferred to the planning loop). We stop shipping a *GUI object* and instead ship the **raw numbers**, then render them ourselves. Needed from Python per "clean" request: - Epoch data array: `epochs.get_data()` → shape `(n_epochs, n_channels, n_times)`. -- Metadata: `ch_names`, `sfreq`, `times`, per-epoch condition (from the marker - registry), and the **auto-reject flags + `drop_log`** for suggestions. - -**Transport is the crux** (and the lesson from this whole debugging session): -don't JSON-serialize float arrays, and don't return a `PyProxy`. Get the numpy -buffer as a **`Float32Array` / `ArrayBuffer`** and send it as a **transferable** -over `postMessage` (zero-copy). Small metadata rides as JSON alongside. This -sidesteps the serialization wall entirely — buffers cross cleanly; Figures don't. + (Returns float64; see the transport note below on downcasting to Float32.) +- Metadata: `ch_names`, `sfreq`, `times`, **per-epoch condition codes** + (`epochs.events[:, -1]` — the numeric marker codes, *labeled* via the marker + registry's `codeToLabel`, not stored on the registry itself), and the + **auto-reject flags + `drop_log`** for suggestions. + +> **Grounding — exclude the Marker/stim channel from the arrays.** `get_data()` +> returns *all* channels, including the trailing `Marker` channel that carries the +> numeric event codes. The existing channel picker already drops it +> (`requestChannelInfo` runs `[ch for ch in clean_epochs.ch_names if ch != +> 'Marker']`); `get_epochs_arrays` must do the same, or the reviewer renders the +> raw code channel as a bogus "trace" and `ch_names` / the channel count are +> off-by-one. (The per-epoch condition codes still come from `epochs.events[:, +> -1]`, which is unaffected by dropping the channel.) + +> **Grounding correction — the registry gives labels, not colors.** +> `buildMarkerRegistry` (`markerRegistry.ts`) returns only `{ codeToLabel, +> eventId }`. There is **no color** on it. Condition colors today are hardcoded +> RGB palettes inside `utils.py` (`plot_topo`, `plot_conditions`). So condition +> coloring in the React UI needs a **new color source** — decide whether to (a) +> define a palette UI-side keyed by condition label, or (b) promote the `utils.py` +> palette into a single shared source both Python plots and the React reviewer +> read from (preferred, avoids the ERP/topo legend and the reviewer disagreeing on +> which color is which condition). Track this as a small design decision, not a +> reuse of existing code. + +**Transport is the crux** (the lesson of this whole debugging session): don't +JSON-serialize float arrays and don't return a `PyProxy`. Get the numpy buffer as +an **`ArrayBuffer`** and post it as a **transferable** (zero-copy); the metadata +above rides as JSON on the same message. Buffers cross cleanly; Figures don't. + +> **Downcasting to Float32 is safe here — and only here.** `get_data()` is +> float64; halving it to Float32 loses precision, but this buffer is *display +> only*. The apply path (§6d) is **index-based** (`epochs.drop(indices)`) — the +> epoch samples never round-trip out of Python and back, so the cleaned `.fif` +> stays bit-identical to what MNE would produce (design goal 3). Keep it that way: +> never reconstruct epochs from the Float32 buffer on the Python side. + +**This means extending the worker message contract, not just adding a Python +helper (grounded in `webworker.js` today).** The current handler is +one-size-fits-all: it runs `data`, and if the result has `.toJs` it converts the +`PyProxy` and posts `{ results, plotKey, dataKey }` **with no transfer list** — so +it can't ship a zero-copy buffer. Phase 0 must: +- Return the numpy buffer as an `ArrayBuffer` (e.g. `arr.tobytes()` off the + `PyProxy`, or via the emscripten heap) and `self.postMessage({ buffer, meta, + dataKey }, [buffer])` — the `[buffer]` transfer list is mandatory, or the buffer + is structured-cloned (a copy). +- Route it on a **new `dataKey`** (e.g. `'epochArrays'`) so it bypasses the + existing PyProxy→`toJs`→SVG-string switch in `pyodideMessageEpic` (which assumes + results are plain JS or an SVG string). ### 6b. Rendering Muse is tiny (4 ch × ~256 samples × dozens of epochs), but we must not design for @@ -162,10 +210,40 @@ Muse is tiny (4 ch × ~256 samples × dozens of epochs), but we must not design ### 6d. Apply path: renderer → Python - Collect rejected epoch indices + bad channels → dispatch an action → epic posts to the worker → Python `epochs.drop(indices)` + set `info['bads']` → save - `-cleaned-epo.fif` via the existing MEMFS/`saveEpochs` path. + `-cleaned-epo.fif`. - The result must be **bit-identical to what MNE's GUI would have produced** — the UI changes, the science does not. +**Persistence is a real gap — the Clean→Analyze round-trip is broken today.** +Traced end to end against `webworker/`, `src/main/index.ts`, `src/preload/index.ts`: +- **Save side:** `saveEpochs` (`webworker/index.ts`) posts `raw_epochs.save("")`, but Pyodide's worker FS is **MEMFS (in-memory)** — there is **no + NODEFS/`syncfs`/mount anywhere in `webworker/`**, so the write lands in the WASM + virtual FS, never on host disk. +- **Load side:** the Analyze screen re-reads *host disk* both times — at + `componentDidMount` it calls `readWorkspaceCleanedEEGData` (main-process `fs`) to + populate the picker, then on selection `LoadCleanedEpochs` → `writeEpochsToMemfs` + → `fs:readFileAsBytes` stages host bytes into MEMFS. It never reuses in-session + MEMFS epochs. +- **The gap:** nothing ever writes the cleaned `.fif` *to* host disk. Main has + write IPC (`fs:storePyodideImagePng`, `eeg:createWriteStream`, + `fs:storeAggregatedBehaviorData`) but **none writes an epoch `.fif` from the + worker**, so a fresh workspace has nothing for Analyze to load. The fix is a + **new MEMFS→host write-back bridge**, and because the worker can't reach + `ipcMain` it must cross every process boundary (see the `electron-ipc-channel` + skill) — don't collapse it to "postMessage → handler". The full chain: Python + `save()` to a MEMFS path → worker `pyodide.FS.readFile(path)` + `postMessage` the + bytes back on a **new `dataKey`** (e.g. `'savedEpochs'`, routed in + `pyodideMessageEpic` beside the existing `epochsInfo`/`channelInfo` cases — which + return no transfer list today, so shipping bytes zero-copy means extending that + reply path too) → a new epic → a new `window.electronAPI` method → the preload + bridge → a new main handler. **Reuse anchor, not new machinery:** + `fs:storePyodideImagePng` (`src/main/index.ts` line 315) is already this pattern — + `(title, imageTitle, rawData: ArrayBuffer)` → `Buffer.from(rawData)` → + `fs.writeFile` to disk; the write-back handler should mirror *it* (a binary + renderer→disk write), not `fs:readFileAsBytes`. This is not a "persist" nicety — + **it repairs a currently broken flow**. + ### 6e. The onboarding layer (the differentiator) - Plain-language explanations of epochs and each artifact type. - **Guided mode** (default for newcomers): step through auto-flagged epochs with @@ -180,21 +258,36 @@ Muse is tiny (4 ch × ~256 samples × dozens of epochs), but we must not design - **Worker protocol** (`pyodide-mne` skill): today's plots use fire-and-forget `postMessage` + `plotKey`/`dataKey` reply routing through `pyodideMessageEpic`. - A data-heavy request/response (fetch epoch arrays, get result back, then let the - user act) pushes toward finally doing the **`runPython` RPC** already tracked in - `TODOS.md` ("Optional Full Pyodide worker RPC"). This feature is the strongest - reason yet to build it — worth deciding early. + Note the `dataKey` pattern **already does request/response round-trips** + (`epochsInfo` → `SetEpochInfo`, `channelInfo` → `SetChannelInfo`), so fetching + epoch arrays and applying rejection can be built on it directly — a `runPython` + RPC is **not a hard prerequisite**. The `TODOS.md` item ("(Optional) Full + Pyodide worker RPC") itself says it's only worth doing "if the FIFO sequencing + ever actually bites." Where it *would* help this feature: an `await`-able RPC + lets an apply-then-refresh sequence (drop epochs → save → re-fetch stats/ERP) + read real results instead of relying on worker FIFO ordering across several + fire-and-forget messages. Treat it as an ergonomics/scaling call, not a blocker + (see Open Question 2). - **New Python helpers** (`webworker/utils.py`): `get_epochs_arrays(epochs)` → buffer + metadata; `apply_rejection(epochs, drop_indices, bad_channels)`. Keep them native-testable (the `tests/analysis/` pattern) so cleaning logic is verified against real MNE in CI. - **New epic(s)** in `pyodideEpics.ts` + **new actions** (fetch/set epoch data, - apply rejection). Reuse `buildMarkerRegistry` for condition labels/colors. -- **New React component** (`EpochReviewer` or similar) replacing the plot area in - `CleanComponent`. Styling: shadcn/ui + Tailwind, brand teal, student tone. + apply rejection). Reuse `buildMarkerRegistry` for condition **labels** (code↔label + only, no colors — see §6a for where condition colors must come from). +- **New React component** (`EpochReviewer` or similar) **added to** + `CleanComponent`. Correction: `CleanComponent` has **no plot area** to replace — + today it renders only subject/recording `