Phase 5: close user-facing option gaps

[suraj-ranganath]

Close option-level gaps in existing user-facing EEGLAB-style functions, including export expressions, filename-backed epoching, editset file paths, comperp and erpimage controls, headplot setup previews, runica selection, and ASR calibration options. Tightens review feedback around path errors, expression sandboxing, std display, and distance validation.

Fixes #136
Part of #131

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[suraj-ranganath]

@claude review this

[claude]

Claude finished @suraj-ranganath's task in 4m 9s —— View job

---

PR Review — Phase 5: close user-facing option gaps

• Gather PR diff and changed files
• Review pop_* function changes for EEGLAB parity
• Review clean_rawdata / ASR calibration changes
• Review docs and help resources
• Review tests
• Post final review

---

Summary

Solid, well-scoped phase that lands real standalone implementations for previously-deferred options (export expr, filename-backed epoch data, editset file paths, comperp display/significance toggles, erpimage event sorting, headplot preview/orilocs) and re-classifies the truly out-of-scope ones (legacy usefft, MATLAB workspace expressions, MATLAB/text history replay, unported ICA backends, FieldTrip coregistration, full Riemannian ASR) as explicit ValueErrors with clear messages. Parity matrix, audit, help resources, and tests are updated consistently. The change keeps EEGLAB parity policy (standalone runtime, no EEGLAB checkout dependency) intact.

Below are findings ordered by significance.

---

1. pop_export expr sandbox — well done, but document the positional-only constraint

src/eegprep/functions/popfunc/pop_export.py 90–185

The AST-based validator is a thoughtful sandbox: it walks every node, restricts attribute access to a fixed whitelist of np.<name>, blocks __class__ / __dict__ escapes (verified by tracing the walker for x.__class__ and np.__class__), uses a SimpleNamespace shim so accidental access to other numpy attributes fails cleanly, and clears __builtins__. Good defense-in-depth.

Two small follow-ups:

• _validate_expression_call rejects all node.keywords, which means np.clip(x, a_min=0, a_max=1) fails. The help (resources/help/pop_export.md) lists clip without mentioning the positional-only restriction; users will hit a confusing error. Either document "positional arguments only" or relax for the whitelisted functions.
• Consider clamping ** to prevent silly DoS (e.g., x ** 999999999 allocating a huge result). Low priority — a user would only foot-gun themselves locally.

Improve expr docs/restrictions →

2. pop_editset — typos on file paths get a misleading error

src/eegprep/functions/popfunc/pop_editset.py 296–308, 247–258, 270–283

_existing_path returns None for any non-existing path, and the caller then raises "cannot evaluate MATLAB workspace expressions for data". A user who simply typoed raw_dta.txt gets a message about MATLAB workspace expressions, which is misdirecting. Suggest distinguishing "looks like a file path (has extension / separator) but does not exist" from "looks like a MATLAB identifier" so the error names the right problem:

def _looks_like_path(text: str) -> bool:
    return any(ch in text for ch in "/\\.") or text.endswith((".set", ".fdt", ".mat", ".txt", ".csv"))

…then raise a "file not found" error in that branch.

Fix this →

3. pop_editset — dataformat is consumed only for data, not for ICA matrices

src/eegprep/functions/popfunc/pop_editset.py 79–82, 270–293

The dataformat= option is threaded only to _assign_data. _load_matrix_file calls infer_dataformat(path) with no explicit override, so a user supplying "dataformat", "ascii" together with binary-ish file extensions for icaweights/icasphere/icachansind will silently use the inferred format. Either thread dataformat through _assign_ica_matrix/_load_array_path too, or document the limitation in pop_editset.md.

4. pop_comperp._onoff_option — operator precedence and _is_default_off for 0

src/eegprep/functions/popfunc/pop_comperp.py 400–411

if value is None or _is_default_off(value) and default is False:

The Python precedence is value is None or (_is_default_off(value) and default is False) which is what's intended, but readers will misread it. Parenthesize for readability.

Also, _is_default_off(0) returns False (because 0 is False is False), so addavg=0 becomes "off" rather than the "default" path. Not a behavioral bug given the call sites, but it's the kind of thing the next contributor will trip over.

5. pop_comperp._plot_std — std semantics may diverge from EEGLAB

src/eegprep/functions/popfunc/pop_comperp.py 444–449

row_means = np.nanmean(stack, axis=1)   # collapse channels first
center = np.nanmean(row_means, axis=0)  # average across datasets
spread = np.nanstd(row_means, axis=0, ddof=1 if stack.shape[0] > 1 else 0)

