Spectopo plot converison

[innaamogolonova]

Implement spectopo plot rendering and tune the figure to match EEGLAB's spectopo output.

Final state:

What this does

Renders pop_spectopo / spectopo channel & component spectra with scalp maps, matching EEGLAB's figure:

• scalp-map row above the spectra axis, a vertical marker at each requested frequency with a leader line to its map, a +/− polarity colorbar, and EEGLAB's per-channel color order;
• scalp maps styled like EEGLAB topoplot: nose + ears, contour lines, head drawn inside the interpolation "skirt" (squeezefac), and a heavier head outline.

Numerical parity

spectopo computes the PSD with a symmetric Hamming window and no detrending, matching EEGLAB's pwelch path — verified equal to MATLAB spectopo (pwelch) to |Δ| < 1e-5 dB across all frequency bins on eeglab_data.set.

Colormap & display

• topoplot and the colorbar honor topoplot_options["colormap"] (default jet), so maps and bar stay consistent.
• The plotting functions build and return the figure without calling plt.show(); pop_spectopo displays it on the interactive path via figure.show() (no blocking, no double-render, no backend heuristic).

Tests

• tests/test_spectopo_parity.py — MATLAB parity test: EEGPrep channel spectra vs live EEGLAB spectopo (pwelch) within 1e-2 dB (observed < 1e-5); regenerates the reference through the MATLAB engine and skips when MATLAB/EEGLAB is unavailable (e.g. CI).
• tests/test_phase4_plot_wrappers.py — headless smoke tests for the plot structure: scalp-map count, colorbar presence, per-frequency markers (channels) / none (components), and no default title.

Notes

• No default title: like EEGLAB, spectopo no longer adds a suptitle by default (previously "Channel/Component spectra and maps") — minor user-visible change.
• The shared colorbar is polarity-only (+/−): with maplimits='absmax' each map is scaled independently.
• Frequency markers/leader lines are drawn only for channel/frequency maps (components have no single frequency).

[arnodelorme]

Looks good. Is there a numerical test case against MATLAB (same numerical output when the same input is being used)

[arnodelorme]

@claude, please comment on the current PR. Any point of concern. Run the spectopo test if you can, comparing the MATLAB and Python versions.

[claude]

Claude finished @arnodelorme's task in 3m 30s —— View job

---

PR #282 review — Spectopo plot conversion

• Read changed files (spectopo.py, topoplot.py, pop_spectopo.py, tests)
• Review PR for points of concern
• Run the spectopo parity test (Python vs MATLAB) — SKIPPED in CI (no MATLAB engine / no EEGLAB checkout available on the runner). The test collects, resolves imports, and skips cleanly via eeglabcompat.get_eeglab. Companion topoplot/spectopo unit tests (49 non-parity tests) all pass.
• Post review

---

Overall

Nice polish job — the plot layout, ears, contour lines, +/− colorbar, leader lines, and per-channel palette all bring the figure much closer to EEGLAB's. Numerics-wise, switching to a symmetric Hamming window and detrend=False is the right move to match MATLAB pwelch, and the docstring's claim of max |Δ| < 1e-5 dB is plausible with that config. The parity test itself is well structured (BLAS pinned, live regeneration via pop_saveset roundtrip, cleans up temp files, skips gracefully when MATLAB is absent) and mirrors test_eeg_rpsd_parity. Good pattern.

Points of concern

1. plt.show() in a library function (spectopo.py:186-187). plot_spectra now calls plt.show() unconditionally on any non-Agg backend. This is a behavior change from fig.tight_layout() and can:

   • Block the interpreter when a caller wanted to compose figures.
   • Cause double-render if the caller (e.g. a Jupyter cell, a GUI wrapper) also renders the returned fig.
   • Interact badly with EEGPrep's own GUI/console flow where pop_spectopo is invoked from a dialog and the outer code may manage the figure.
     Consider gating on an explicit show=True kwarg (default off) or leaving the show call to pop_spectopo. At minimum, the "agg" check assumes matplotlib's canonical "Agg" backend name — fine today, but any embedded backend (module://...) will trigger plt.show() too.

2. Empty titles in pop_spectopo.py (lines 57 & 70). The default titles "Channel spectra and maps" / "Component spectra and maps" were replaced with "". EEGLAB's spectopo also does not add a suptitle by default, so this matches parity — but it's a subtle user-visible regression for anyone who relied on the previous default. Worth mentioning in the PR description / release notes.

3. Fixed jet colormap in the polarity colorbar (spectopo.py:247). The colorbar is hard-coded to jet, but a user passing topoplot_options={"colormap": ...} will get scalp maps in a different cmap while the polarity bar stays jet. Minor, but easy to fix — read the cmap out of topoplot_options (default jet) and pass it into the ScalarMappable.

4. Symbolic +/− colorbar can mislead. With maplimits: "absmax", each scalp map has its own scale, so the shared bar is polarity-only. This matches EEGLAB, but the current ticks (-0.8, 0, 0.8) are cosmetic-only and unrelated to any data. If the intent is purely EEGLAB parity, fine — worth a one-line docstring note that the colorbar is polarity-only.

5. Test coverage is narrow relative to the PR scope. The PR is mostly a plot conversion (topoplot cosmetics, layout, colorbar, leader lines, ears, contour, +/- bar), but the parity test only asserts numerical equality of pwelch output. That's the right answer to @arnodelorme's ask — but it does not guard against future regressions in the plot layout / topoplot styling. Consider:

   • A smoke test that renders with sample data on Agg and asserts axis counts, colorbar presence, and title behaviour.
   • A visual-parity capture case under tools/visual_parity/ (per .agents/skills/eeglab-gui-visual-parity/SKILL.md) to lock in the EEGLAB look.

6. Trace color indexing (spectopo.py:159). Uses _TRACE_COLORS[(index + 1) % len(...)]. EEGLAB's canonical mapping is allcolors{mod(k-1,7)+1} (1-based → 0-based). With (index+1) % 7, channel 0 gets red (index 1) instead of teal (index 0). Not a bug for parity if the author verified against mod(k,7)+1 (which some spectopo variants do use), but it's worth double-checking against the exact EEGLAB source revision being ported — hard to verify here because src/eegprep/eeglab/ isn't populated on this runner.

7. freq_case gates leader lines (spectopo.py:146). For component maps (map_values passed in), no leader lines are drawn. That's correct — components don't map to a single frequency — but the vertical markers on spec_ax also get skipped. EEGLAB also skips them in the component case, so parity holds; just flagging so a reviewer notices this asymmetry is intentional.

8. _frequency_window snaps xlim to the nearest computed frequency bin. EEGLAB uses the requested bounds directly. On a coarse frequency grid the axis limits will differ slightly from EEGLAB's. Only cosmetic.

9. Parity test tolerance. SPECTRA_ATOL_DB = 1e-2 with observed |Δ| < 1e-5 dB gives 3 orders of magnitude of headroom — good buffer against float noise across BLAS builds, but tight enough to catch a real regression. Reasonable choice.

Answer to @arnodelorme's question

Yes — the PR adds tests/test_spectopo_parity.py, which regenerates a live MATLAB reference via EEG.spectopo and asserts np.testing.assert_allclose(py_spectra, ml_spectra, atol=1e-2) on the same eeglab_data.set dataset. I couldn't run it on this runner (no MATLAB engine, no EEGLAB checkout; the test skips with MATLAB/EEGLAB not available). It should exercise correctly on a machine with EEGPREP_EEGLAB_ROOT set and the MATLAB engine installed. Worth running locally before merge to confirm the observed 1e-5 dB match still holds.

Lint

• uv run ruff check and uv run ruff format --check on the four changed files: all pass.
• 49 spectopo/topoplot unit tests pass; 4 parity tests skipped (MATLAB not available).

Test plan when MATLAB is available

uv run pytest tests/test_spectopo_parity.py -v

· Branch: feature/inna-conversion

[arnodelorme]

@innaamogolonova would you mind addressing Claude comment. I think the plt.show() is an important problem. I am not sure the figure was meant to be shown like this. Then there are a few additional details to look into. Respond point by point. Make sure to use Fable if you can (better than Opus).

[innaamogolonova]

Addressing comments from Claude:

1. plt.show() removed, show call left to pop_spectopo
2. Added short note in PR description and one line comments
3. Colorbar cmap fix
4. Comment added
5. Added a smoke test
6. Verified correct: spectopo.m:165,552
7. Comment added
8. Cosmetic fix
9. No change needed

[innaamogolonova]

@claude please review and comment on the current PR. Review the addressed comment changes. Point out points of concern.

[claude]

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

---

PR #282 re-review — addressed comments

• Fetch branch, diff against develop, re-read all changed files
• Verify each of the 9 previous points was addressed as described
• Look for new points of concern
• Run relevant tests (test_phase4_plot_wrappers.py -k spectopo, test_topoplot.py, test_spectopo_parity.py) and pre-commit
• Post the re-review

---

Verification of the addressed items

#	Original point	Status	Notes
1	plt.show() in library function	✅ Fixed	plot_spectra returns the figure only; pop_spectopo.py:106-107 calls figure.show() on the GUI path. No backend heuristic.
2	Empty default title	✅ Documented	Both branches now carry # EEGLAB spectopo adds no default suptitle inline (pop_spectopo.py:57, :70), and the PR description calls it out.
3	Colorbar hard-coded to jet	✅ Fixed	spectopo.py:251 now reads (topoplot_options or {}).get("colormap") or "jet", so scalp maps and colorbar stay in sync.
4	Polarity-only bar could mislead	✅ Documented	Comments at _draw_maps_row docstring (lines 230-234) and above set_ticks (253-255) explain that with maplimits="absmax" the bar carries no per-map scale.
5	Test coverage narrow for plot layout	✅ Added	New test_pop_spectopo_channel_figure_structure (scalp-map count, colorbar presence at x0 > 0.9, per-frequency vertical markers, empty suptitle) and test_pop_spectopo_component_figure_omits_frequency_markers. Both pass headlessly on Agg.
6	Trace color indexing convention	✅ Verified	Comment updated to reference mod(k,7)+1 and the author reports it matches spectopo.m:165,552. That's the actual convention used in the spectopo channel-color loop (not all EEGLAB call sites use mod(k-1,7)+1), so (index + 1) % len(_TRACE_COLORS) in Python is correct.
7	freq_case gate is asymmetric on purpose	✅ Documented	Inline comment at spectopo.py:146-147 explains that component maps span the whole spectrum, so markers/leader lines are channel-only.
8	set_xlim snapping to bin	✅ Fixed	_frequency_window now returns raw (low, high) alongside (min_idx, max_idx); plot_spectra uses the raw bounds for set_xlim and only uses the snapped indices for the y-range window (which is what _spectra_ylim needs).
9	Parity tolerance	✅ No change needed	1e-2 dB tolerance with observed <1e-5 dB is a defensible headroom.

Points of concern (new / residual)

None of these are blockers; ordered by decreasing importance.

1. Head/ears may draw outside the axes when plotrad < rmax (topoplot.py:294-305). headrad = squeezefac * rmax = rmax² / plotrad; when a caller passes an explicit plotrad < 0.5, squeezefac > 1, headrad > 0.5, and the head/ears now poke past the fixed ax.set_xlim(-0.6, 0.6) / ax.set_ylim(-0.6, 0.65) (lines 309-310). The default path is safe because plotrad = max(plotrad, 0.5) (line 181), but user-supplied plotrad=0.4 etc. will clip. Cheapest fix: expand the axes limits to max(0.6, headrad + margin). Fix this →

2. Colorbar cmap fallback is right but subtle. plt.get_cmap((topoplot_options or {}).get("colormap") or "jet") treats colormap=None and colormap="" and colormap=0 all as "use jet." That's the intent, but if a user ever passes a literal Colormap instance (rather than a string name), plt.get_cmap(cmap_obj) in newer matplotlib versions raises. Consider using the value directly when it's already a Colormap. Optional.

3. Smoke test axis-count heuristic is brittle (test_phase4_plot_wrappers.py:107-108). [ax for ax in fig.axes if ax.images] depends on the colorbar axis not exposing .images. Today it doesn't, so the test passes; but any future change that adds a legend/annotation axis with an image, or switches the colorbar to a ScalarMappable backed by a QuadMesh, would silently break this assertion. A more explicit check would be to filter by axis label or by the axes stored in a returned map_axes list. Not urgent.

4. _frequency_window doesn't normalize inverted bounds. If a caller passes freqrange=[25, 2], set_xlim(low, high) yields an inverted axis. EEGLAB tolerates that (MATLAB also inverts). _spectra_ylim already sorted((min_idx, max_idx)), so the y-range is safe. If you want to be strict, sort bounds[:2] at the top of _frequency_window. Optional.

5. figure.show() in pop_spectopo on the Agg backend emits a UserWarning (pop_spectopo.py:106-107). Only hit when gui=True (i.e. a user-driven interactive session), so tests never exercise it — but if someone forces gui=True in a headless script, matplotlib will print FigureCanvasAgg is non-interactive, and thus cannot be shown. Fine to leave, but worth being aware of.

6. Parity test still unrunnable on this runner (no MATLAB engine / no EEGLAB checkout available). The test structure — BLAS pinned, live regeneration via .set roundtrip, tmp file cleanup, graceful skip — is correct and mirrors test_eeg_rpsd_parity. Please confirm the observed <1e-5 dB match locally before merge.

Test results on this runner

• uv run pytest tests/test_phase4_plot_wrappers.py -k spectopo -v → 8 passed (including the two new smoke tests).
• uv run pytest tests/test_topoplot.py tests/test_spectopo_parity.py → 26 passed, 4 skipped (parity tests skip: no MATLAB/EEGLAB).
• ./pre-commit.py --changed-from origin/develop → OK.
• uv run ruff check on the six changed files → All checks passed.

Summary

All nine review points from the previous pass were addressed correctly: the show-side-effect is gone, the colorbar honors the requested colormap, comments now explain the polarity bar, the component/channel asymmetry, the trace-color indexing convention (verified against spectopo.m:165,552), and the axis-limit fix. A smoke test now guards the layout. The remaining concerns are minor and non-blocking; the biggest one is the plotrad < rmax clipping edge case in topoplot.py. Recommend running the MATLAB parity test locally once before merge to confirm the <1e-5 dB result on your machine.
· Branch: feature/inna-conversion