Port EEGLAB envtopo with ranking, maps, and click-to-enlarge

[innaamogolonova]

Port EEGLAB envtopo: rank ICA component contributions (mp/pv/pp/rp), draw the summed and per-component envelopes with scalp maps, and return the six EEGLAB outputs, verified against MATLAB. Adds click-to-enlarge, the summed-metric label, vert markers, and a sumenv toggle. Also fixes pop_saveset writing empty channel coordinates as 0, which misplaced no-location channels on reload.

Visual parity:

[innaamogolonova]

@claude please review this PR which brings envtopo up to parity with EEGLAB's implementations. Highlight any points of concern that should be addressed before merge.

[claude]

Claude finished @innaamogolonova's task in 5m 27s —— View job

---

Review — PR #307: Port EEGLAB envtopo

• Read the PR diff and understand scope
• Review envtopo.py port for EEGLAB parity and correctness
• Cross-check with EEGLAB envtopo.m behavior
• Review pop_envtopo.py and GUI wiring
• Review pop_saveset.py channel-coordinate fix
• Review axcopy.py new helper
• Review tests (test_envtopo.py, test_envtopo_parity.py, test_pop_saveset.py, test_phase4_plot_wrappers.py)
• Post consolidated review

Overall

Solid, well‑tested port. The envtopo core is small (~500 lines) and clearly organized; the ranking/peak/envelope pipeline reads well and matches EEGLAB semantics. The parity suite covers all four sortvar modes (mp/pv/pp/rp), compnums, compsplot, limcontrib, and subcomps against MATLAB's live oracle, and the closed‑form unit tests use orthonormal maps so peaks and ranks are analytically checkable. axcopy is a nice, minimal analog of the EEGLAB feature, and the pop_saveset fix is scoped and comes with a proper regression test. Pre‑commit is clean; tests/test_envtopo.py, tests/test_pop_saveset.py, and the envtopo tests in tests/test_phase4_plot_wrappers.py all pass locally (uv run pytest, matplotlib.use("Agg")).

Points to address before merge

1. GUI empty subcomps field triggers "remove all but candidates" — likely wrong default UX

   • In pop_envtopo._run_gui (pop_envtopo.py:141) an empty edit box maps to [], which is then handed to envtopo(subcomps=[]).
   • envtopo._resolve_subcomps (envtopo.py:280–291) treats [] as "remove all but the candidate components", whereas subcomps=0 (the console/default) means "remove none".
   • Result: opening the GUI, leaving the "Component numbers to remove …" field blank, and hitting OK silently subtracts every non‑candidate component before ranking — the opposite of what most users will expect. Please either (a) have _run_gui translate empty → 0 (i.e. "remove none") or (b) drop the [] → remove-all-but-candidates branch entirely (it isn't exercised by any test and is not what EEGLAB's blank field does). Also worth adding an explicit test for the empty GUI subcomps case.

2. Enlarged scalp‑map annotation shows a bare number with no unit or %

   • _redraw_map_popup at envtopo.py:495 writes f"{metric_mode}: {metric_value:.2f}". For mp the value is a raw power in µV² (no unit shown); for pv/pp/rp it's a percent (no % shown). The envelope panel already uses the labeled/percent form (f"{metric_label} {summed_metric:.2f}%" at envtopo.py:426) — the popup should be consistent, e.g. f"{label}: {value:.2f}%" for the three percent modes and f"mp: {value:.2f} µV²" (or drop mp altogether since MATLAB doesn't annotate it) for the raw‑power mode.

3. plotchans + full chanlocs will misalign the top scalp maps

   • In _draw_maps_row (envtopo.py:458) the map values are max_projections[plot_channels, source] (size = len(plotchans)), but locs at envtopo.py:336 is built from the full chanlocs list. If a caller passes plotchans=[…] together with the full chanlocs, the two arrays are size‑mismatched and topoplot will either raise or silently misplace values.
   • Not reachable through pop_envtopo today (no GUI/kwarg surfaces plotchans) so this is low severity, but it's still a latent trap in the public envtopo() API. Either subset locs to plot_channels before passing to topoplot, or document/enforce that chanlocs must already correspond to plotchans.

