[newtimef] Bring pop_newtimef time-frequency plots to EEGLAB parity

[innaamogolonova]

Port the single-condition pop_newtimef and newtimef ERSP/ITC workflow to EEGLAB parity: EEGLAB-matching decomposition numerics, two-sided baseline bootstrap significance, and the full image figure with marginal panels, tick values, scalp inset, phase-sign and phase-only coloring, pcontour outlines, and color limits. Adds a MATLAB parity test covering the ERSP, ITC, and p-value outputs, plus user documentation. Two-condition comparison stays out of scope.

[claude]

⚠️ Code review skipped: your organization's credit balance is too low for Code Review to run.

An organization admin can purchase credits or upgrade the plan in Plans & Billing.

Once credits are available, reopen this pull request to trigger a review.

[innaamogolonova]

🤖 Automated code review — generated by Claude Code at a maintainer's request. Not a human review.

Code review

• Overall assessment: Safe to merge after resolving one merge conflict. Strong, well-tested work.
• Highest-risk area: The significance/p-value numeric change and the timefreq.py decomposition fixes — verified independently, they hold up.
• Merge recommendation: Safe to merge once the changelog.rst conflict is resolved; nits below are optional.

What I ran (results)

Check	Result
test_phase4_timefreq_statistics.py (EEGPREP_SKIP_MATLAB=1)	66 passed, 4 skipped
test_console_workspace.py + test_menu_placeholder_inventory.py	100 passed
test_pac_time_frequency_helpers.py	8 passed
MATLAB parity test test_newtimef_matches_eeglab_ersp_itc_and_pvalues (against a real EEGLAB checkout)	PASSED — ERSP (dB) & complex ITC to 1e-6, freqs/times to 1e-9, two-sided p-values to 1e-12
Other MATLAB parity tests in the file (timefreq_helpers, ramberg_schmeiser)	PASSED
ruff check / ruff format --check / ty check on changed files	All clean

The MATLAB parity test ran EEGLAB's actual newtimef through the MATLAB engine and matched at the tolerances above — the deterministic ERSP/ITC and time/frequency parity is confirmed against real EEGLAB. (test_signalstat_statistics_match_eeglab in the same file fails locally, but that is unrelated to this PR — EEGLAB's signalstat.m calls skewness(), which needs MATLAB's Statistics & ML Toolbox, not licensed on this install.)

Blocking

None. One required housekeeping item:

• Merge conflict in docs/source/changelog.rst — the PR is CONFLICTING with develop. It's the only conflict and it's trivial (both branches prepended Unreleased entries). Rebase/resolve before merge; no code conflict.

Important

• The p-value parity test asserts against a hand-reimplemented compute_pvals, not EEGLAB's actual function. In test_newtimef_matches_eeglab_ersp_itc_and_pvalues, the engine.eval block reimplements the rank/fold formula inline. The docstring says it's "checked against EEGLAB's own compute_pvals," which overstates it. This is understandable — compute_pvals is a nested subfunction of newtimef.m and can't be called directly — so it's a pragmatic choice, not a defect. But the two-sided-for-ITC behavior is a central and somewhat surprising claim of this PR, and it rests on the reimplementation plus the manual mask-fraction check (0.052 vs 0.053), not an automated end-to-end comparison of the ITC significance mask (which can't match anyway, due to bootstrap RNG). Suggest tightening the docstring to something like "matches EEGLAB's compute_pvals formula (reimplemented inline; the function is a private subfunction of newtimef.m)" so the scope is honest. The ERSP P and complex R comparisons do call real EEGLAB newtimef at 1e-6 and were confirmed passing against a real EEGLAB checkout — that part is solid.

Nits

• phase_only auto-detection is decided from a single cell — newtimef.py _plot_time_frequency: abs(float(itc_magnitude.flat[0]) - 1.0) < 1e-4. Using only the (freq[0], time[0]) corner to flip the entire ITC panel into phase-degrees mode is fragile; wavelet edge effects make that corner the most likely place for a spurious near-unity value. Since the intent is to detect the single-trial all-ones case, np.allclose(itc_magnitude, 1.0) (or a trial-count check) expresses the intent robustly. Display-only — does not affect returned ersp/itc/significance.
• boottype is silently ignored — _bootstrap_power/_bootstrap_itc now do _ = boottype and always time-shuffle. That matches EEGLAB's 'shuffle' default, but a non-default boottype becomes a silent no-op. Given the PR raises NotImplementedError for other unsupported options (overlap), consider failing loudly on a non-default boottype for consistency.
• Benign matplotlib warning — UserWarning: Attempting to set identical low and high ylims fires from _draw_image_panel's imshow on small-freqs edge cases (e.g. test_pop_newtimef_forwards_color_limits). Harmless; a guard for freqs[0] == freqs[-1] in the extent would silence it.

Test gaps

• The negative-ntimesout subsample off-by-one is guarded only by a Python reimplementation, not MATLAB. test_timefreq_subsample_times_match_eeglab_colon encodes the EEGLAB colon grid by hand rather than comparing to MATLAB output — same pattern as the p-value note. This is the trickiest off-by-one in the PR (np.arange(start, stop, step) dropping the trailing point). Lower risk because newtimef's default path uses positive timesout (the eeg_lat2point path), which the MATLAB parity test does cover. Consider adding a negative-ntimesout case to the MATLAB parity test.

EEGLAB parity notes

• Removing frequency de-duplication is a cross-cutting output-shape change to timefreq, rippling into pac, newcrossf, and newtimef. Correctly traced to pac ((5,3,8) → (5,5,8), freqs2=[4,4,6,6,8]) and documented in the changelog. newcrossf tests in the same file still pass, so no silent breakage.
• plotphase default flip to 'on' in newtimef matches EEGLAB (g.plotphase='on'), and the GUI correctly injects plotphase='off' when the checkbox is unchecked. The former loud NotImplementedError is now a real implementation, and the test was updated to match.
• Two-sided compute_pvals for ITC is the right EEGLAB behavior and explains the ~alpha baseline false-positive scatter now reproduced. Worth eyeballing newtimef.m once more to confirm the R significance call uses the two-sided default, since it isn't asserted end-to-end.

---

_{🤖 Posted by Claude Code on behalf of @innaamogolonova. Findings are advisory; a human should confirm before merge.}

[innaamogolonova]

Addressing PR review:

Addressed

• Merge conflict in changelog.rst (blocking) — resolved by merging origin/develop (965608e). The Unreleased section is now a clean union of the newtimef bullets and develop's pop_prop/scalp-map entries. PR is MERGEABLE again.
• p-value test docstring overstates "EEGLAB's own compute_pvals" (important) — reworded in e500d87. The comment now says the two-sided p-value is checked against EEGLAB's compute_pvals formula, reimplemented inline (it's a private subfunction of newtimef.m and can't be called directly). The ERSP P / complex R comparisons still call real EEGLAB newtimef at 1e-6.
• Nit: phase_only decided from a single corner cell — fixed in e500d87. Now np.allclose(itc_magnitude, 1.0) (guarded on non-empty), which expresses the "all-ones single-trial" intent and can't misfire on a wavelet-edge cell. Display-only; no change to returned ersp/itc/significance.
• Nit: boottype silently ignored — fixed in f2b4a6d. newtimef now raises NotImplementedError for any boottype other than 'shuffle' (case-insensitive, matching EEGLAB's strcmpi), consistent with the overlap guard. 'rand'/'randall' build materially different nulls (newtimef.m:1282–1347), so failing loudly is the parity-safe behavior rather than returning a shuffle result under a different name. Removed the now-dead boottype plumbing from the private bootstrap helpers; added a guard test; changelog updated.
• Test gap: negative-ntimesout off-by-one had no MATLAB ground truth — added test_timefreq_negative_ntimesout_times_match_eeglab in c590f7d. It runs real EEGLAB timefreq with a negative ntimesout and compares times (no hand-encoded colon formula). frames=122, winsize=16, nsub=10 puts EEGLAB's colon endpoint exactly on the grid, so a trailing-point off-by-one would change the output length and fail. Passes against a real EEGLAB checkout.

Verified, no code change

• Two-sided compute_pvals for ITC — confirmed from source: newtimef.m:1382 calls compute_pvals(abs(R), abs(Rboottrials')) with no tail arg, and compute_pvals (line 2096) defaults tail='both' → 2*min(p, 1-p). EEGPrep's two-sided ITC p-values match this.
• Frequency de-dup removal and plotphase default flip — noted as correct; already traced to pac and covered by tests/changelog.

Deferred

• Nit: benign "identical ylims" UserWarning — left as-is. It fires from _draw_image_panel's imshow only when a decomposition yields a single output frequency (a degenerate test fixture, not a real ERSP/ITC analysis). It's a warning, not an error; the plot renders correctly and no numeric output/parity is affected. Guarding it would add defensive code for a near-impossible input. Happy to add the ~2-line extent guard if you'd prefer a warning-free run.

CI note

The two red checks — Build Documentation and Ruff and ty — are not from this PR. They're pre-existing develop-wide breakage from 11fb6f9 ("docs: add 11 tested user guide illustration scripts"): three example
files fail ruff format --check, and 11 gallery examples call Path(__file__), which sphinx-gallery executes without __file__ defined (NameError → the HTML build exits non-zero). The same two checks fail identically on #316–#318; merging develop here just surfaced them. The fix belongs in a small separate PR against develop (it would green-light all the stuck PRs at once), so it's intentionally not bundled into this parity PR. Neither check is conflict-blocking.

[innaamogolonova]

🤖 Automated re-review — generated by Claude Code at a maintainer's request, after the three follow-up commits. Not a human review.

Code review (re-review)

• Overall assessment: Safe to merge. All four findings from the prior review are addressed, verified, and tested.
• Highest-risk area: The significance/p-value numerics and timefreq decomposition — now confirmed against real EEGLAB, including the previously-untested negative-ntimesout subsample path.
• Merge recommendation: Safe to merge.

What changed since the last review

Three fix commits, on top of a develop merge that resolved the changelog conflict:

Prior finding (severity)	Fix commit	Verdict
p-value test docstring overstated "EEGLAB's own compute_pvals" (important)	e500d87a	✅ Now says "reimplemented inline … private subfunction of newtimef.m and cannot be called directly." Honest and accurate.
phase_only decided from a single corner cell (nit)	e500d87a	✅ Now np.allclose(itc_magnitude, 1.0) — whole-array check, correctly targets the single-trial all-ones case.
boottype silently ignored (nit)	f2b4a6db	✅ Now raises NotImplementedError for non-shuffle; param dropped from both bootstrap helpers; default is "shuffle", so default calls are unaffected.
Negative-ntimesout off-by-one guarded only by a hand-encoded colon (test gap)	c590f7df	✅ New @pytest.mark.matlab test ground-truths timefreq output times against real EEGLAB at 1e-9.
Changelog merge conflict (housekeeping)	965608ed	✅ develop merged; PR is now mergeable.

What I ran (results)

Check	Result
test_phase4_timefreq_statistics.py + test_pac_time_frequency_helpers.py (EEGPREP_SKIP_MATLAB=1)	75 passed, 5 skipped
test_newtimef_matches_eeglab_ersp_itc_and_pvalues (real EEGLAB)	PASSED
test_timefreq_negative_ntimesout_times_match_eeglab (new, real EEGLAB)	PASSED
ruff check / ruff format --check / ty check (changed files)	All clean
boottype caller audit (pop/GUI layer)	No caller forwards a non-shuffle boottype → the hard rejection breaks nothing internal

Blocking

None.

Important

None. The prior Important finding (p-value test scope) is resolved by the honest docstring — the two-sided p-value math still matches MATLAB exactly, now with accurate wording about how it is verified.

Nits

• Benign matplotlib warning persists — UserWarning: Attempting to set identical low and high ylims still fires from _draw_image_panel's imshow on small-freqs edge cases. Cosmetic; a freqs[0] == freqs[-1] guard on the extent would silence it. Not merge-blocking.

Test gaps

None outstanding. The negative-ntimesout path — the trickiest off-by-one in the PR — is now covered by a real EEGLAB comparison, and it passes.

EEGLAB parity notes

• boottype rejection is intentionally stricter than EEGLAB, which accepts 'rand'/'randall'. This is the right call for now: only the 'shuffle' null is implemented, and those alternatives build materially different nulls, so failing loudly (consistent with how overlap is handled) beats silently returning shuffle results. The changelog documents it.
• Two-sided compute_pvals for both ERSP and ITC remains validated by the deterministic parity test (ERSP/ITC to 1e-6) plus the inline-formula p-value check (to 1e-12), both passing against real EEGLAB.

---

Unrelated to this PR: any Build Docs / Ruff CI failure on this PR is inherited from develop (commit 11fb6f93, the gallery examples), not introduced here — this PR's own changed files are ruff/ty/format clean.

_{🤖 Posted by Claude Code on behalf of @innaamogolonova. Findings are advisory; a human should confirm before merge.}

[arnodelorme]

🤖 Review of this PR, generated by Claude Code at a maintainer's request.

Summary: the timefreq numerics, the compute_pvals port, the ITC null, and the figure work check out against the vendored EEGLAB sources. One blocking numerical issue: the ERSP significance null is not the bootstat shuffle permutation the changelog describes, and no parity test covers it. The current CI failures are unrelated (fixed on develop by #321); merging develop should turn the checks green.

Verified

• timefreq.py vs timefreq.m (all cited line ranges): FFT-path detrend is a no-op; wavelet detrend applies only to the single-channel branch; exact-zero bin guard; subitc returns the pre-subtraction ITC; requested frequencies are not de-duplicated; output times use eeg_lat2point with round-half-away-from-zero; the negative-ntimesout colon grid no longer has the trailing extra point.
• _baseline_pvalues vs compute_pvals: exact, including ties (MATLAB's stable sort rank equals searchsorted(..., side="right") + 1).
• ITC null: permuting each trial's baseline time course and recomputing ITC matches bootstat → shuffleonedim(arg1, 2) in distribution per frequency.
• Ran tests/test_phase4_timefreq_statistics.py and tests/test_pac_time_frequency_helpers.py on the branch: 75 passed, 5 MATLAB-skipped (no MATLAB engine locally). ruff check clean.

Blocking: ERSP null construction differs from EEGLAB

bootstat with 'boottype','shuffle', 'shuffledim' 2 (the default) calls shuffleonedim(arg1, 2), which permutes the baseline time axis independently for every (frequency, trial), and then applies mean(arg1,3). Each accumulated null value is therefore a trial-mean of independently permuted baseline samples, and ceil(naccu/n_base) iterations are pooled.

_bootstrap_power in this PR does the opposite order: it averages over trials first (baseline_stat) and then resamples the trial-mean baseline columns with replacement (_resample_baseline_times). Consequences:

1. The null only equals EEGLAB's when baseline power is independent across trials. Any trial-common baseline structure (drift, slow oscillation) inflates the PR's null spread relative to EEGLAB's.
2. The PR null has at most n_base distinct values per frequency (vs ~naccu in EEGLAB), which coarsens the p-values and affects FDR.

Quick simulation (1 freq, 60 baseline points, 30 trials, naccu=400, alpha=0.05; null in dB):

baseline	EEGLAB lower/upper	PR lower/upper	distinct null values (EEGLAB / PR)
iid noise	9.83 / 10.18	9.84 / 10.22	420 / 60
trial-common drift	9.92 / 10.37	9.42 / 10.71	420 / 60

The two new MATLAB parity tests check the deterministic ERSP/ITC values and the p-value formula on fixed arrays; neither exercises the null construction, so the changelog claim that "bootstrap thresholds ... closely match EEGLAB" is not yet demonstrated.

Suggested fix: in _bootstrap_power, permute boot_source along time per (frequency, trial), average over trials, apply _power_to_output, and accumulate ceil(naccu / n_base) iterations, mirroring bootstat. Better still, port the bootstat shuffle path once and use it for both ERSP and ITC. A parity test could seed MATLAB's RNG and compare the sorted null (or at least the thresholds within a loose tolerance) for a small case.

Non-blocking

• Changelog: eleven per-commit bullets for one feature, and they contradict each other (one says the images use jet, a later one says turbo). Please consolidate into two or three bullets describing the final behavior.
• PR description does not mention the pac output-shape change (frequency de-duplication removed) or that boottype other than 'shuffle' now raises.
• timefreq.py importing eeg_lat2point from popfunc mirrors EEGLAB's own dependency and creates no import cycle; fine.
• turbo is consistent with topoplot, erpimage, headplot, and spectopo in the repo, so the deliberate departure from EEGLAB's jet fits house style.

[innaamogolonova]

Updated PR description

Port the single-condition pop_newtimef / newtimef ERSP/ITC workflow to EEGLAB parity: EEGLAB-matching timefreq decomposition numerics, permutation-based baseline bootstrap significance (two-sided compute_pvals for both ERSP and ITC), and the full single-condition image figure — marginal panels with EEGLAB tick values, channel/component scalp inset, phase-sign and phase-only ITC coloring, pcontour outlines, and erspmax/itcmax color limits. Two-condition comparison (cell input, condstat, commonbase) stays out of scope for a follow-up PR.

Behavior changes to note

• pac output shape changes. timefreq no longer de-duplicates requested output frequencies, so freqs/nfreqs requests that snap several values onto the same FFT bin now return one frequency per request (as in EEGLAB) — e.g. pac output (5, 3, 8) → (5, 5, 8). Other timefreq numeric fixes: FFT-path detrend is a no-op, eeg_lat2point time-window centering, the negative-ntimesout subsample grid drops its spurious trailing point, subitc returns the pre-subtraction ITC, and exact-zero spectral bins are guarded.
• newtimef now rejects a non-shuffle boottype. Only EEGLAB's default 'shuffle' permutation null is implemented; 'rand'/'randall' build materially different nulls, so they raise NotImplementedError instead of being silently treated as 'shuffle'.

Significance

The ERSP/ITC bootstrap null is built EEGLAB's way — permute each trial's baseline time course, then average over trials (bootstat 'shuffle') — and each point's p-value folds to a two-sided tail (compute_pvals) for both ERSP and ITC.

Tests (MATLAB parity against a real EEGLAB checkout)

• ERSP (dB) & complex ITC to 1e-6, times/freqs to 1e-9, two-sided p-values to 1e-12.
• Negative-ntimesout output-time grid vs real EEGLAB timefreq.
• ERSP significance null spread vs real EEGLAB bootstat (permute-then-average), plus a fast non-degeneracy guard.

[innaamogolonova]

Comments addressed.

Blocking: ERSP significance null construction — fixed in 6f00f05. _bootstrap_power now mirrors bootstat's shuffle: it permutes each trial's baseline time course and then averages over trials, instead of averaging first and resampling the fixed trial-mean. Confirmed against real EEGLAB bootstat — the per-frequency null spread now matches (a new @pytest.mark.matlab test), plus a fast guard that the null is no longer degenerate (was capped at n_base distinct values). On the trial-common-drift case the old null was ~3.8× too wide; it now agrees with EEGLAB.

Changelog (contradictory per-commit bullets, jet vs turbo) — consolidated in 037034b: the 12 newtimef entries are now 3 bullets describing the final behavior (turbo only).

PR description — updated to call out the two behavior changes: the pac output-shape change from removing frequency de-duplication ((5,3,8) → (5,5,8)), and that a non-shuffle boottype now raises.

The eeg_lat2point import and the turbo house-style departure were noted as fine — no change. CI: the branch now carries the develop gallery-examples fix (#321), so the earlier Build Docs / Ruff reds are resolved.