Display figures from GUI/console and add a plot on/off flag

[innaamogolonova]

Plotting pop_* functions built a figure but never displayed it from the GUI or eegprep-console. Route every plot wrapper and the topoplot core through a shared show_figures helper, and keep the console's matplotlib non-interactive so display is explicit. Adds a keyword-only plot flag on the plot wrappers: plot='on', plot=True, or omitting it shows the window; plot='off' or plot=False builds and returns the figure without a window; any other value raises ValueError. Updates docstrings and GUI help.

[innaamogolonova]

@claude review this PR please, outline points of concern. the PR is supposed to address the following two issues:

1. Plots not displaying from the GUI — plot wrappers built a figure but never showed it; now they display via show_figures.
2. No terminal show/suppress control — added plot='on'/'off', with the console kept non-interactive so plot='off' produces no window (and no focus steal).

[claude]

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

---

PR Review — Display figures from GUI/console + plot='on'/'off' flag

• Fetch PR diff vs origin/develop (merge base 72f81bd7)
• Review plot_utils.show_figures helper
• Review console non-interactive backend change
• Review pop_* wrappers for consistent plot='on'/'off' handling
• Review topoplot core changes
• Review tests
• Review docs/help updates
• Post findings

Overall this is a nice, focused improvement — centralizing display through show_figures(), plus a scriptable plot='on'/'off' flag. The tests cover both the interactive-backend show path and the plot='off' close path. A few concerns worth addressing:

---

1. Layering violation: sigprocfunc/topoplot.py imports from popfunc/plot_utils.py

src/eegprep/functions/sigprocfunc/topoplot.py:7 now does:

from eegprep.functions.popfunc.plot_utils import show_figures

Per AGENTS.md, sigprocfunc/ is low-level signal processing and popfunc/ is the wrapper layer. The dependency should flow popfunc → sigprocfunc, not the reverse. This is a cycle waiting to happen (and semantically wrong for a "pop"-namespaced helper). Two clean options:

• Move show_figures (and the tiny backend_can_display) into a neutral module such as functions/miscfunc/plot_utils.py (or sigprocfunc/_plot_display.py), and re-export from popfunc.plot_utils for the existing callers.
• Or keep a private _show_if_interactive-style helper local to sigprocfunc/topoplot.py — matches the pre-PR shape and avoids the upward import.

Fix this →

---

2. plot=False / plot=0 silently opens a window

plot_utils.py:29 uses if str(plot).lower() == "off":. A Python user who reasonably writes pop_spectopo(EEG, ..., plot=False) gets str(False).lower() == "false" — not "off" — so the window opens anyway. Same for plot=0. Given the flag is Python-facing and the type annotation is str, either:

• Accept and normalize truthy/falsy values (False, 0, "off", "no", etc.), or
• Explicitly validate and raise on unrecognized values so silent misconfiguration can't happen.

Fix this →

---

3. Inconsistent ('plot','off') name/value pair handling across wrappers

pop_spectopo.py:56 uniquely pops plot out of the options dict:

plot = str(options.pop("plot", plot))

None of the other wrappers do this. An EEGLAB-style history call like pop_topoplot(EEG, 0, [1], 'plot', 'off') will end up with 'plot' in the options dict for topoplot() (which does not understand it) rather than actually suppressing the window. Recommend either:

• Doing the same options.pop("plot", plot) in every wrapper that goes through parse_key_value_args, or
• Documenting that the flag is Python-keyword-only and dropping the pop_spectopo special case for consistency.

Fix this →

---

4. plot_channel_locations no longer has a display guard and no plot kwarg

pop_topoplot.py:194 used to gate its fig.show() on _backend_can_display(). That guard was removed and the display was pushed into topoplot() core — but the _blank_topoplot code path calls show_figures(fig) unconditionally when it owns the figure. On any non-Agg backend that will now show, regardless of caller intent. Consider:

• Threading a plot='on'/'off' kwarg through plot_channel_locations and passing it to show_figures, or
• Reintroducing a small internal helper in the topoplot core to skip when the caller doesn't want a window.

---

5. plt.close(fig) on plot='off' — subtle gotcha for callers

show_figures closes the figure on plot='off' (plot_utils.py:33). The docstring hint says:

plot="off" suppresses the window but still returns the built figure.

show_figures itself returns None, and the wrappers still return the (now-closed) Figure. fig.savefig(...) still works, but adding axes or later calling plt.show() on it will not. Consider:

• Rewording the docstring so callers know the figure is unregistered from pyplot.
• Adding a small test that fig.savefig(BytesIO()) works after plot='off' to lock the intended contract.

Also — pop_prop._plot_one_property attaches figure.eegprep_activity_view = property_activity_browser(...) before show_figures(...). If plot='off' closes the figure, the attached browser may end up with a dangling parent. Worth verifying that behavior explicitly.

