Skip to content

[ICLabel] Complete viewprops diagnostics parity - #167

Merged
suraj-ranganath merged 2 commits into
feature/eeglab-full-standalone-completionfrom
phase/163-iclabel-viewprops-diagnostics
Jun 7, 2026
Merged

[ICLabel] Complete viewprops diagnostics parity#167
suraj-ranganath merged 2 commits into
feature/eeglab-full-standalone-completionfrom
phase/163-iclabel-viewprops-diagnostics

Conversation

@suraj-ranganath

@suraj-ranganath suraj-ranganath commented Jun 7, 2026

Copy link
Copy Markdown
Member

Add eeg_icalabelstat with EEGLAB-style threshold summaries, explicit standalone ICLabel network limits, and focused ICLabel/viewprops tests. Document the component review workflow and mark the Phase 6 parity rows implemented while preserving native EEGPrep diagnostic displays.

Limitations called out intentionally:

  • Standalone Python ICLabel ships only the bundled default netICL.mat; lite and beta remain explicit MATLAB/Octave-engine paths unless their artifacts are packaged later.
  • pop_iclabel intentionally has no dialog Help button because the ICLabel plugin inputdlg3 call does not pass a help callback; packaged pophelp text remains available.
  • eeg_icalabelstat keeps EEGLAB console-style print output by default, with verbose=False and stream= available for non-console callers.

Closes #163

@claude

claude Bot commented Jun 7, 2026

Copy link
Copy Markdown

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


Claude PR Review — #167 ICLabel viewprops diagnostics parity

  • Read the diff against base
  • Review eeg_icalabelstat.py (new function)
  • Review iclabel.py network-limit changes
  • Review pop_icflag.py / pop_iclabel.py changes
  • Check EEGLAB parity
  • Review tests
  • Post review feedback

Overall this is a clean, well-tested addition. The standalone network-limit guard, the torch-import deferral, and the eeg_icalabelstat summary all read well and are covered by focused tests. I found one likely regression and a couple of smaller notes.


🔴 Likely regression: removing help_text hides the pop_iclabel Help button

pop_iclabel.py drops help_text="pophelp('pop_iclabel')" from the dialog spec. The Qt renderer gates the Help button on help_text being truthy:

# src/eegprep/functions/guifunc/qt.py:296
if spec.help_text and spec.show_help_button:
    help_button = QtWidgets.QPushButton(spec.help_label)
    ...

With help_text now None, the Help button no longer renders in the ICLabel dialog. Note the original value was not broken — pophelp._normalise_function_name already unwraps pophelp('pop_iclabel')pop_iclabel (qt.py:1050 also falls back to spec.function_name), so help content resolved correctly before. The net effect of this change is simply that the button disappears, which is a parity loss vs. EEGLAB's dialog.

Two ways to resolve, depending on intent:

  • Keep the button (recommended): restore a truthy help_text (e.g. help_text="pophelp('pop_iclabel')", matching every other pop_* dialog in the repo), or
  • if the goal was to rely on function_name resolution, change the gate at qt.py:296 to (spec.help_text or spec.function_name) and spec.show_help_button so all dialogs benefit — but that's a broader change worth its own discussion.

Either way, please add a GUI test asserting the Help button is present for pop_iclabel, since nothing currently covers it.

Fix this →


🟡 Parity question: is eeg_icalabelstat an EEGLAB function or EEGPrep-native?

The ICLabel plugin isn't vendored under src/eegprep/eeglab/, so I couldn't diff against a MATLAB oracle. The name follows the eeg_* convention and the PR describes "EEGLAB-style threshold summaries," but I'm not aware of an eeg_icalabelstat.m in the upstream ICLabel plugin. If this is an EEGPrep-native diagnostic (which AGENTS.md permits), it'd be worth a one-line note in the module docstring/help saying so, rather than implying a 1:1 port — that keeps the parity matrix honest. If it does mirror an upstream function, please confirm the printed format matches (label text, capitalization, ordering) so it's a true port.

Minor on the format: the printed label uses IClabel class "..." — odd capitalization vs. the canonical ICLabel. Worth double-checking that's deliberate.

🟡 print() in a library function

eeg_icalabelstat._print_summary uses print(..., file=stream). AGENTS.md prefers logging over print outside scripts. This is borderline-acceptable as EEGLAB console-parity output, and the stream/verbose params make it controllable — just flagging for consistency. No change required if the console-mimicking behavior is intentional.

✅ Things that look good

  • iclabel._normalize_algorithm correctly validates default/lite/beta and the NotImplementedError for non-default networks under engine=None fires before the torch import — good ordering, well tested (test_python_iclabel_rejects_unbundled_alternate_networks_before_runtime_dependencies).
  • Deferring the torch import so MATLAB/Octave engines no longer require torch is a nice correctness fix.
  • _threshold_vector scalar/per-class handling and the [0,1] validation are solid; class-name fallback to the 7 standard classes is covered.
  • Calling eeg_icalabelstat in the pop_icflag GUI path is correctly guarded with not isinstance(EEG, list) and _require_iclabel.

I did not run the test suite or pre-commit in this review. Worth running uv run pytest tests/test_iclabel_statistics.py tests/test_gui_pop_iclabel.py plus a GUI smoke check of the Help button before merge.

@suraj-ranganath

suraj-ranganath commented Jun 7, 2026

Copy link
Copy Markdown
Member Author

🤖 Phase 6 ICLabel/viewprops visual parity evidence

iclabel_pop_prop_extended_dashboard_side_by_side.png pop_icflag_dialog_side_by_side.png
iclabel_pop_prop_extended_dashboard_side_by_side.png pop_icflag_dialog_side_by_side.png
pop_iclabel_dialog_side_by_side.png pop_viewprops_dialog_side_by_side.png
pop_iclabel_dialog_side_by_side.png pop_viewprops_dialog_side_by_side.png

@suraj-ranganath
suraj-ranganath merged commit 3688e68 into feature/eeglab-full-standalone-completion Jun 7, 2026
@suraj-ranganath
suraj-ranganath deleted the phase/163-iclabel-viewprops-diagnostics branch June 7, 2026 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant