[GUI] Add pop and File menu actions

[suraj-ranganath]

Add EEGPrep GUI wrappers and main-window dispatch for select, resample, run ICA, clean_rawdata, ICLabel, and File menu workflows. Implements File import/export, BIDS, STUDY, history, settings, and dataset load/save actions with tests, help/docs updates, visual parity captures, and regression coverage.

[suraj-ranganath]

🤖 Specification

Problem
EEGPrep's main GUI menus exposed several EEGLAB workflows as placeholders even though EEGPrep already had core processing implementations or plugin functions that users expect to reach from the main window. The missing GUI paths affected data selection, resampling, ICA decomposition, clean_rawdata, and ICLabel, and left users without consistent command-history strings or help resources for these workflows.

Approach
The branch adds EEGLAB-style pop wrappers and dialog specs for pop_runica, pop_iclabel, and pop_clean_rawdata, expands pop_select and pop_resample with GUI and return_com support, and routes the corresponding main-window menu actions to real wrappers. It also updates the Qt renderer for listbox controls, validation, and checkbox-driven enabled states, adds packaged help resources, and extends visual parity capture cases for the new dialogs.

Key code
The main dispatch path now treats the new actions as implemented wrappers instead of placeholders:

_MULTIPLE_DATASET_ACTIONS = {
    "pop_clean_rawdata",
    "pop_iclabel",
    "pop_reref",
    "pop_resample",
    "pop_runica",
    "pop_select",
}
...
elif name == "pop_clean_rawdata":
    from eegprep.plugins.clean_rawdata.pop_clean_rawdata import pop_clean_rawdata
    out = pop_clean_rawdata(selection, return_com=True)
elif name == "pop_resample":
    from eegprep.functions.popfunc.pop_resample import pop_resample
    out = pop_resample(selection, return_com=True)
elif name == "pop_runica":
    from eegprep.functions.popfunc.pop_runica import pop_runica
    out = pop_runica(selection, return_com=True)
elif name == "pop_select":
    from eegprep.functions.popfunc.pop_select import pop_select
    out = pop_select(selection, return_com=True)

Tests
Focused GUI and menu coverage validates dialog specs, picker metadata, dispatch routing, wrapper history strings, invalid input handling, optional EEG field handling, multiple datasets, colon range parsing, and clean_windows integration after pop_select succeeds. Visual parity tooling now has capture cases for pop_select, pop_resample, pop_runica, pop_iclabel, and pop_clean_rawdata. Local verification passed with EEGPREP_SKIP_MATLAB=1 uv run --no-sync pytest -m "not slow" -q and the pre-push focused test set.

[claude]

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

---

Code review

• Overall assessment: Needs changes — a few user-visible issues in the new pop wrappers (history round-trip, dead GUI control). No blocker for the underlying processing.
• Highest-risk area: Round-trippability of history strings for pop_select, pop_runica, pop_clean_rawdata (1-based vs 0-based; cell vs array syntax).
• Merge recommendation: Needs changes.

Blocking

None.

Important

1. pop_select GUI emits 0-based numeric channels in history
src/eegprep/functions/popfunc/pop_select.py:664-666 subtracts 1 from each int channel returned by the dialog so it can be fed to eeg_decodechan (which is 0-based). The same options dict is then handed to _history_command at line 740, so a user typing 1 2 3 in the dialog produces EEG = pop_select( EEG, 'channel', [0 1 2]);. Replayed in EEGLAB MATLAB (1-based), this selects the wrong channels — or errors on 0. Suggested fix: keep options 1-based and convert only at the call site to eeg_decodechan, or build the history off the raw GUI text before subtracting.

2. pop_runica GUI emits 0-based chanind in history
src/eegprep/functions/popfunc/pop_runica.py:143 calls _parse_channel_text(..., one_based=True) which subtracts 1 (line 235); the resulting list is then serialised by _history_command at line 259-260. EEGLAB stores 1-based chanind in history. Same fix as above — convert at the engine boundary, not at parse time.

3. pop_clean_rawdata "Pop up scrolling data window" checkbox is dead UI
src/eegprep/plugins/clean_rawdata/pop_clean_rawdata.py:144 declares tag="vis" but _run_gui (lines 149-185) never reads result.get("vis"). EEGLAB pops up vis_artifacts post-cleaning. Suggested fix: either wire it up (call into the scroll viewer when checked) or drop the checkbox to avoid promising behavior that doesn't happen.

4. pop_clean_rawdata history serialises string lists with the wrong MATLAB syntax
src/eegprep/plugins/clean_rawdata/pop_clean_rawdata.py:251-252 always wraps lists/tuples/arrays in [ ... ]. For Channels / Channels_ignore (lists of strings), EEGLAB expects cell-array syntax {'Fz' 'Cz'}, and [Fz Cz] is invalid MATLAB. Mirror pop_select._history_value (lines 747-748) and emit {...} when every item is a string.

Nits

• src/eegprep/plugins/ICLabel/pop_iclabel.py:72 — getattr(weights, "size", len(weights) if hasattr(weights, "__len__") else 0) == 0 is over-engineered and conflicts with AGENTS.md ("No ad-hoc compatibility hacks such as hasattr(...)"). np.asarray(weights).size == 0 is enough.
• _parse_key_value_args, _parse_numeric_text, _parse_text_tokens, _history_value are now copy-pasted across pop_select.py, pop_runica.py, pop_clean_rawdata.py (and pop_reref.py). A functions/popfunc/_pop_utils.py would let history-format fixes land in one place.

Test gaps

• No test asserts that the GUI path for pop_select produces a 1-based history string when the user enters numeric channel indices. Adding a case to tests/test_gui_pop_select.py against a fake renderer that returns "1 2 3" for the chans tag would have caught finding 1.
• No test exercises the vis checkbox in tests/test_gui_pop_clean_rawdata.py. A renderer stub that returns vis=True would currently observe no side effect — which is exactly the gap to flag.
• No test for pop_runica GUI numeric chanind round-trip in tests/test_gui_pop_runica.py.

EEGLAB parity notes