This is std of the channel-averaged ERP across datasets. EEGLAB's comperp std handling in plottopo/erpimage style overlays computes std across channels (per dataset) for the "addstd" trace, not std across datasets of channel-averaged signals. Worth checking against the EEGLAB MATLAB implementation under src/eegprep/eeglab/functions/popfunc/pop_comperp.m (it forwards to plottopo which uses the per-channel std). If the goal is EEGLAB-faithful display, this likely needs a tweak; otherwise document that the std band is across datasets.

6. clean_artifacts.py — silent fallback for unknown Distance values

src/eegprep/plugins/clean_rawdata/clean_artifacts.py 233–246

distance = str(Distance).strip().lower()
useriemannian = 'calib' if distance not in {'euclidian', 'euclidean'} else None

This mirrors EEGLAB's ~strcmpi(Distance, 'euclidean') semantics, so it's intentional, but a typo like Distance='euclidiann' silently flips on the Riemannian-calib path. Adding an explicit {'euclidian', 'euclidean', 'riemannian'} allowlist (raising on others) would catch typos while staying parity-compatible for documented values. Optional.

7. pop_runica — subtle history-emission change

src/eegprep/functions/popfunc/pop_runica.py 47–50

has_programmatic_options = bool(parsed)
icatype = _normalise_icatype(parsed.pop("icatype", icatype))
has_programmatic_options = has_programmatic_options or icatype != "runica"

Adding the second line means a call like pop_runica(EEG, icatype='picard') (with the keyword living on the function signature rather than parsed) now suppresses the GUI even when the user only set icatype. That's a behavior change relative to the prior commit. The test test_unported_ica_algorithm_fails_clearly confirms pop_runica(EEG, icatype='fastica') now reaches the backend dispatcher (instead of opening a GUI then failing). That's plausibly what's wanted, but it would be worth a note in pop_runica.md so users understand "non-default icatype = headless" is the new default.

8. pop_epoch._load_filename_backed_data — shape inference is layout-specific

src/eegprep/functions/popfunc/pop_epoch.py 628–648

if array.ndim == 2 and pnts > 0 and trials > 1 and array.shape[1] == pnts * trials:
    array = array.reshape(array.shape[0], trials, pnts).transpose(0, 2, 1)

This silently reinterprets a (nbchan, pnts*trials) ASCII file as channel-major, trial-fastest ordering. That matches EEGLAB .fdt layout but not necessarily ASCII files saved by user scripts. Worth a one-line note in pop_epoch.md about the assumed flatten order so a researcher loading hand-saved ASCII doesn't get silently wrong epoch boundaries.

9. pop_erpimage._event_sort_values — latency-ms is computed when not used

src/eegprep/functions/popfunc/pop_erpimage.py 419–430

Minor: _event_latency_ms is called every iteration even when both window.size == 0 and field_name != "latency". Cheap, but trivially hoistable.

Also the renorm check is restricted to {"", "no", "yes"}. EEGLAB allows arbitrary MATLAB formulas; flagging this as an explicit non-goal in the help (already done) is good — make sure the error message ("renorm formulas are not available") points users to the supported yes/no values, which it does.

10. Tests — good coverage, one nit

• test_pop_comperp_supports_display_options_and_significance scales the same data by different factors, so the paired t-test always has near-zero variance noise and the p-values are guaranteed finite. That's fine to exercise the plumbing, but it doesn't validate the significance highlighting logic for borderline cases. Consider adding one explicit test that asserts at least one shaded region appears (or doesn't) given known alpha.
• test_filename_backed_data_is_loaded_for_epoching is a nice replacement for the old "not implemented" test.
• test_pop_editset_loads_data_chanlocs_and_ica_from_files exercises the full file-path matrix — exactly the right shape.

Nits

• src/eegprep/functions/sigprocfunc/headplot.py 130: return type Path | Figure is honest but a bit awkward. Could split into a separate headplot_setup_preview function — but only if you want stricter signatures. The current shape mirrors EEGLAB's overload, which is the parity-preserving choice.
• .notes/implementation-notes.html: the new Phase 5 block is appended above the pre-existing Phase 2 heading, leaving two <h1> titles in one document. Reorder so the page-level <title> and <h1> agree, or wrap Phase 2 under an <h2>.

Verdict

Approve in spirit — code is on-altitude, tests track behavior, docs and the parity matrix are kept in sync. None of the findings above are blockers; (1) (sandbox docs), (2) (file-not-found error clarity), and (5) (_plot_std semantics vs. EEGLAB) are the most worth addressing before merge. (5) is the only one with potential parity-divergence implications and is worth at least confirming against pop_comperp.m.
• Branch: phase/5-user-facing-options