pop_topoplot conversion

[innaamogolonova]

pop_topoplot: EEGLAB scalp-map parity

Brings pop_topoplot (and the shared topoplot) in line with EEGLAB for 2-D scalp maps.

Changes

• Per-map component scaling: component maps now scale each to their own absmax (was one shared scale); ERP latency maps keep a single shared symmetric scale — matching EEGLAB's maplimits='absmax' vs shared behavior.
• Colorbar labels: component-map colorbars use -/0/+ (arbitrary per-map scale), like EEGLAB; ERP colorbars stay numeric.
• GUI display: figures now show when launched from the GUI — pop_topoplot and plot_channel_locations previously built figures but never displayed them (same class of bug as the earlier spectopo fix).
• 5% color-axis margin: topoplot widens the color axis by EEGLAB's 5% caxis margin, so color scaling matches exactly (also improves spectopo, which scales via topoplot).

Verified against MATLAB EEGLAB

• Interpolated grid Zi identical to 2e-13; colormap (turbo) and contour levels identical.
• Plotted CLim now matches exactly — ERP [0 100 200 300] → ±16.52; components [1 2 3] → ±4.51 / ±1.92 / ±1.78.
• tests/test_pop_topoplot.py updated; suite green.

Notes

• Color scaling is at full numerical parity

[innaamogolonova]

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

[claude]

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

---

PR Review: pop_topoplot conversion

• Gather context on the PR changes
• Read modified files (pop_topoplot.py, topoplot.py, test_pop_topoplot.py)
• Cross-reference with EEGLAB behavior
• Analyze potential issues and concerns
• Post review with findings

---

Summary

The scope is tight and the changes are well-motivated. Per-map component scaling, -/0/+ polarity colorbars, the missing figure.show() in the GUI-launched path, and the 5% caxis margin are all real EEGLAB parity fixes, and the removal of the _is_absmax_maplimits special case in _plot_map_pages is a nice simplification (pop_topoplot now owns the default up-front and topoplot handles the string branch itself).

Below are concerns worth addressing (or at least discussing) before merging.

---

Concerns

1. _clim_kwargs mis-widens asymmetric numeric maplimits — topoplot.py:434

def _clim_kwargs(amin, amax):
    return {
        "vmin": amin + np.sign(amin) * _CLIM_MARGIN * abs(amin),
        "vmax": amax + np.sign(amax) * _CLIM_MARGIN * abs(amax),
    }

This is really vmin = 1.05 * amin, vmax = 1.05 * amax. That is correct for the absmax case where amin = -amax, but for asymmetric limits it doesn't widen the range — it shifts it.

• maplimits=[10, 100] → [10.5, 105] (low end moves up, range narrowed on the bottom)
• maplimits=[0, 100] → [0, 105] (no widening at the bottom because np.sign(0) == 0)
• maplimits=[-5, 100] → [-5.25, 105] (fine, but visually asymmetric)

EEGLAB's topoplot.m only applies the 5% factor inside the absmax branch (amax = 1.05*max(max(abs(Zi))), then amin = -amax); for explicit numeric MAPLIMITS it uses the user's values unmodified. Current pop_topoplot callers only ever hit the symmetric case, so this doesn't manifest today — but topoplot is public and this becomes a footgun the moment anyone calls it directly with an asymmetric range (spectopo, plugins, notebooks). Two safer options:

• Only apply the margin when maplimits == 'absmax' (i.e. inline the widening in that branch and drop the helper), or
• Widen relative to the range: pad = _CLIM_MARGIN * (amax - amin); return {"vmin": amin - pad, "vmax": amax + pad} (this reduces to ±1.05·L for symmetric limits, so the current tests still pass, and behaves sensibly for [10, 100] too).

2. plt.get_backend().lower() != "agg" is too permissive — pop_topoplot.py:110, 207

if gui and plt.get_backend().lower() != "agg":
    for figure in figures:
        figure.show()

This excludes agg but still calls figure.show() on non-interactive backends like pdf, svg, ps, cairo, template, which emit UserWarning: FigureCanvas<X> is non-interactive, and thus cannot be shown. Prefer:

if gui and plt.isinteractive():
    ...

or gate on matplotlib.get_backend() in matplotlib.rcsetup.interactive_bk. The same fix applies to plot_channel_locations (which additionally isn't gated on gui, so headless PDF/SVG runs will warn there).

There's a related asymmetry: pop_topoplot only calls .show() when gui=True. EEGLAB shows figures regardless. A user calling pop_topoplot(EEG, ..., gui=False) from eegprep-console in an interactive backend gets no figure without also enabling plt.ion(). Worth confirming whether that matches the intended console UX (plot_channel_locations shows unconditionally, which is inconsistent with pop_topoplot's gate).

3. Shared colorbar's gradient corresponds only to the last plotted map — pop_topoplot.py:269

def _add_map_colorbar(fig, image, axes, *, component):
    cbar = fig.colorbar(image, ax=axes, shrink=0.7)
    ...

image is ax.images[-1] from the loop, i.e. the last map on the page. For component pages, each map now has its own clim, so the colorbar's numeric range and color gradient match only the last IC. The -/0/+ labels obscure this (the whole point), but the gradient itself is still that of the last image, which is a subtle inconsistency vs. what users might expect from a "shared" colorbar. If EEGLAB reproduces exactly this "last-map-drives-the-legend" behavior for component pages, keep it; if not, worth a comment noting why we accept it (or moving the colorbar per-axes for component pages, since we've stopped sharing the scale anyway).

4. test_pop_topoplot_component_colorbar_uses_polarity_labels — negative assertion is weak — tests/test_pop_topoplot.py:135

erp_labels = [text.get_text() for text in erp_figs[0].axes[-1].get_yticklabels()]
assert erp_labels != ["-", "0", "+"]

Almost anything satisfies "not equal to that exact three-element list" — even an empty list, or a labels-only-drawn-on-some-ticks state. Consider asserting positively that the ERP colorbar labels are numeric (e.g. all(text_is_numeric(l) for l in erp_labels) or check that at least one non-empty label parses to a float).

5. Self-referential expected values in test_pop_topoplot_component_pages_scale_each_map_to_own_absmax — tests/test_pop_topoplot.py:110-114

The expected clim is computed by calling topoplot(..., noplot="on") and multiplying by 1.05, so the test asserts the widening is consistently applied rather than validating the widened value against an external oracle. That's fine for regression coverage, but with the numbers cited in the PR body (±4.51 / ±1.92 / ±1.78) already verified against MATLAB, it's cheap to hard-code one of those as a golden and pin the parity claim into CI — otherwise the tests will silently accept future drift in _clim_kwargs or in griddata_v4.

6. Minor style / cleanups

• pop_topoplot.py:97-98: plot_options = dict(options) then .setdefault(...) — since _plot_map_pages immediately .pop()s from its options dict, this shallow copy is only needed if options might be mutated elsewhere later. Not wrong, just extra motion.
• _add_map_colorbar returns cbar but no caller uses the return value; drop it or document why.
• _clim_kwargs and its _CLIM_MARGIN constant would be easier to reason about if the docstring stated the actual formula (v = 1.05 * v, not "widens by 5%") — the two are the same for symmetric absmax but not for asymmetric inputs (see refactor for packaging and test distribution #1).

---

Nothing blocking, but I'd want #1 and #2 fixed before merge

• refactor for packaging and test distribution #1 is a latent correctness issue in a public function; the current tests don't exercise it, but the next topoplot caller that passes asymmetric maplimits will hit it.
• Refactor #2 is a benign UX regression on non-agg/non-interactive backends (spurious warnings), easy to fix.
• Fix mutable default argument and incorrect file loading order #3–coords not defined #6 are code-quality / test-quality improvements you can take or leave.

I ran no tests locally in this review pass — this is a read-only review of the diff against origin/develop.
• Branch: feature/inna-conversion

[innaamogolonova]

Addressing review comments:

#1 — _clim_kwargs asymmetric widening: This is faithful to EEGLAB — topoplot.m:1247-1250 applies the same per-endpoint v → 1.05·v to numeric maplimits, not just absmax, so a range-relative pad would diverge from the oracle. Keeping as-is (all pop_topoplot callers pass symmetric limits anyway).

#2 — backend guard too permissive: Fixed in e2c5707 — display is now gated on whether the backend is interactive (excluding Agg/PDF/SVG/PS/Cairo/template), so non-interactive backends no longer warn. Also aligned with EEGLAB, which shows the figure from both the GUI and the command line.

#3 — shared colorbar reflects the last map: Intentional parity — EEGLAB's component pages do the same (cbar('vert',0,get(gca,'clim')) uses the last axes' clim), and the -/0/+ labels exist precisely to signal that the scale is per-map, not a shared numeric one. Keeping as-is.

#4 — weak negative assertion: Fixed in 1053db8 — the ERP branch now positively asserts the colorbar labels are numeric (≥2 non-empty labels, each parses as a float) instead of just != ["-","0","+"].

#5 — self-referential expected values: This test is intentionally regression-only: it verifies the per-map scaling and 5% margin are applied consistently. The numeric core it relies on — the interpolated grid — is already pinned against MATLAB to 1e-5 in TestTopoplotParity, so external-oracle coverage exists where it matters.

#6 — minor cleanups: Done in 76f4247 — dropped the unused _add_map_colorbar return and rewrote the _clim_kwargs comment to state the exact formula (1.05*v per endpoint). The dict(options) copy is intentional, since _plot_map_pages pops from it.

[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 2m 44s —— View job

---

PR Re-review: addressed comment changes

• Read updated pop_topoplot.py, topoplot.py, test_pop_topoplot.py at PR head (76f4247)
• Verify refactor for packaging and test distribution #1 justification against EEGLAB topoplot.m (lines 1243–1251)
• Verify Refactor #2 interactive-backend guard fix (commit e2c5707)
• Verify Resolve conflict between lincuan:patch-1 and sccn:develop in OctaveWrapper #4 numeric-labels assertion fix (commit 1053db8)
• Verify coords not defined #6 cleanups (commit 76f4247)
• Post updated review

---

Summary

The three commits that landed since the first pass address the two blocking concerns and the test-quality nit cleanly. The one I flagged as a latent correctness bug (#1) turns out to be a misread on my part — EEGLAB really does apply the 5% margin per-endpoint regardless of how amin/amax were sourced (I confirmed against sccn/eeglab functions/sigprocfunc/topoplot.m:1247–1251). Keeping _clim_kwargs as-is is the correct parity call. The remaining items (#3, #5) were accepted with reasoning that matches EEGLAB semantics.

---

Verified against EEGLAB

#1 — _clim_kwargs per-endpoint widening. Retracting my earlier concern. EEGLAB topoplot.m:1247-1251:

cax_sgn = sign([amin amax]);
if ~all(cax_sgn == 0)
    caxis([amin+cax_sgn(1)*(0.05*abs(amin)) amax+cax_sgn(2)*(0.05*abs(amax))]);
end

sits outside the MAPLIMITS dispatch (which just sets amin/amax from absmax, maxmin, or the numeric pair verbatim), so it applies to numeric limits too. The Python is a straight port. ✅

#4 — Positive numeric-label assertion. _is_numeric_label now parses float(text.replace("−", "-")) (nice catch on matplotlib's U+2212 minus rendering — a bare float() would falsely fail on "−16.52"), and the ERP branch asserts len(erp_labels) >= 2 and all(_is_numeric_label(...)). Genuinely tests the positive claim now. ✅

#6 — Cleanups. _add_map_colorbar no longer returns an unused cbar; _clim_kwargs comment now states the exact formula (1.05*v away from zero). Both applied. ✅

---

Concerns worth mentioning (all minor)

1. Zero-endpoint edge case in _clim_kwargs — topoplot.py:434

def _clim_kwargs(amin, amax):
    return {
        "vmin": amin + np.sign(amin) * _CLIM_MARGIN * abs(amin),
        "vmax": amax + np.sign(amax) * _CLIM_MARGIN * abs(amax),
    }

EEGLAB's version guards with if ~all(cax_sgn == 0) — if both amin and amax are zero, it skips caxis entirely, leaving matplotlib's autoscale. The port always returns {vmin: 0, vmax: 0} in that case, which matplotlib rejects (ValueError: vmin == vmax when set on the image). This can leak in through maplimits=[0, 0] (already possible via kwargs) and through an all-zero absmax map (already guarded upstream by limit > 0 — only the numeric branch is exposed). One-line fix, matches EEGLAB exactly:

if amin == 0 and amax == 0:
    return {}

2. _NONINTERACTIVE_BACKENDS is a hardcoded list — pop_topoplot.py:432

The set {"agg", "cairo", "pdf", "pgf", "ps", "svg", "template"} is exactly matplotlib's current matplotlib.rcsetup.non_interactive_bk — nice, and correct today. Using that module attribute directly would future-proof against a new file-only backend being added by matplotlib:

from matplotlib.rcsetup import non_interactive_bk
_NONINTERACTIVE_BACKENDS = frozenset(name.lower() for name in non_interactive_bk)

Take it or leave it — the drift risk is very low.

3. Behavioral change: pop_topoplot now displays figures on every interactive backend

Previously the figure.show() loop was gated on gui=True. It's now gated only on _backend_can_display(), so any direct call like pop_topoplot(EEG, typeplot=1, items=[0, 100]) will pop a window when running under QtAgg/TkAgg/etc. This matches EEGLAB's behavior (EEGLAB always shows the figure), but it's a semantic change from the previous branch: batch scripts that construct figures for later export will now flash windows on interactive backends. Nothing to fix — worth calling out in the PR description or release notes so downstream users aren't surprised.

4. Shared colorbar for component pages (previously #3) — accepted, no action

Author is right that EEGLAB uses the last axes' clim (cbar('vert', 0, get(gca,'clim'))) and that the -/0/+ labels exist precisely to signal per-map scaling. Parity is the right call. If you ever want to defend against confused readers, a one-line comment near _add_map_colorbar calling this out would help future maintainers not "fix" it back.

5. Self-referential test (previously #5) — accepted, no action

Reasonable to lean on TestTopoplotParity for the MATLAB-oracle grid check and keep this test as a regression harness. Pinning one of the reported CLim values (±16.52, ±4.51, …) as a MATLAB-golden would still be cheap insurance if the mapping between Zi.max() and the plotted CLim ever drifts. Optional.

---

Verdict

Ready to merge from my side. #1 above is a one-liner if you want it in this PR; #2 and #3 are nits/documentation. No blocking correctness or parity concerns remaining.

I ran no tests locally in this pass — read-only review of the diff against origin/develop.
• Branch: feature/inna-conversion

[innaamogolonova]

IC Component Maps:

Component GUI Dialog:

ERP GUI Dialog:

ERP Scalp Maps:

ERP Discrepancy Explanation:

[innaamogolonova]

ERP scalp maps after fix in sccn/eeglab#952