---

6. show_figures docstring says "still returns the built figure" — it returns None

Small but confusing:

def show_figures(figures, *, plot="on") -> None:
    """... plot="off" suppresses the window but still returns the built figure."""

The function returns nothing; the caller (the pop_* wrapper) is what returns the figure. Reword the docstring to make that clear (or drop the return sentence).

---

7. Console switch: enable_gui("qt") → enable_matplotlib("qt")

console.py:660-663:

self.shell.enable_matplotlib("qt")
plt.ioff()

enable_matplotlib deliberately turns interactive mode ON, so the immediate plt.ioff() is important. The test test_run_console_forwards_cli_options_to_gui_launcher correctly asserts not plt.isinteractive(), which is good. A couple of things to double-check:

• Is there anything else in the workspace (extensions, tutorials) that assumed plt.isinteractive() was true after console startup?
• The _FakeShell in tests/test_console_workspace.py now must implement enable_matplotlib instead of enable_gui. Confirm no other tests/plugins still stub enable_gui.

---

8. Tests — nice coverage, one small suggestion

tests/test_phase4_plot_wrappers.py:862-988 exercises the interactive-vs-noninteractive paths well and even locks in the "topoplot core displays only when it owns the figure" contract. Two nice-to-haves:

• Add a case for plot=False / plot=0 (see finding Refactor #2). If normalized to "off", assert no window; if rejected, assert ValueError.
• Cover the ('plot','off') name/value form for at least pop_spectopo (currently the only wrapper that handles it), to prevent silent regressions.

---

9. Minor: pop_comperp builds the return dict before showing

pop_comperp.py:97-98:

result = {"erp1": erp1, ..., "figure": figure}
show_figures(figure, plot=plot)

On plot='off', result['figure'] is now the closed figure — same gotcha as #5, worth noting in the help doc if we keep the close-on-off semantics.

---

Summary of the fixes for the two stated issues

• Issue 1 (plots not displaying from the GUI): the shared show_figures() helper plus plt.ioff() in the console and enable_matplotlib("qt") collectively fix this. ✅
• Issue 2 (no terminal show/suppress control): the new plot='on'/'off' kwarg on all listed wrappers plus the plt.close(fig) on off achieves the "no window, no focus steal" behavior. ✅

The remaining points above are correctness/consistency polish rather than blockers.

I did not run the test suite or pre-commit.py — this comment is a code-review only, per the trigger request.
· Branch: feature/inna-conversion

[innaamogolonova]

Addressed the actionable points in c59a8ff and f3600c2:

• 2 (silent plot=False): show_figures now accepts 'on'/'off' and True/False, and raises ValueError on anything else — no more silent misconfiguration. Annotations updated to str | bool.
• 3 (inconsistent 'plot' handling): treating plot as a Python keyword-only flag; kept the pop_spectopo options.pop("plot", …) only as a guard so the value can't leak into the core spectopo(plot=…) and suppress drawing.
• 6 (docstring): reworded show_figures — it returns None; the caller keeps the figure, which is closed but still usable for savefig.
• 8 (tests): added bool-accepts, unknown-value-raises, and a savefig-after-plot='off' regression test.
• 5 / 7 / 9: verified — pop_prop's activity browser survives plot='off'; nothing else stubs enable_gui or assumes interactive mode; closed-figure semantics are documented.

Left as-is: 1 (no import cycle, and it matches the existing sigprocfunc/eegplot.py → plot_utils precedent) and 4 (plot_channel_locations always shows, which is correct for its only caller). Can change strict layering is preferred.

[innaamogolonova]

@claude review the changes that addressed the previous comment. review the state of the PR now and outline any other potential points of concern.

[claude]

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

---

PR Review — Follow-up on c59a8ff / f3600c2

• Fetch updated diff vs origin/develop
• Verify show_figures bool/'on'/'off' normalization + ValueError
• Verify per-wrapper plot handling / pop_spectopo guard
• Verify new tests (test_phase4_plot_wrappers.py)
• Re-scan console changes and other wrappers for new concerns
• Post findings

Thanks for the quick turnaround — most of the previous review's findings are now cleanly addressed. Rechecking each:

#	Prev. finding	Status
1	sigprocfunc/topoplot → popfunc.plot_utils layering	Deferred (author cites sigprocfunc/eegplot.py precedent — confirmed at sigprocfunc/eegplot.py:12). Fair, but see note below.
2	plot=False/0 silently opens window	✅ _plot_enabled in plot_utils.py:266 accepts bool, raises on anything else; annotations updated to str | bool.
3	Inconsistent ('plot','off') name/value pair handling	⚠️ Partially — see finding A below.
4	plot_channel_locations no display guard / no plot kwarg	Deferred (author: only-caller always wants to show). Fair.
5	plt.close(fig) on plot='off' gotcha for callers	✅ Docstring reworded; pop_prop browser attribute survives plt.close() since Python attrs outlive pyplot registry removal (verified in _property_browser.py).
6	show_figures docstring "returns the built figure"	✅ Fixed at plot_utils.py:24-30.
7	enable_gui→enable_matplotlib grep	✅ Confirmed — only remaining occurrence of "enable_gui" is a comment at console.py:658. _FakeShell.enable_matplotlib at test_console_workspace.py:1456 is the only stub.
8	Tests for bool + name/value + savefig	✅ Added: test_show_figures_plot_accepts_bool, test_show_figures_plot_rejects_unknown_value, test_wrapper_plot_flag_accepts_bool, test_plot_off_figure_still_savable.
9	pop_comperp returns closed figure in dict on plot='off'	Documented behavior; same gotcha holds. Non-blocker.

---

Remaining / new points of concern

A. pop_spectopo still accepts ('plot','off') EEGLAB-style pairs — nowhere else does

The author's stated contract is now "plot is a Python keyword-only flag." That is consistent everywhere except pop_spectopo.py:57:

plot = options.pop("plot", plot)

This rebinds plot, so a caller writing pop_spectopo(EEG, 1, [0, 1000], "EEG", "plot", "off") (MATLAB-style history) gets its window suppressed — but the same pair in pop_topoplot / pop_timtopo / etc. is silently absorbed into **kwargs for the core function (which does not understand plot) and the window still opens. That is the same asymmetric behavior finding #3 flagged.

The comment above the line says the intent is defensive against spectopo(plot=…) contamination — that's correct and important, since sigprocfunc/spectopo.py:43 really does take a plot kwarg. But options.pop("plot", None) (discard, don't rebind) achieves the guard without accepting MATLAB-style pairs in one wrapper only. Recommend:

options.pop("plot", None)  # guard: keep 'plot' out of spectopo() core

That preserves the guard and makes the "Python-keyword-only" contract truly uniform. Add a small regression test that ('plot','off') name/value pair does not suppress in any wrapper (or does everywhere, if you decide the other direction).

Fix this →

B. Minor: "plot" is dead in _split_spectopo_options spectral keys

pop_spectopo.py:283:

spectral_keys = {"plot", "winsize", "overlap", "nfft"}

plot is already popped at line 57 before _split_spectopo_options runs, so it can never survive to reach this set. If you keep the defensive intent (guard both call sites), fine — but at that point the current plot = options.pop("plot", plot) should also just be options.pop("plot", None). Either way one of these two is redundant.

C. test_plot_off_figure_still_savable doesn't lock the closure contract

tests/test_phase4_plot_wrappers.py:1033-1041 proves savefig works after plot='off', but doesn't assert the figure was actually unregistered from pyplot. Consider adding:

assert not plt.fignum_exists(figure.number)

so the contract "closed but savable" is fully pinned.

D. Docstring wording — closure is a real side effect worth naming

plot_utils.py:24-30 describes closure as "the figures are closed (unregistered from pyplot)". Reads fine, but for GUI/console users it's worth being explicit that this is why plot='off' from the console never steals focus (matching the console.py comment at console.py:661-663). Tiny prose tweak, not a blocker.

E. Layering: acceptable but worth marking

sigprocfunc/topoplot.py:7 now imports show_figures from popfunc.plot_utils. The precedent (sigprocfunc/eegplot.py:12) is real, so this is consistent — but the direction is against AGENTS.md ("sigprocfunc/ is low-level"). If you ever do the "move display helpers to a neutral module" cleanup, both eegplot.py and topoplot.py become one commit; worth putting on the backlog rather than in this PR.

F. pop_spectopo plot: str | bool = "on" guard — passing plot=True now, but keyword-only

Just to confirm: since plot is keyword-only in every wrapper's signature (declared after *args), plot=True/False/'on'/'off' cannot collide with positional args — that's correct. But the **kwargs capture would absorb plot='off' in wrappers that had no explicit plot= parameter. All 15 wrappers I checked declare plot: str | bool = "on" explicitly, so this is fine as-is. Just note that any future plot wrapper added elsewhere must remember to declare plot explicitly, or it'll silently no-op the flag.

---

Summary

• Blockers: none.
• One correctness/consistency polish (finding A) worth doing before merge: make pop_spectopo's plot handling actually match the "Python-keyword-only" contract by dropping the rebind.
• Tests + docstrings look good. The plot='off' closure semantics are now well-tested and documented.

I did not run the test suite or pre-commit.py on this pass either — code review only, per your request.
· Branch: feature/inna-conversion