• pop_runica: only icatype='runica' is supported; the dialog also drops EEGLAB's "Concatenate datasets" checkbox and the 'interrupt','on' default in history. Already documented as a limitation via NotImplementedError — fine as a follow-up, but worth tracking.
• pop_resample (pre-existing in develop, now reachable from the main menu): resample_eeg assumes 2D (nbchan, pnts) and won't handle epoched (nbchan, pnts, trials) data; EEG['icaact'] is not resampled; boundary events are not honored when resampling continuous data with gaps. Not introduced by this PR, but the new GUI dispatch makes them user-reachable — consider a follow-up issue.
• Help markdown (pop_runica.md, pop_clean_rawdata.md, pop_iclabel.md, pop_resample.md) is much shorter than what EEGLAB pophelp() shows from the MATLAB headers (every option enumerated). For GUI Help-button parity, expanding these would close the gap users will notice immediately.

Inline comments were also posted on the four important findings.

Checked: correctness/regressions in changed code, EEGLAB parity against vendored MATLAB sources, EEG dict semantics, test coverage for changed GUI paths, AGENTS.md compliance.

---

• Pre-review gate
• Find repository instructions
• Summarize the PR
• Run 4 parallel review agents
• Validate candidate issues
• Post summary review comment
• Post inline comments
  · branch feat/more-pop-functions

[suraj-ranganath]

🤖 GUI parity attachments

Side-by-side EEGLAB vs EEGPrep screenshots for GUI components added in this PR

Contact sheet:

pop_select dialog:

pop_resample dialog:

pop_runica dialog:

pop_iclabel dialog:

pop_clean_rawdata dialog:

Local source artifacts are under .visual-parity/, but they are intentionally not committed to the repo.

[suraj-ranganath]

🤖 Addressed the automated review findings in 6ef2558.

Changes made:

• Kept GUI-entered numeric pop_select channels 1-based in history while converting to 0-based only for the Python apply path.
• Kept GUI-entered numeric pop_runica chanind 1-based in history while converting to 0-based only before running ICA.
• Made the clean_rawdata vis checkbox explicit: it now notifies users that the rejected-data scrolling viewer is not yet available in EEGPrep instead of silently doing nothing.
• Fixed clean_rawdata history formatting for string channel lists so MATLAB-style cell arrays are emitted.
• Simplified ICLabel ICA presence checking with np.asarray(weights).size.

Added focused regressions for each user-visible issue above.

Local verification after the fix:

• EEGPREP_SKIP_MATLAB=1 uv run --no-sync pytest tests/test_gui_pop_select.py tests/test_gui_pop_runica.py tests/test_gui_pop_clean_rawdata.py -q -> 19 passed
• EEGPREP_SKIP_MATLAB=1 uv run --no-sync pytest tests/test_gui_pop_select.py tests/test_gui_pop_resample.py tests/test_gui_pop_runica.py tests/test_gui_pop_iclabel.py tests/test_gui_pop_clean_rawdata.py tests/test_gui_main_window.py tests/test_visual_parity.py tests/test_package_exports.py tests/test_clean_windows.py -q -> 106 passed
• EEGPREP_SKIP_MATLAB=1 uv run --no-sync pytest -m "not slow" -q -> 865 passed, 218 skipped, 11 deselected
• ./pre-commit.py --fix on staged files -> OK

[suraj-ranganath]

@claude review this

[claude]

Claude finished @suraj-ranganath's task in 6m 0s —— View job

---

Re-review of fix commit 6ef2558

• Verify the four important fixes from the prior review
• Re-scan changed files for new issues
• Confirm tests cover the regression cases
• Post review summary

Overall assessment: All four important findings from the prior review are properly fixed and covered by focused regression tests. Ready to merge once the small follow-up nits below are addressed (or explicitly deferred).

Merge recommendation: Approve with optional cleanups.

Verified fixes from the prior review

1. pop_select GUI history is now 1-based — src/eegprep/functions/popfunc/pop_select.py:669-679 introduces _gui_options_for_apply() that performs the 1→0-based subtraction only for the apply path, while the options dict fed to _history_command keeps 1-based ints. Verified by tests/test_gui_pop_select.py:109-128: GUI input "1 2 3" selects Fz/Cz/HEOG and produces EEG = pop_select( EEG, 'channel', [1 2 3]);. ✓

2. pop_runica GUI history is now 1-based — src/eegprep/functions/popfunc/pop_runica.py:150-159 adds _gui_chanind_for_apply(), _run_gui no longer subtracts 1 (line 146). Verified by tests/test_gui_pop_runica.py:69-93: GUI input "1 2" results in EEG['icachansind']=[0, 1] and history 'chanind', [1 2]. ✓

3. pop_clean_rawdata vis checkbox is no longer dead — src/eegprep/plugins/clean_rawdata/pop_clean_rawdata.py:195 records the checkbox state via the sentinel _SHOW_VIS_ARTIFACTS_KEY, then _notify_vis_artifacts_unavailable() (lines 276-290) shows a "viewer not yet available in EEGPrep" Qt info dialog (or logs a warning when there is no Qt app). Verified by tests/test_gui_pop_clean_rawdata.py:87-127 (mock asserts _notify_vis_artifacts_unavailable is called once and the sentinel never leaks into history). ✓

4. pop_clean_rawdata cell-array history for string lists — src/eegprep/plugins/clean_rawdata/pop_clean_rawdata.py:262-265 now mirrors pop_select._history_value: [Cz Pz] is replaced with {'Cz' 'Pz'} when every item is a string. Verified by tests/test_gui_pop_clean_rawdata.py:129-144. ✓

Nit fix: pop_iclabel._require_ica simplified to np.asarray(weights).size == 0 per AGENTS.md (src/eegprep/plugins/ICLabel/pop_iclabel.py:74). ✓

New issues / suggestions

Nit — dead one_based keyword left in _parse_channel_text (src/eegprep/functions/popfunc/pop_runica.py:247-251)
After the fix, every caller invokes _parse_channel_text(text) (no kwarg). The one_based=False branch on line 249 is now dead code introduced by this PR. AGENTS.md says "Remove imports, variables, functions, and files that your change made unused", so the keyword + branch should be dropped:

def _parse_channel_text(text):
    return [_parse_scalar(value) for value in re.split(r"[\s,]+", text.strip().strip("[]")) if value]

Fix this →