4. kwargs.pop("compnums", kwargs.pop("components", None)) always pops components

   • pop_envtopo.py:81: Python evaluates the default argument first, so kwargs.pop("components", None) always runs, silently discarding a caller's components=… even when compnums=… is also present. Use kwargs.pop("compnums", None) first and fall back with if compnums is None: compnums = kwargs.pop("components", None).

5. Minor: reference recomputed in envtopo() after _contributions already computed it

   • envtopo.py:156 duplicates the reference computation from _contributions (envtopo.py:186). Either return it from _contributions or make it a tiny helper — trivial cleanup.

6. Minor cosmetic parity: vert markers are solid black dashed, ~2 px thick

   • EEGLAB draws the user vert markers as thinner light lines (typically linewidth 0.8, gray or the summed‑fill blue). Not a correctness issue; the current style is quite prominent and can compete visually with the summed envelope. Worth a quick side‑by‑side against EEGLAB for the visual‑parity gate.

Nice things worth calling out

• Explicit docstring flagging both deliberate departures (compvars semantics; the rms per‑sample‑count fix vs. MATLAB's broken branch). This is the right pattern.
• EnvtopoResult is a clean NamedTuple with a boundary note that compframes are 0‑based and comptimes are ms — the exact place indexing boundaries usually go wrong.
• The pop_saveset fix uses object‑dtype columns only when at least one channel has a missing coordinate; homogeneous numeric columns stay numeric. The regression test both round‑trips through pop_loadset and asserts on the raw MATLAB struct.
• axcopy docstring correctly notes it's a no‑op on non‑interactive backends; the tests exercise it via a synthetic click. This keeps CI headless while still covering the enlarge path.
• Parity test regenerates the MATLAB reference live (no committed .mat) and its comment about envtopo.m:731 double‑applying the sort permutation is a helpful landmine flag.

Test status

• uv run pytest tests/test_envtopo.py tests/test_pop_saveset.py — 24 passed (all pre‑existing plus the new coverage)
• uv run pytest tests/test_phase4_plot_wrappers.py -k envtopo — 3 passed (click‑to‑enlarge, history threading, existing wrapper)
• MATLAB parity tests in tests/test_envtopo_parity.py are gated on MATLAB Engine + EEGLAB and are skipped in this environment; the harness structure is standard.
• ./pre-commit.py --changed-from origin/develop — clean
  • Branch: feature/inna-envtopo

[innaamogolonova]

Summary of changes:

1. Blank GUI subcomps now removes none (00d7678) — a blank "remove components" field mapped to [], which triggered the "remove all but candidates" branch and corrupted the black data-envelope reference. It now maps to 0 (remove none), matching EEGLAB's blank-field behavior and the console default. Added a GUI regression test plus a _resolve_subcomps semantic test.
2. Enlarged maps annotated with units (c3257df) — the pop-out scalp map showed a bare number; it now shows µV² for mp and % for pv/pp/rp, consistent with the envelope panel. Tests cover both a raw-power and a percent mode.
3. plotchans + full chanlocs alignment (a27d00b) — a plotchans subset paired with full chanlocs size-mismatched topoplot (raised IndexError). locs is now subset to the plotted channels; no-op for the default and pre-subset cases. Added an envtopo()-level test.
   4/5. Minor cleanup (ee9d154) — clarified the compnums/components fallback (this was not a behavior bug — compnums already took precedence, so it's a readability fix) and deduped the reference-power computation into a _reference helper.

[innaamogolonova]

@claude can you check the updates to this PR

[claude]

Claude finished @innaamogolonova's task in 3m 31s —— View job

---

Re-review — PR #307 follow-up on prior feedback

• Fetch latest commits and diff since previous review (00d7678, c3257df, a27d00b, ee9d154)
• Verify fix refactor for packaging and test distribution #1: blank GUI subcomps → remove none — resolved
• Verify fix Refactor #2: enlarged map annotation units — resolved (test structure regression, see below)
• Verify fix Fix mutable default argument and incorrect file loading order #3: plotchans + full chanlocs alignment — resolved
• Verify fix Resolve conflict between lincuan:patch-1 and sccn:develop in OctaveWrapper #4/5: compnums/components fallback + reference dedup — resolved
• Run relevant tests + pre-commit — all green

Overall

All four follow-up commits directly address the prior feedback and land the intended fixes. One small test-structure regression from c3257df is worth cleaning up before merge, but there's no correctness change needed to the ported code.

Verified fixes

1. Blank GUI subcomps → remove none (00d7678) — pop_envtopo._run_gui now maps an empty edit box to 0 via numeric_vector(...).tolist() or 0 (pop_envtopo.py:146), and the new test_pop_envtopo_blank_gui_subcomps_removes_none asserts subcomps=0 appears in the emitted history and subcomps=[] does not. The _resolve_subcomps semantic test in test_envtopo.py locks the 0 vs [] divergence for the API layer too. Help text updated to explain the blank-field behavior. ✓
2. Enlarged map units (c3257df) — _redraw_map_popup now formats mp as µV² and pv/pp/rp as % (envtopo.py:499–505), consistent with the envelope-panel label. Tests cover both a raw-power (mp) and a percent (pp) mode. ✓
3. plotchans + full chanlocs alignment (a27d00b) — _build_figure now subsets locs to plot_channels when the full chanlocs was passed alongside a plotchans subset (envtopo.py:341–343). Guard is plot_channels.size < len(locs), so a caller-pre-subset chanlocs (equal length) is left untouched. New test_plotchans_subset_with_full_chanlocs_draws_maps covers the previously size-mismatching case at the envtopo() boundary. ✓
4. compnums/components fallback clarity (ee9d154) — Rewritten as an explicit if compnums is None: (pop_envtopo.py:74–76). Behavior unchanged, but the read is now obvious; the previous form worked because kwargs.pop returns before the default is evaluated (I mis-called this as a live bug in my earlier review — it wasn't; the fix is a readability win, not a correctness one). ✓
5. Reference dedup (ee9d154) — Extracted _reference(data_win, metric_mode) and called from both envtopo() and _contributions() (envtopo.py:156, 186, 207–209). ✓

One nit to fix before merge

Test-structure regression in c3257df — the new test_pop_envtopo_enlarged_map_annotation_uses_percent_for_pvaf (tests/test_phase4_plot_wrappers.py:1468) was inserted inside the old test_pop_envtopo_click_enlarges_map_and_envelope. The tail lines 1487–1494 (# A non-left button does not pop anything out …) are now indented under the new function rather than the old one, so:

• test_pop_envtopo_click_enlarges_map_and_envelope no longer verifies the right-button-ignore contract it was written for.
• The new pvaf test ends up running the right-button-ignore check against its own already-closed figure (plt.close(figure) at 1485, then a .transData.transform on map_ax at 1489 and a redundant plt.close(figure) at 1494). It passes today only because matplotlib still dispatches to detached callbacks and envtopo's right-click branch is a no-op — but the intent is jumbled.

Fix is a small reshuffle: move the "non-left button" block back under test_pop_envtopo_click_enlarges_map_and_envelope (right after the env-panel enlarge check), and let the pvaf test end at plt.close(figure). Also drop the duplicate plt.close(figure). Fix this →

Test status

• uv run pytest tests/test_envtopo.py tests/test_pop_saveset.py — 26 passed
• uv run pytest tests/test_phase4_plot_wrappers.py -k envtopo — 5 passed (both new tests included)
• ./pre-commit.py --changed-from origin/develop — clean
• MATLAB parity tests in tests/test_envtopo_parity.py remain gated on MATLAB Engine + EEGLAB and are skipped here.

Once the test-structure nit is untangled, this looks ready to merge.
• Branch: feature/inna-envtopo