Nit — _pop_utils.py extraction (re-flagging from prior review)
_parse_key_value_args, _parse_text_tokens, and _history_value are still copy-pasted across pop_select.py, pop_runica.py, pop_clean_rawdata.py, pop_reref.py, and (partially) pop_adjustevents.py. The prior review suggested a shared functions/popfunc/_pop_utils.py; this PR added another copy and the next history-format fix will need to be applied in N places again. Worth a follow-up PR.

UX nit — vis checkbox defaults to checked (src/eegprep/plugins/clean_rawdata/pop_clean_rawdata.py:154)
With value=True, every GUI run of pop_clean_rawdata triggers the "viewer not yet available" popup. EEGLAB defaults this to checked because the viewer actually exists there. Until vis_artifacts is ported, defaulting to value=False would be friendlier to users. Pre-existing parity choice — flag for the author rather than a blocker.

Test coverage

• tests/test_gui_pop_select.py:109 covers the 1-based history regression for numeric channel input.
• tests/test_gui_pop_runica.py:69 covers the 1-based chanind history regression.
• tests/test_gui_pop_clean_rawdata.py:87 covers the vis checkbox notification path.
• tests/test_gui_pop_clean_rawdata.py:129 covers cell-array string-list history.
• tests/test_gui_main_window.py:546 parametrises new dispatch routing across all five new actions.

Test status

Locally ran the touched GUI tests via uv run --no-sync pytest (PySide6 is unavailable in the runner so 4 GUI-render tests are properly skipped):

• tests/test_gui_pop_select.py tests/test_gui_pop_runica.py tests/test_gui_pop_clean_rawdata.py → 19 passed
• tests/test_gui_pop_iclabel.py tests/test_gui_pop_resample.py tests/test_gui_main_window.py → 40 passed, 4 skipped (PySide6)

Did not run the full -m "not slow" suite — the author already reported 865 passed, 218 skipped, 11 deselected after the fix commit.

Carried-over follow-ups (not introduced by this PR, but now user-reachable)

• pop_resample/resample_eeg only handles 2D continuous data, doesn't touch icaact, and doesn't honor boundary events. Now reachable from the main menu — worth tracking as a follow-up issue.
• pop_runica only supports icatype='runica' (other algorithms raise NotImplementedError); the dialog also drops EEGLAB's "Concatenate datasets" checkbox and the default 'interrupt','on' history flag. Already documented as a limitation — fine as a follow-up.
• Help markdown (pop_runica.md, pop_clean_rawdata.md, pop_iclabel.md, pop_resample.md) is still much shorter than EEGLAB's pophelp text. Expanding these will close the parity gap users will notice from the GUI Help button.
  · branch feat/more-pop-functions

[suraj-ranganath]

🤖 Addressed the latest review nits in 496c1a7.

Fixed:

• Removed the now-dead one_based keyword and branch from pop_runica._parse_channel_text.
• Defaulted the clean_rawdata rejected-data viewer checkbox to unchecked until EEGPrep has a real vis_artifacts viewer.
• Added a regression assertion for the clean_rawdata vis default.

I did not extract _pop_utils.py in this PR because that would touch older pop_reref / pop_adjustevents history formatting code and broaden the change beyond these GUI action fixes. I agree it is worth a follow-up refactor.

Local verification:

• EEGPREP_SKIP_MATLAB=1 uv run --no-sync pytest tests/test_gui_pop_runica.py tests/test_gui_pop_clean_rawdata.py -q -> 12 passed
• EEGPREP_SKIP_MATLAB=1 uv run --no-sync pytest tests/test_gui_pop_select.py tests/test_gui_pop_resample.py tests/test_gui_pop_runica.py tests/test_gui_pop_iclabel.py tests/test_gui_pop_clean_rawdata.py tests/test_gui_main_window.py tests/test_visual_parity.py tests/test_package_exports.py tests/test_clean_windows.py -q -> 106 passed
• ./pre-commit.py --fix on staged files -> OK

[suraj-ranganath]

🤖 Added the _pop_utils.py extraction in 296dccc.

What changed:

• Added shared parse_key_value_args, parse_text_tokens, and format_history_value helpers under src/eegprep/functions/popfunc/_pop_utils.py.
• Refactored pop_select, pop_runica, pop_clean_rawdata, pop_adjustevents, and pop_reref to use the shared helpers while preserving their existing pop-specific history syntax.
• Added direct coverage in tests/test_pop_utils.py for key parsing, token parsing, and EEGLAB-style history value formatting.

Verification:

• EEGPREP_SKIP_MATLAB=1 uv run --no-sync pytest tests/test_pop_utils.py tests/test_gui_pop_select.py tests/test_gui_pop_runica.py tests/test_gui_pop_clean_rawdata.py tests/test_pop_adjustevents.py tests/test_pop_reref.py tests/test_gui_pop_reref.py -q -> 49 passed, 43 skipped
• EEGPREP_SKIP_MATLAB=1 uv run --no-sync pytest tests/test_pop_utils.py tests/test_gui_pop_select.py tests/test_gui_pop_resample.py tests/test_gui_pop_runica.py tests/test_gui_pop_iclabel.py tests/test_gui_pop_clean_rawdata.py tests/test_gui_main_window.py tests/test_visual_parity.py tests/test_package_exports.py tests/test_clean_windows.py tests/test_pop_adjustevents.py tests/test_pop_reref.py tests/test_gui_pop_reref.py -q -> 136 passed, 43 skipped
• EEGPREP_SKIP_MATLAB=1 uv run --no-sync pytest -m "not slow" -q -> 871 passed, 218 skipped, 11 deselected
• ./pre-commit.py --fix on staged files -> OK

[suraj-ranganath]

@claude review this

[claude]

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

---

Re-review covering the _pop_utils.py extraction (296dccc) and File-menu commit (76b7e8b)

• Inspect the latest commits and full diff vs origin/develop
• Verify the four important review findings from the prior review still hold
• Audit the new _pop_utils.py extraction
• Review the new File menu actions commit (~30 new files, ~2 200 added lines)
• Check tests and CI status (Tests + Docs jobs green on the latest push)
• Post review summary

Overall assessment: The original GUI dispatch work + the _pop_utils.py refactor are in solid shape. The newly added 76b7e8b "Implement File menu actions" is a substantial second feature (30 new files, ~2 200 LOC) that was layered on top of an existing review and was not mentioned in the PR description. None of the issues below are blockers, but several would land cleanly as small follow-up commits before merge.

Merge recommendation: Approve once the important items below are addressed (or explicitly deferred to a follow-up PR).

---

Verified — original review findings (commits 6ef2558 / 496c1a7)

1. pop_select GUI numeric channels → 1-based history via _gui_options_for_apply (src/eegprep/functions/popfunc/pop_select.py:674-684). Verified by tests/test_gui_pop_select.py:109. ✓
2. pop_runica GUI numeric chanind → 1-based history via _gui_chanind_for_apply (src/eegprep/functions/popfunc/pop_runica.py:151-160). Verified by tests/test_gui_pop_runica.py:69. ✓
3. pop_clean_rawdata vis checkbox is no longer a no-op (src/eegprep/plugins/clean_rawdata/pop_clean_rawdata.py:200, 247-261) and defaults to unchecked. Verified by tests/test_gui_pop_clean_rawdata.py:88-128. ✓
4. pop_clean_rawdata string channel lists emit {'Cz' 'Pz'} cell syntax via format_history_value (src/eegprep/functions/popfunc/_pop_utils.py:87-100). Verified by tests/test_gui_pop_clean_rawdata.py:130-145. ✓

---

_pop_utils.py extraction (296dccc) — looks good

parse_key_value_args / parse_text_tokens / format_history_value cleanly capture the variations used by pop_select, pop_runica, pop_clean_rawdata, pop_adjustevents, and pop_reref via small knobs (bool_style, cell_for_sequence, string_separator, empty_sequence, none_as_empty, dict_formatter, number_formatter). Each call site keeps its EEGLAB-specific syntax. Covered by tests/test_pop_utils.py and the existing per-pop history regressions. No issues found.

---

Newly added File-menu commit (76b7e8b) — issues worth addressing

Note: the PR description ("Add EEGPrep GUI wrappers and main-window dispatch for select, resample, run ICA, clean_rawdata, and ICLabel") does not mention this commit. Consider expanding the PR body so reviewers and the changelog reflect the broader scope, or splitting the File menu work into its own PR.

Important

1. pop_runscript silently swallows .m and .txt scripts — src/eegprep/functions/popfunc/pop_runscript.py:13-17

if path.suffix.lower() == ".py":
    exec(path.read_text(encoding="utf-8"), exec_globals)
elif path.suffix.lower() not in {".m", ".txt"}:
    raise ValueError("History scripts must be .py, .m, or .txt files")
return f"LASTCOM = pop_runscript('{path}');"

For .m/.txt the function does nothing yet still returns a success-shaped history string and is reachable via the Tools → Run script menu. Either run the script, or raise NotImplementedError so the user knows EEGPrep can only run .py scripts today. Also note exec(...) is being run on a user-picked file — fine because the user picked it, but worth a one-line docstring warning.

Fix this →

2. Cross-module import of a private helper _channel_labels — src/eegprep/functions/popfunc/_file_io.py:315 is imported by both pop_export.py:11 and plugins/EEG_BIDS/pop_exportbids.py:10:

from eegprep.functions.popfunc._file_io import _channel_labels

The underscore prefix conventionally means "internal to this module". Either drop the underscore (e.g., channel_labels) since it's now part of the file_io helper API, or duplicate the small 13-line function into each call site. Keep _file_io.py module-private even if specific helpers become public.

3. History strings interpolate paths without escaping — affects almost every new pop function:

• pop_biosig.py:14, pop_fileio.py:32, pop_writeeeg.py:21, pop_importbids.py:25, pop_exportbids.py:38, pop_importerplab.py:34, pop_importpres.py:34, pop_loadstudy.py:17, pop_runscript.py:17, pop_saveh.py:25, bids_tools.py (and pop_resample._history_command for non-path values is fine).

Example:

command = f"EEG = pop_fileio('{path}');"

If the path contains a single quote, the resulting MATLAB command is malformed and cannot be replayed. Use the existing format_history_value(str(path)) helper which already handles ' escaping (_pop_utils.py:73-74). Same applies to filenames passed straight into f-strings.

4. pop_chanevent drops pre-existing urevent — src/eegprep/functions/popfunc/pop_chanevent.py:53

out["urevent"] = [dict(event) for event in out["event"]]

EEGLAB's pop_chanevent.m preserves the existing urevent and extends it. The current Python port wipes it whenever the channel produces any new events, even with 'delevent', 'off'. Worth aligning before users start relying on this path.

5. pop_clean_rawdata _run_gui always returns a sentinel even when GUI was cancelled — looks correct on second pass (_run_gui returns None when result is None). No action needed; noting it because it was easy to misread.

Nits

• pop_runica._parse_channel_text (pop_runica.py:234-235): now correctly free of the dead one_based kwarg.
• pop_export.py:23-24 raises NotImplementedError for expr= but the menu doesn't expose this option — fine, but the help text under resources/help/pop_export.md (if added later) should note the gap.
• pop_savestudy._history_command is split into filename/filepath kwargs even when caller passed a single path. EEGLAB usually emits pop_savestudy(STUDY, EEG, 'filename', name, 'filepath', dir) so this matches. Just confirming.
• pop_resample.py carries an old commented-out test_pop_resample_local() and __main__ block with a hard-coded /Users/arno/Python/... path (pop_resample.py:410-431). Not introduced by this PR; would be nice to clean up while the file is being touched.
• pop_chanevent.py:35 error message says "chan indices must be 1-based and within EEG.nbchan" — the dispatcher already constrains the QInputDialog to a min of 1, so this branch is reachable only via programmatic calls. Fine.
• _pop_utils.py is named with a leading underscore yet imported from many files. Since it's already a stable shared module, dropping the underscore (or keeping it but making sure __all__/typed docstring is explicit) would be more idiomatic. Optional.

Test gaps

• No test exercises pop_runscript with a .m file — the silent-noop path in finding 1 is uncovered.
• No test asserts that history strings emitted by the new pop functions are round-trippable when the user's filename contains a single quote. Adding one regression per affected pop_* would catch the escaping issue in finding 3 cheaply.
• test_file_menu_pop_functions.py is the main integration test for the new File-menu pop modules. Several wrappers — pop_importerplab, pop_importpres, pop_writeeeg, pop_biosig — have no direct unit test, only the chained call through pop_fileio / pop_importevent. Worth one focused test each.

---

CI status

• Tests workflow on the latest push: success (run id 25859459451).
• Documentation Build and Deploy: success (run id 25859459461).
• I did not re-run the full local suite. The author reported EEGPREP_SKIP_MATLAB=1 uv run --no-sync pytest -m "not slow" -q → 871 passed, 218 skipped, 11 deselected after 296dccc, which matches CI green.

---

Carried-over follow-ups (not introduced by this PR)

• pop_resample/resample_eeg: 2D-only, no icaact, doesn't honor boundary events. Now reachable from menu + multi-dataset selection — worth a follow-up issue.
• pop_runica: only icatype='runica'; dialog drops "Concatenate datasets" and 'interrupt','on' from history.
• Help markdown (pop_runica.md, pop_clean_rawdata.md, pop_iclabel.md, pop_resample.md) is still much shorter than EEGLAB's pophelp text.

Checked: regressions against the prior review, EEGLAB parity for the new File-menu pop functions, EEG dict semantics, AGENTS.md compliance (cross-module private import, dead kwargs, history round-trippability), test coverage. No blocking changes.
· branch feat/more-pop-functions

[suraj-ranganath]

🤖 GUI parity attachments update

Added user-attachment side-by-side screenshots for the GUI surfaces currently added by this PR, including the newly implemented File menu and the pop-function dialogs. These are GitHub user attachments and are not committed to the repo.

Side-by-side EEGLAB vs EEGPrep screenshots

Contact sheet:

File menu:

pop_select dialog:

pop_resample dialog:

pop_runica dialog:

pop_iclabel dialog:

pop_clean_rawdata dialog:

Local source artifacts are under .visual-parity/, including .visual-parity/more_pop_file_menu_contact_sheet.png, and remain untracked.

[suraj-ranganath]

@claude review this

[claude]

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

---

Re-review covering the File-menu fixes (655cbfc) and the resample/ICA parity work (b6f90aa)

• Inspect new commits since last review
• Verify earlier findings still hold
• Review changes in detail
• Run focused tests
• Check CI status
• Post review summary

Overall assessment: All previously flagged findings are addressed and well-tested. The new resample/ICA parity work (b6f90aa) is a meaningful step forward — proper segment-based continuous resampling, epoched event remapping, multi-algorithm ICA dispatch (picard, runamica15), and dataset concatenation. A few small parity/UX issues are worth a quick look before merge.

Merge recommendation: Approve. The items below are small enough to either land in a follow-up commit on this branch or be deferred to a follow-up PR.

---

Verified — previous review findings

File-menu commit 655cbfc:

1. pop_runscript now raises NotImplementedError for .m/.txt instead of silently returning a success-shaped history string — src/eegprep/functions/popfunc/pop_runscript.py:21-22, covered by tests/test_file_menu_pop_functions.py:181-189 (test_pop_runscript_rejects_matlab_and_text_scripts). ✓
2. _channel_labels renamed to public channel_labels and imports updated in pop_export.py and pop_exportbids.py. ✓
3. History paths are now escaped via format_history_value(...) everywhere they were string-interpolated: pop_biosig, pop_fileio, pop_writeeeg, pop_importerplab, pop_importpres, pop_saveh, pop_loadstudy, pop_exportbids, pop_importbids, pop_runscript, pop_expevents, plus bids_tools._update_bids_metadata now emits the actual metadata fields. Regression tests assert _matlab_string(...) for paths containing ' in tests/test_file_menu_pop_functions.py:178, 195, 226, 244, 268, 282. ✓
4. pop_chanevent now preserves existing urevent and extends it via _events_with_existing_urevents/_events_with_new_urevents, with explicit coverage at tests/test_file_menu_pop_functions.py:113-128. ✓
5. Stale test_pop_resample_local() + __main__ block with the hard-coded path was removed from pop_resample.py. ✓

Earlier round (still holding):

• pop_select/pop_runica GUI numeric channel input is 1-based in history; _gui_options_for_apply / _gui_chanind_for_apply only convert at the call site. ✓
• pop_clean_rawdata vis checkbox is no longer dead (notifies users that the viewer is not ported yet) and defaults to unchecked. ✓
• pop_clean_rawdata string lists emit {'Cz' 'Pz'} via shared format_history_value. ✓

---

b6f90aa — resample/ICA parity (substantive rewrite)

resample_eeg is properly rewritten:

• Continuous data is now segmented on EEGLAB boundary events (_segment_bounds honours boundary types and option_boundary99 -99 codes), resampled segment-by-segment, then re-concatenated; event/urevent latencies are remapped per-segment, with the half-sample boundary convention (latency % 1 == 0.5) preserved via indices[segment_index] - 0.5. (src/eegprep/functions/popfunc/pop_resample.py:155-297)
• Epoched 3-D data is now handled: _resample_epoched_events rebases each event into its epoch, urevent is cleared (matches EEGLAB), and output["icaact"] is invalidated.
• setname gets the EEGLAB-style resampled suffix and saved="no" is set.

pop_runica is significantly broadened (no longer NotImplementedError for non-runica):

• picard (via eeg_picard) and runamica15/amica (via eeg_amica) are routed in _run_ica_backend.
• The dialog now lists 5 algorithms and adds concatenate/concatcond checkboxes when called with list[EEG].
• _runica_on_datasets dispatches between _runica_by_subject_session (concatcond) and _runica_concatenated (concatenate) and single-dataset runs.
• _prepare_ica_dataset saves prior ICA into etc.oldica* and drops etc.ic_classification before recomputing — nice EEGLAB-style touch covered by tests/test_gui_pop_runica.py:test_existing_ica_is_saved_and_iclabel_removed_before_recompute.

Help markdown for pop_runica.md, pop_resample.md, pop_iclabel.md, pop_clean_rawdata.md was expanded (now 34–52 lines each) and addresses the prior parity gap with pophelp().

---

New issues found in b6f90aa

Important (worth resolving on this branch)

1. _runica_by_subject_session raises on datasets missing subject — src/eegprep/functions/popfunc/pop_runica.py:227-229

The GUI default for concatcond is True for multi-dataset calls (line 138). When users select multiple datasets in the menu without a subject field set and click OK, this raises ValueError("Subject names are required to concatenate datasets by subject and session"). EEGLAB silently treats subject-less datasets as a single group. Either treat missing subjects as one group (e.g., group by (subject or "", session)) or fall back to per-dataset ICA when there are no subjects, instead of erroring.

Fix this →

2. _run_ica_backend injects lrate=0.001 default for runica — src/eegprep/functions/popfunc/pop_runica.py:300-302

if icatype == "runica":
    runica_options = dict(options)
    runica_options.setdefault("lrate", 0.001)

EEGLAB's runica.m auto-picks lrate = 0.00065 / log(chans^2) when not set by the user. Forcing 0.001 here overrides that derivation for every caller that doesn't pass lrate explicitly. Either drop the default (let eeg_runica apply EEGLAB's formula) or implement the chan-based formula. Easier fix: remove the setdefault.

Fix this →

Nits

3. _finalize_ica_dataset(output, output) is a no-op call — src/eegprep/functions/popfunc/pop_runica.py:190

In the chanind branch, both args are the same dict, so the function's copy-back loop does nothing. Either drop the call or refactor _finalize_ica_dataset so the chanind branch passes the prepared dict separately to preserve the saved-ICA copy-back symmetry.

4. _segment_bounds uses round(latency + 0.5) where math.ceil reads more clearly — src/eegprep/functions/popfunc/pop_resample.py:210-212

The semantics are equivalent for the realistic input range (positive latencies with the half-sample boundary convention). int(math.ceil(latency)) would express the intent more directly.

5. pop_chanevent with delevent="on" keeps old urevent entries — src/eegprep/functions/popfunc/pop_chanevent.py:48-58

EEGLAB's pop_chanevent.m with 'delevent','on' typically rebuilds urevent to match the new event list. Preserving the prior urevents may be reasonable, but the new events' urevent pointers reference indices in the combined list — those pointers and the residual orphan entries can confuse downstream code that walks the urevent table. Worth adding a test for delevent="on" or aligning with EEGLAB.

6. pop_runica dataset argument is consumed but never exposed via the dialog — src/eegprep/functions/popfunc/pop_runica.py:47

dataset= is parsed and forwarded to _dataset_indices to subset which entries get ICA. That's fine for programmatic calls. The dialog doesn't expose a "datasets to use" listbox, though, so GUI users always run ICA on every dataset. EEGLAB's dialog has a dataset list — flag for future parity if it's worth chasing.

---

Test status

Locally with uv run --no-sync pytest:

• tests/test_pop_resample_python.py tests/test_gui_pop_runica.py tests/test_gui_pop_clean_rawdata.py tests/test_file_menu_pop_functions.py → 38 passed
• tests/test_pop_utils.py tests/test_gui_pop_select.py tests/test_gui_pop_resample.py tests/test_gui_pop_iclabel.py tests/test_gui_main_window.py tests/test_visual_parity.py → 76 passed, 5 skipped (PySide6 unavailable)

I did not re-run the full -m "not slow" suite; the author reported it green after b6f90aa.

CI status

• Tests workflow on the latest push: ✅ success (run id 25879543084)
• Documentation Build and Deploy: ✅ success (run id 25879543081)

---

Carried-over follow-ups (not blocking)

• Help markdown is now substantially expanded. The remaining EEGLAB pophelp text contains MATLAB-only details — closes the parity gap for users.
• _pop_utils.py shared helper extraction is done.
• pop_resample boundary-event handling and ICA reactivation (icaact cleared) are now correct for both continuous and epoched data — the prior follow-up note is largely resolved.

Checked: regressions against prior reviews, EEGLAB parity for resample/ICA paths, EEG dict semantics, AGENTS.md compliance (no dead kwargs, no compatibility hacks, history round-trippability), test coverage. No blocking changes.
· branch feat/more-pop-functions

[suraj-ranganath]

@claude review this

[claude]

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

---

Re-review covering commits 390ec56, b30df90, and a754b4d

• Identify new commits since prior review (b6f90aa)
• Verify earlier findings still hold
• Review new commits in detail
• Run focused tests (66 + 15 + 38 passed locally; PySide6-only cases skipped)
• Check CI status (Docs ✅; Tests still in progress at review time)
• Post review summary

Overall assessment: All previously flagged issues are now addressed. The new commits cleanly resolve the prior important findings (lrate=0.001 default, _subject_session_key requiring subjects, _finalize_ica_dataset(output, output) no-op, _segment_bounds ceil) and add a useful dataset-selection control to the runica dialog. Two genuine bug fixes — pop_chanevent/pop_importevent urevent handling, and pop_saveh history ordering — also land here with focused regression coverage. Nothing blocking.

Merge recommendation: Approve.

---

Verified — previous review findings

From the b6f90aa re-review (now resolved):

1. _runica_by_subject_session no longer raises on subject-less datasets — src/eegprep/functions/popfunc/pop_runica.py:242-257 uses _subject_session_key which falls back to "" for missing subject/session. Verified by tests/test_gui_pop_runica.py:215-228 (test_concatcond_groups_datasets_without_subjects). ✓
2. _run_ica_backend no longer injects lrate=0.001 for runica — src/eegprep/functions/popfunc/pop_runica.py:319-321 now just calls eeg_runica(EEG, sortcomps=reorder, **options). Verified by test_runica_uses_default_lrate_when_unset assertion self.assertNotIn("lrate", runica.call_args.kwargs) in tests/test_gui_pop_runica.py:73, 182. ✓
3. _finalize_ica_dataset(output, prepared) is now meaningful — pop_runica.py:189-208 builds output = copy.deepcopy(prepared) after running the chanind subset, then calls _finalize_ica_dataset(output, prepared) so the oldica* etc keys saved on prepared propagate symmetrically with the non-chanind path. ✓
4. _segment_bounds uses math.ceil — pop_resample.py:208-211. ✓
5. pop_chanevent rebuilds urevent when delevent='on' — pop_chanevent.py:43-56 now wipes both event and urevent and rebuilds urevent indices from scratch via _events_with_new_urevents(events, 0). Covered by tests/test_file_menu_pop_functions.py:135-148 (test_pop_chanevent_rebuilds_urevents_when_replacing_events). ✓ This matches EEGLAB's pop_chanevent.m (EEG.event = []; EEG.urevent = []; when delevent='on').
6. pop_runica's dataset listbox — _runica dialog now opens with a "Datasets to use for ICA decomposition" listbox (pop_runica.py:141-148), backed by the new multi-select listbox support in qt.py:325-343, 416-418, 700-701. Validation refuses an empty selection. Verified by test_gui_dialog_spec_adds_concatenate_controls_for_multiple_datasets, test_gui_dataset_selection_routes_to_dataset_argument, test_qt_renderer_reads_multiselect_listbox_as_one_based_rows, and test_qt_renderer_defaults_to_all_selected_dataset_rows. ✓

---

New commits — detailed review

390ec56 "Address ICA and event review fixes": focused fix commit that lands the four resolved items above. Clean.

b30df90 "Add dataset selection to runica dialog":

• Adds a multi-select listbox via a new _MULTI_SELECT_PROPERTY sentinel (qt.py:24). The reader uses widget.property(_MULTI_SELECT_PROPERTY) to switch between single-row and selectedIndexes. Sequence-typed value (list/tuple/set) triggers ExtendedSelection mode and pre-selects given rows. Help markdown is updated to document the new 'dataset' keyword and the new dialog control.
• _history_command now emits 'dataset', [n m] ahead of the rest. EEGLAB's pop_runica accepts 'dataset', datasets_idx as a key/value pair, so this round-trips.
• Minor: the dialog grows to (824, 454) and cases.json is updated accordingly.

a754b4d "Fix file menu review regressions":

• pop_saveh no longer reverses allcoms before writing (pop_saveh.py:20). EEGPrep's add_history() appends to ALLCOM so it is chronological (oldest-first); the old reversed(allcoms) produced reverse-chronological output. The new behavior matches a user-readable replay script. Verified by the updated assertion in tests/test_file_menu_pop_functions.py:191.
• pop_importevent now uses the same _events_with_existing_urevents / _events_with_new_urevents helpers as pop_chanevent to keep urevent consistent across append='on'/append='off'. Covered by tests/test_file_menu_pop_functions.py:84-91.
• pop_runscript dispatcher now propagates CURRENTSET from the script's namespace back into the session via a new _currentset_list helper (menu_actions.py:743-753). Covered by tests/test_gui_main_window.py:708-725.
• pop_fileio_brainvision_mat moves from IMPLEMENTED_ACTIONS to PLACEHOLDER_ACTIONS (menu_actions.py:39, menu_placeholders.py:42). The previous "implemented" entry routed to pop_fileio which doesn't actually parse the BrainVision MATLAB export — calling it as a placeholder is more honest. The new menu test (test_file_menu_actions_are_implemented_or_explicit_placeholders) pins this expectation.

---

Small follow-up nits (optional, non-blocking)

• _currentset_list boolean fall-through (menu_actions.py:743-753): int/float truthy values pass through to the final int(value) branch instead of returning early. Currently produces correct output for all realistic inputs, but tightening the structure (isinstance(value, bool) short-circuit, or simply if isinstance(value, (int, float)): return [int(value)] if int(value) > 0 else []) would be a touch clearer.
• _finalize_ica_dataset etc-key copy is a partial no-op when output = deepcopy(prepared): the for key in [...] loop never adds new keys because output_etc already contains them. The function still serves a purpose (popping ic_classification), but the loop is dead in the chanind path. Could be simplified to just the ic_classification pop. Cosmetic.
• _dataset_indices accepts an empty list and returns no-op output (pop_runica.py:358-367): a programmatic caller passing dataset=[] will silently get the original list back. Defensible (Qt validation guards the GUI path), but worth a ValueError("dataset must contain at least one index") if the API ever gains public callers.

---

Test status

Local uv run pytest:

• tests/test_gui_pop_runica.py → 15 passed, 1 skipped (PySide6)
• tests/test_gui_pop_clean_rawdata.py tests/test_file_menu_pop_functions.py tests/test_pop_resample_python.py tests/test_gui_main_window.py → 66 passed, 5 skipped (PySide6)
• tests/test_pop_utils.py tests/test_gui_pop_select.py tests/test_gui_pop_resample.py tests/test_gui_pop_iclabel.py tests/test_visual_parity.py → 38 passed

CI status (at review time)

• Tests workflow on the latest push: ⏳ in progress (run id 25908201454)
• Documentation Build and Deploy: ✅ success (run id 25908201460)

If the Tests run goes green this is ready to merge.

---

Carried-over follow-ups (not introduced by this PR)

• pop_resample/resample_eeg: segment-based continuous resampling + epoched event remapping are now correct; the only remaining gap is whether downstream consumers reuse icaact (now invalidated and recomputed, matching EEGLAB).
• pop_runica: still raises NotImplementedError for ICA backends outside runica/picard/runamica15/amica. The dialog now exposes 5 algorithm presets and a dataset selector, closing most of the prior parity gap.
• Help markdown for pop_runica.md, pop_clean_rawdata.md, pop_iclabel.md, pop_resample.md is in good shape (40–50 lines each).
• Shared _pop_utils.py helpers are in place; future history-formatting fixes land in one place.

Checked: regressions against prior reviews, EEGLAB parity for runica dataset selection and event/urevent semantics, EEG dict semantics, AGENTS.md compliance, test coverage. No blocking changes.
· branch feat/more-pop-functions

[arnodelorme]

@claude can you check backward compatibility of this pull request

[claude]

Claude finished @arnodelorme's task in 5m 10s —— View job

---

Backward compatibility audit — feat/more-pop-functions vs origin/develop

• Inspect the full diff vs origin/develop (65 files, +5846/-351)
• Identify modified public APIs (signatures, return values, behavior)
• Identify modified GUI/menu surfaces
• Identify modified data shapes (EEG dict fields, history strings)
• Check for renames / removed symbols
• Summarise backward-compatibility risk

Overall: No public-symbol removals, no public-symbol renames, and no return-value contract breaks at the eegprep.X package level. All call-site changes are additive and the existing kwarg-based callers continue to work. There are a few behavior changes at the data-shape level that are EEGLAB-parity improvements but could surprise existing consumers; none of them are blockers, but they are worth calling out in a changelog entry.

---

Public API surface

Package exports (src/eegprep/__init__.py) — pure additions to _LAZY_EXPORTS. No removed or renamed entries. New: pop_biosig, pop_chanevent, pop_clean_rawdata, pop_expevents, pop_expica, pop_export, pop_exportbids, pop_fileio, pop_iclabel, pop_importbids, pop_importdata, pop_importepoch, pop_importerplab, pop_importevent, pop_importpres, pop_loadstudy, pop_runica, pop_runscript, pop_saveh, pop_savestudy, pop_study, pop_studyerp, pop_studywizard, pop_writeeeg. ✓

pop_select signature (src/eegprep/functions/popfunc/pop_select.py:18)

• Old: pop_select(EEG, **kwargs)
• New: pop_select(EEG, *args, gui=None, renderer=None, return_com=False, **kwargs)
• Kwarg call sites unchanged (pop_select(EEG, channel=[…]), pop_select(EEG, notrial=[…]), etc.) — all in-tree callers in clean_artifacts.py, clean_channels*.py, clean_flatlines.py, clean_windows.py, bids_preproc.py, pop_epoch.py, eegobj.py continue to work.
• pop_epoch.py:293-294 already handled tuple returns, so return_com=False default is fine.
• Behavior change: pop_select(EEG) with valid data and no kwargs/positional args now launches the Qt dialog (because gui=None resolves to True when options is empty). Previously this was a no-op. No in-tree caller does this; external scripts relying on no-op behavior would now block on a dialog. Easy mitigation: pass gui=False.
• New capability (additive): MATLAB-style positional 'key', value args via *args, list-of-datasets input, and return_com=True.

pop_resample signature (src/eegprep/functions/popfunc/pop_resample.py:21)

• Old: pop_resample(EEG, freq, engine=None)
• New: pop_resample(EEG, freq=None, engine=None, *, gui=None, renderer=None, return_com=False, fc=None, df=None)
• pop_resample(EEG, 250), pop_resample(EEG, 250, engine='scipy'), pop_resample(EEG, 250, engine='matlab') all still work and still return a single EEG dict by default. ✓
• Behavior change: pop_resample(EEG) with no freq now launches the dialog (was: would error trying to use None as a rate). Strictly more permissive.
• Output-side changes (EEGLAB-parity fixes; visible to downstream consumers):
  • EEG['icaact'] is now cleared to np.array([]) (old code left a stale array → silent correctness bug)
  • EEG['setname'] gets the resampled suffix
  • EEG['saved'] = "no"
  • Continuous data is now segmented on boundary events before resampling (old code resampled across boundaries)
  • 3-D epoched data now supported (old code would crash)
  • Event/urevent latencies remapped per segment with half-sample boundary convention preserved
  • These match EEGLAB but break value-equality assertions on setname / icaact / event latencies if any external caller relies on the prior (incorrect) shape.

pop_reref / pop_adjustevents — internal refactor only. Imports and call sites unchanged; history-string output identical. ✓

clean_windows (src/eegprep/plugins/clean_rawdata/clean_windows.py:208)

• Behavior change: the pop_select success path now drops event, urevent, epoch, icaact, reject, stats, specdata, specicaact and resets pnts/xmax. Previously these were left to whatever pop_select had done. Effect: any downstream code that read EEG['event'] after clean_windows returned will now see an empty list. Functions in EEGLAB do not preserve event tables across clean_windows, so this matches the reference, but it is a visible regression for anyone who relied on the prior Python-side behavior.

pop_chanevent / pop_importevent — both are new modules, so no backward-compat surface of their own. They do rebuild EEG['urevent'] from scratch when delevent='on'. (No prior Python implementation to break.)

channel_labels (src/eegprep/functions/popfunc/_file_io.py:315) — renamed from _channel_labels to public during the PR. Used internally by pop_export and pop_exportbids; no external symbol was ever exposed. ✓

---

GUI / menu surface

• IMPLEMENTED_ACTIONS in menu_actions.py gains 32 entries; PLACEHOLDER_ACTIONS loses the matching ones. All previously placeholder menu items now route to real implementations — purely additive from a user perspective.
• One exception: pop_fileio_brainvision_mat moved from IMPLEMENTED_ACTIONS to PLACEHOLDER_ACTIONS (menu_actions.py:39, menu_placeholders.py:42). The previous "implemented" entry routed to pop_fileio, which doesn't actually parse the BrainVision MATLAB export — the new placeholder is more honest, but technically the menu button's behavior changed.
• ICLabel/menu.py: added ica:on to userdata for the three ICLabel items. EEGLAB-style: these menu entries now require an ICA decomposition to be enabled. If any test/script asserted these items were always enabled, that assumption breaks.
• qt.py: new listbox, toggle_enabled, multi-select listbox callbacks, and _MULTI_SELECT_PROPERTY sentinel. New rendering capabilities — additive.

---

Test surface

• tests/conftest.py adds the new GUI test files to GUI_FILE_SUFFIXES so they get marker-categorised correctly. Existing test classification unchanged. ✓
• No existing test was modified to relax tolerances or weaken assertions.
• tests/test_pop_resample.py:22 still passes engine='poly' and expects MATLAB parity; routing through the rewritten path was kept compatible with 'poly' and 'scipy'.

---

Risks worth flagging in a changelog entry

1. pop_resample now mutates setname / clears icaact / sets saved='no' — EEGLAB-correct, but a value-shape change for any downstream code that compares the output dict to the input.
2. pop_resample now segments continuous data on boundary events — datasets with boundary events will produce numerically different samples around the boundaries than they did on develop.
3. clean_windows now drops event/urevent/epoch/icaact on the pop_select success path — matches EEGLAB but changes the in-Python behavior.
4. pop_select(EEG) and pop_resample(EEG) with no other args now open a GUI dialog — headless scripts that called these as a no-op would block. Mitigation: pass gui=False or explicit args.
5. pop_fileio_brainvision_mat is now a placeholder rather than a no-op router — menu invocations will show the placeholder dialog instead of silently doing nothing.

None of these are blockers; they're either EEGLAB-parity fixes or pre-existing silent bugs being corrected. Recommend documenting points 1–4 in the release notes so users porting scripts off prior develop are not surprised.
· branch feat/more-pop-functions