[Fable 5 Findings] Phase 4 — Architecture: kill duplication, fix ownership & decompose sprawl

[suraj-ranganath]

🤖 Auto-generated from the thermo-nuclear whole-codebase code-quality review at commit 8ba958e (tip of PR #190). Part of the Fable 5 Findings epic #193.

Rationale

The same contract implemented two or more times, often already drifted — the single loudest maintainability theme in the review. Highlights: the CLI implements the transform pipeline twice with divergent validation; is_on() and bootstrap/threshold/FDR helpers are reimplemented across timefreq and statistics; layering inversions (k-means numerics imported upward from a pop wrapper; firfilt's canonical firws owned by clean_rawdata's private/); and a few modules that grew past ~1000 lines because distinct concepts were never split.

How to approach

Remedy direction: one canonical helper imported downward, parallel branches collapsed into a single explicit flow, and large modules split by stable ownership (numerics vs history/dialog glue) — not by arbitrary line count. None of these are behavioral bugs today, but each is a place where the next fix has to be made twice or silently lands in only one copy.

Findings in this phase (45)

Every item below was confirmed by an independent adversarial verification pass against the working tree. Check items off as they land. Full per-finding detail (with verifier notes and full evidence) is in THERMO_NUCLEAR_REVIEW.md on the review branch.

1. Parallel winrej/rejection-family plumbing duplicated between _eegplot_rejection.py and pop_eegplot.py

• functions/popfunc/_eegplot_rejection.py (lines 193-368)
  • Why: _eegplot_rejection.py and pop_eegplot.py independently reimplement the same EEGBrowser rejection scaffolding: _row_count (identical bodies), _pad_rows vs _row_marks (same zero-pad-to-(row_count,trials) logic), _displayed_families vs _displayed_rejection_families, _has_family_marks vs _has_rejection_family, _manual_color/_family_color vs _manual_color/_reject_color, _as_winrej_rows, and the family/…
  • Failure mode: Adding or renaming a rejection family, or changing how reject*E row masks are padded, must be edited in both files; a miss desyncs the epoched browser (pop_eegthresh et al via _eegplot_rejection) from pop_eegplot's own browser, so the same dataset shows different superposed marks depending on entry point.
  • Fix: Hoist the shared helpers (_row_count, row-mask padding, displayed-families, color lookup, winrej-row coercion) and the family/color constants into one module (pop_eegplot.py already exports the constants) and import them in _eegplot_rejecti…
  • Evidence: _eegplot_rejection.py:312-316 _row_count is identical to pop_eegplot.py:311-315; _eegplot_rejection.py:300-309 _row_marks duplicates pop_eegplot.py:299-308 _pad_rows; family-detection pairs _has…

2. Boundary-event detection forked into 5+ helpers with divergent sentinels; is_boundary_event uses -1, others -99

• functions/popfunc/_event_utils.py (lines 93-98 (used at pop_rmdat.py:123, pop_selectevent.py:241))
  • Why: The canonical port eeg_findboundaries (eeg_findboundaries.py:48-60) matches EEGLAB: string startswith('boundary') OR numeric -99 gated on option_boundary99. At least five independent reimplementations exist with drifting semantics: _event_utils.is_boundary_event (numeric ==-1, exact 'boundary'), eeg_eegrej._is_boundary_event (numeric ==-99 ungated, exact 'boundary'), pop_resample._is_boundary_even…
  • Failure mode: is_boundary_event treats numeric type -1 as a boundary (a value no EEGLAB convention uses) and does NOT recognize numeric -99 boundaries. For datasets imported with option_boundary99 numeric boundaries: pop_rmdat (_event_windows) will not clamp time windows at boundaries, so removed windows can span discontinuities; pop_selectevent with deleteevents='on' wil…
  • Fix: Delete is_boundary_event and the other ad-hoc copies; route all boundary checks through eeg_findboundaries (which pop_epoch already wraps as bool(eeg_findboundaries(EEG=[event]))).
  • Evidence: _event_utils.py:96 ... float(event_type) == -1: return True; eeg_findboundaries.py:58 ev.get('type') == -99; pop_resample.py:236 ... event_type == -99.

3. is_on() reimplemented ~9 times across modules with genuinely divergent semantics

• functions/popfunc/_pop_utils.py (lines 99-105 (canonical) vs copies listed in evidence)
  • Why: _pop_utils.is_on (line 99) is the canonical EEGLAB on/off normalizer, yet at least nine modules define a private _is_on instead of importing it, and they do NOT agree. pop_runica:598 and pop_export:243 use str(value).lower() in {...} (so _is_on([1]) and _is_on(np.array([1])) become False, whereas canonical is_on returns True via bool(value)); pop_loadset:183 and pop_newset:250 drop t…
  • Failure mode: A maintainer reading a GUI value through _is_on in one module gets different truthiness than another module for list/array/unknown-string inputs, so identical history/GUI values can be interpreted as on in one pop function and off in another - a latent correctness bug that is invisible until an ndarray or non-canonical string reaches the predicate.
  • Fix: Delete the private _is_on/_is_empty copies and import is_on/is_empty_value from _pop_utils. Because sigprocfunc/timefreqfunc should not import upward from popfunc, relocate these tiny canonical predicates to a lower shared module…
  • Evidence: newcrossf:417 return str(value).lower() not in {"0", "false", "off", "no", "none"} (inverted) vs pop_runica:599 return str(value).lower() in {"on", "yes", "true", "1"} vs canonical _pop_utils.py:9…

4. Two divergent component_activations implementations (popfunc._rejection vs popfunc._plot_utils) compute ICA activations differently

• functions/popfunc/_rejection.py (lines 111-121)
  • Why: _rejection.component_activations (lines 111-121) always recomputes from weights@sphere using order='F' reshape and never consults stored EEG['icaact']. _plot_utils.component_activations returns stored icaact when present, otherwise recomputes with C-order reshape and additionally validates icachansind ranges. These are two parallel canonical implementations of 'get component activations' that disa…
  • Failure mode: A dataset whose stored icaact was sign-flipped or reordered (e.g. by the posact path) yields different activations from the two code paths: rejection scoring recomputes-from-weights while pop_prop trusts stored icaact. Score/visual disagreement and divergent rejection marks for the same dataset, hard to diagnose because both functions share the same name.
  • Fix: Consolidate on the canonical sigprocfunc/popfunc._plot_utils helper (which already powers eeg_getica) and have _rejection.rejection_data call it, or document explicitly why rejection must always recompute and ignore stored icaact. Either wa…
  • Evidence: _rejection.py:120 activations = finite_matmul(finite_matmul(weights, sphere), data_2d[icachansind]) (always recompute, order='F'); _plot_utils.py:99-105 icaact = EEG.get('icaact'); if icaact is not…

5. sortcomps and posact blocks are triplicated verbatim across eeg_runica.py, eeg_amica.py, eeg_picard.py

• functions/popfunc/eeg_runica.py (lines 52-110)
  • Why: The component-variance sort (variance_metric = sum(icawinv^2,axis=0)*sum(icaact^2,axis=1); windex = argsort[::-1]; reorder icaact/icaweights/icawinv) and the sign-normalization loop are copy-pasted into eeg_runica.py (52-85), eeg_amica.py (99-133), and eeg_picard.py (75-108) with only the pinv import differing. This is the canonical ICA-field post-processing logic living in three places, so a fix…
  • Failure mode: A correctness fix or behavior change applied to one backend silently diverges from the others. The posact invariant bug already exists identically in two of the three copies, demonstrating the drift cost concretely.
  • Fix: Extract one shared helper (e.g. in ica_utils.py) finalize_ica_fields(EEG, *, sortcomps, posact) operating on icaweights/icasphere/icawinv/icaact, and call it from all three eeg* backends. Fix the posact invariant in that single place.
  • Evidence: eeg_runica.py:57 variance_metric = np.sum(EEG['icawinv'] ** 2, axis=0) * np.sum(icaact_2d**2, axis=1) is byte-identical to eeg_amica.py:104 and eeg_picard.py:80; the whole for r in range(ncomps): i…

6. pop_comperp reimplements canonical is_on as private _is_on

• functions/popfunc/pop_comperp.py (lines 408-413)
  • Why: Same canonical helper duplicated again: _is_on (l.408-413) duplicates _pop_utils.is_on (l.99-105) and is used heavily in _plot_comperp (l.249-266) and _onoff_option (l.405). Three private copies of the same on/off parser now exist across this scope (here, pop_erpimage, plus the canonical _pop_utils).
  • Failure mode: Divergence risk across the on/off semantics that gate every add/sub/diff curve in the comperp plot; a fix to one copy won't propagate.
  • Fix: Import is_on from _pop_utils and remove the local _is_on (l.408-413).
  • Evidence: def _is_on(value: Any) -> bool: ... return value.strip().lower() in {"on", "yes", "true", "1"} (l.408-410).

7. Lowpass filtering applied via six repeated lowpass* calls with duplicated srate lookup

• functions/popfunc/pop_comperp.py (lines 65-82, 304-315)
  • Why: When a lowpass cutoff is given, the same float(datasets[int(add_indices[0])].get('srate', 1) or 1) srate expression is recomputed six times (l.67,69,71,73,76,80) and _lowpass_erp/_lowpass_stack (l.304-315) are near-identical (same butter design, differ only in filter axis 1 vs 2). The whole block could collapse to one srate variable and one axis-parameterized helper applied across the erp/stack…
  • Failure mode: Six copies of the cutoff/srate guard and filter design are easy to update inconsistently (e.g. change filter order in one path), and the verbose block obscures that all six operations are the same filtfilt with two axis choices.
  • Fix: Compute srate once; merge _lowpass_erp/_lowpass_stack into one _lowpass(values, cutoff, srate, axis) helper and call it for each array, dropping the repeated srate expressions.
  • Evidence: erp1 = _lowpass_erp(erp1, float(lowpass[0]), float(datasets[int(add_indices[0])].get("srate", 1) or 1)) repeated for erp2, erpsub, add_stack, sub_stack, diff_stack (l.67-82); _lowpass_erp axis=1 vs…

8. pop_erpimage reimplements canonical is_on as private _is_on

• functions/popfunc/pop_erpimage.py (lines 514-519)
  • Why: _is_on (l.514-519) is byte-for-byte identical in logic to the canonical is_on in _pop_utils.py (l.99-105), which pop_topoplot.py and pop_headplot.py already import as _is_on. pop_erpimage instead defines its own copy and uses it at l.60, 506.
  • Failure mode: Two parallel on/off normalizers drift over time (e.g. if the canonical one gains a token); EEGLAB on/off parsing becomes inconsistent across pop functions in the same scope.
  • Fix: Import is_on from eegprep.functions.popfunc._pop_utils and delete the local _is_on (l.514-519), updating call sites.
  • Evidence: Local: return value.strip().lower() in {"on", "yes", "true", "1"} (l.516) vs canonical value.strip().lower() in {"1", "on", "true", "yes"} (_pop_utils l.102).

9. pop_load_frombids mixes raw-format decoding and montage matching into a 1200-line popfunc wrapper

• functions/popfunc/pop_load_frombids.py (lines 39-447, 905-1070)
  • Why: A popfunc should be thin user-facing/history glue. This file owns: NEO EDF/BDF/BrainVision stream decoding and rescaling (159-374), full EEG-dict construction (381-441), and a self-contained montage-matching/coordinate-inference engine that scans resources/montages, scores caps, and computes spherical coords (905-1070). Those are sigprocfunc/plugin-level numerical concerns, not BIDS-loader glue,…
  • Failure mode: The neo-based EDF/BDF reader and the montage-inference scorer can only be exercised through the BIDS entry point; a future non-BIDS importer re-implements them (as pop_fileio already does via MNE), so the two raw-EEG readers drift and bugfixes land in only one.
  • Fix: Extract the NEO raw-reader and the montage-inference/coord-assignment into sigprocfunc (or a shared reader module) and have pop_load_frombids call them, leaving the popfunc as BIDS-sidecar orchestration plus history.
  • Evidence: io = NeoIO(filename) ... data_T = io.get_analogsignal_chunk(...) ... for filename in filenames: ... data = loadmat(os.path.join(montage_path, filename) ... score = (fraction_in_data, bonus1020, fracti…

10. pop_rejkurt and pop_jointprob are near-identical files differing only in the marks function and stats field names

• functions/popfunc/pop_rejkurt.py (lines 24-217)
  • Why: pop_rejkurt.py and pop_jointprob.py share the same five-function scaffold (pop_*, *_dialog_spec, _run_gui, _vistype_from_gui, _apply_one, _history_command) with the list-vs-single dispatch, gui handling, trials<=1 guard, elecrange normalization, update_reject_fields, rejected = flatnonzero+1, display-browser-vs-pop_rejepoch branch, and 9-arg history command all duplicated essentially verbatim. The…
  • Failure mode: Five+ rejection pops (pop_eegthresh, pop_jointprob, pop_rejkurt, pop_rejspec, pop_rejtrend) carry the same control flow. A fix to the shared scaffold (e.g. the trials guard message, superpose handling, the reject-then-pop_rejepoch ordering) must be applied independently to each, and they will drift. _vistype_from_gui being copied verbatim in two files is the…
  • Fix: Extract the shared epoched-rejection scaffold into a single helper in rejection.py (e.g. run_epoched_rejection(EEG, icacomp, marks_fn, kind, stats_writer, display, ...)) that the per-method pop* functions call with a marks callback and fi…
  • Evidence: pop_rejkurt.py:157-163 _vistype_from_gui is identical to pop_jointprob.py:153-159; _apply_one bodies match line-for-line except kurtosis_marks vs jointprob_marks and the stats field names at pop…

11. Two divergent chanloc->struct-array converters in one module

• functions/popfunc/pop_saveset.py (lines 240-305, 419-465)
  • Why: _chanlocs_to_struct_array (240-305) already converts a list of chanloc dicts to a MATLAB struct array with a 13-field field_spec (including type, urchan, unit), and is used for chaninfo.removedchans/nodatchans. The inline block at 419-465 re-implements the exact same dict->structured-array conversion for EEG.chanlocs with a different, hand-maintained 12-field dtype that omits unit…
  • Failure mode: A field added/changed in one converter (e.g. unit, or a future coordinate field) is written to chaninfo's removedchans but dropped from the primary chanlocs (or vice-versa), producing .set files whose main and removed channel structs have inconsistent schemas.
  • Fix: Delete the inline 419-465 block and serialize eeglab_dict['chanlocs'] through _chanlocs_to_struct_array, so the single helper is the one canonical chanloc-serializer.
  • Evidence: field_spec = [('labels', 'U100'), ... ('urchan', np.int32), ... ('unit', 'U20')] vs inline dtype = np.dtype([... ('labels', 'U100'), ... ('urchan', np.int32), ...])

12. pop_topochansel duplicates pop_chansel's label/index resolution

• functions/popfunc/pop_topochansel.py (lines 57-72, 74-89)
  • Why: pop_topochansel is a 'compatibility wrapper for pop_chansel' but only delegates to pop_chansel in the GUI branch. In the non-GUI branch it reimplements channel-label extraction (_channel_labels, l.74-78) and selection-to-1-based-index resolution (_resolve_selection, l.80-89), which duplicate pop_chansel's _channel_values (l.73-94) and _selection_to_indices (l.109-131). pop_chansel even exposes pop…
  • Failure mode: Two label-to-index resolvers with subtly different rules (e.g. pop_chansel uses regex token parsing _parse_text and is_int_text; pop_topochansel uses str.split + isdigit, so 'Cz3' vs '-1' or quoted labels behave differently) silently diverge, breaking pop_topochansel parity with the dialog it claims to wrap.
  • Fix: Have the non-GUI branch call pop_chansel's selection resolution (e.g. via pop_chansel_selected_string / a shared _selection_to_indices) instead of maintaining a parallel parser.
  • Evidence: pop_topochansel _resolve_selection: index = lowered.index(str(token).lower()) + 1 (l.84) parallels pop_chansel _selection_to_indices: selected.append(lower_values.index(value) + 1) (l.126).

13. pop_topoplot reimplements shared python_literal verbatim

• functions/popfunc/pop_topoplot.py (lines 446-464, 425-443)
  • Why: _python_literal (l.446-464) is a verbatim copy (same logic, only the docstring differs) of _plot_utils.python_literal (l.168-187), which every other plot wrapper in this scope already uses via history_command. _history_command (l.425-443) also builds the same pop_x(EEG, key=value) shape that _plot_utils.history_command produces.
  • Failure mode: The literal-formatting rules for console history (NaN/inf/list/tuple) now live in two places; if one is changed (e.g. ndarray handling), pop_topoplot's emitted history silently diverges from every other plot function's.
  • Fix: Import python_literal from _plot_utils and delete _python_literal; consider routing _history_command through history_command (it accepts positional literals + kwargs) to drop the bespoke builder.
  • Evidence: Line-by-line comparison shows identical bodies (offset only by python_literal's docstring): both implement the same NaN/inf/list/tuple branches ending in return repr(value).

14. 25 identical single-name dispatch branches re-dispatch into a second elif chain on the same string

• functions/guifunc/menu_actions.py (lines 354-444)
  • Why: dispatch() hand-writes 25 if base == "pop_X": self._run_pop_function("pop_X", parent); return branches (354-428) whose only varying token is the literal name, then run_pop_function (1073-1287) immediately partitions on that same name through its own elif chain. The action name is matched twice and the branch list is maintained in two places. Adding/renaming one pop* action means editing dispat…
  • Failure mode: A maintainer adds a pop_* case to _run_pop_function but forgets the dispatch() if base == stanza; the menu item then silently falls through to show_coming_soon() at line 481 even though _run_pop_function knows how to run it. The reverse drift (dispatch branch with no _run_pop_function arm) hits the else: self.show_coming_soon(name, parent) at 1285-1287.
  • Fix: Collapse lines 354-428 into the same set-membership branch already used for the rejection group at 429-444 (e.g. one if base in _SIMPLE_POP_ACTIONS: self._run_pop_function(base, parent, variant=variant)), so the name is matched once and o…
  • Evidence: if base == "pop_reref":\n self._run_pop_function("pop_reref", parent)\n return

15. Pop-result interpretation contract duplicated in console.py and menu_actions.py; GUI copy already diverges (drops STUDY results)

• functions/guifunc/menu_actions.py (lines 1058-1073, 1800-1831 (vs src/eegprep/functions/adminfunc/console.py:1297-1362, 400-444))
  • Why: The logic that decodes a pop_* return value into (alleeg, eeg, currentset, command) is implemented twice. menu_actions._extension_dataset_state (1800) is byte-equal to console._extract_pop_dataset_state (1312); menu_actions._extension_eeg_and_command (1809) is byte-equal to console._extract_pop_eeg_and_command (1297); _is_eeg_selection (menu_actions 1824 / console 1347) and `_history_o…
  • Failure mode: An extension pop_* that returns a STUDY tuple works from the console but is silently dropped in the GUI: _extension_dataset_state needs len>=4 (returns None), then _extension_eeg_and_command sees result[0] is a study dict without data (_is_eeg_selection False) and result[1] is a list (command=''), so it returns (None, '') and `_apply_extension_resu…
  • Fix: Move the pop-result decoders (_is_eeg_selection, _history_only_command, dataset/eeg/study extractors, _EEG_CORE_FIELDS) into one shared module (e.g. a small helper next to EEGPrepSession or in adminfunc) and have both console.accept_p…
  • Evidence: menu_actions 1804 if not isinstance(alleeg, list) or not _is_eeg_selection(eeg) or not isinstance(command, str): is identical to console 1316; menu_actions 1826 return "data" in value and any(key i…

16. FIR band-edge / desired-amplitude math duplicated between qt.py renderer and firfilt plugin

• functions/guifunc/qt.py (lines 1274-1311)
  • Why: qt.py's _firpm_order_shape reconstructs equiripple FIR band edges and the desired-amplitude table that already live in the firfilt plugin's design_firpm (src/eegprep/plugins/firfilt/_filtering.py:177-189). Both independently compute edges via np.sort(np.concatenate([cutoff - transition/2, cutoff + transition/2])) and both hard-code the identical desired_by_type = {bandpass:[0,1,0], bandstop:[1,0,1…
  • Failure mode: The two copies have already diverged: qt.py keeps edges in Hz and has an extra single-cutoff highpass/lowpass special case (lines 1285-1293 producing 4-point edges [0, low, high, nyquist]), while _filtering.py normalizes edges by Nyquist (/ (srate/2)) with no such special case. A future fix to band-edge handling or the amplitude table in one location (e.g.,…
  • Fix: Extract the band-edge + desired-amplitude construction into one firfilt helper (e.g. in plugins/firfilt/_filtering.py) returning (edges, desired) and have both design_firpm and qt.py's _estimate_firpm_order call it. Keep the Hz-vs-Nyquist n…
  • Evidence: qt.py:1294 edges = np.sort(np.concatenate([cutoff - transition / 2, cutoff + transition / 2])) and qt.py:1299-1304 desired_by_type = {"bandpass": [0, 1, 0], "bandstop": [1, 0, 1], "highpass": [0, 1…

17. QtDialogRenderer is a 1200-line stateless class-as-namespace (46 staticmethods, no instance state)

• functions/guifunc/qt.py (lines 48-1264)
  • Why: QtDialogRenderer spans lines 48-1264 (the bulk of a 1311-line file) and holds zero instance fields. grep shows 46 @staticmethod methods and only 4-5 instance methods (run, build_dialog, _build_widget, _connect_callback, _run_tf_cycle_calc), none of which read or write self. It is instantiated exactly once (inputgui.py:20 renderer = QtDialogRenderer()) and every internal call is `QtDialogRenderer…
  • Failure mode: The class wrapper forces every helper to be reached via the QtDialogRenderer. prefix and bundles ~40 unrelated per-dialog callbacks/validators (reref, interp, headplot, firpm, tf_cycle, eegplot, etc.) into one undecomposed unit. New per-dialog logic accretes here because there is no module boundary pushing related callbacks/validators into their own files,…
  • Fix: Demote the class to module-level functions (the renderer carries no state) with run/build_dialog as the entry points, and split the per-dialog validators and callbacks (firpm/firws estimation, headplot, tf_cycle, reref/interp validation…
  • Evidence: qt.py:48 class QtDialogRenderer:; inputgui.py:20 renderer = QtDialogRenderer(); staticmethod count from grep = 46; instance-method scan returns only _build_widget/_connect_callback/_run_tf_cycle_c…

18. ConsoleEegh re-implements session.add_history and mutates ALLCOM/LASTCOM without notifying session listeners

• functions/adminfunc/console.py (lines 261-277 (vs session.py add_history 363-371))
  • Why: EEGPrepSession.add_history is the canonical helper for appending to ALLCOM/LASTCOM (AGENTS: history must go through session helpers such as add_history/notify_changed). ConsoleEegh forks that logic inline (calls eegh on session.ALLCOM and assigns session.LASTCOM directly) and refreshes only its own namespace via pull_from_session, skipping notify_changed.
  • Failure mode: A console eegh('EEG = pop_x(EEG)') appends to session.ALLCOM and sets session.LASTCOM but never calls notify_changed, so other registered change listeners (e.g. a second EEGPrepConsoleWorkspace sharing the session, or any GUI panel bound to history) are not refreshed until their own next sync. Numeric eegh(0)/eegh(-n) mutate session.ALLCOM in place (vi…
  • Fix: Route ConsoleEegh's string/append and numeric-clear cases through session.add_history (and a session-level history-edit helper for the destructive numeric ops) so a single canonical mutator runs and notify_changed fires, then pull_from_sess…
  • Evidence: ConsoleEegh.__call__: normalized = eegh(command, self.bridge.session.ALLCOM) ... self.bridge.session.LASTCOM = normalized\n self.bridge.pull_from_session() — compared to session.add_history: self.L…

19. Bundled-plugin metadata duplicated in plugin_menu and already drifting from extensions registry

• functions/adminfunc/plugin_menu.py (lines 74-145)
  • Why: extensions.py::_bundled_records() (extensions.py:315-510) is the authoritative declarative source for the five bundled plugins (name, version, description, capabilities, first pop function). plugin_menu.py re-declares the same five plugins as a hand-maintained dict literal _BUNDLED_PLUGINS (lines 74-140), exposed publicly as bundled_plugins() (line 143-145) and consumed only by tests, not by the l…
  • Failure mode: The two descriptions have ALREADY diverged: extensions.py:414 says dipfit is 'Source-localization menu surfaces with EEGPrep-native spherical DIPFIT workflows.' while plugin_menu.py:124 says 'Source-localization menu surfaces and FieldTrip-backed DIPFIT workflows.' A user reading bundled_plugins() sees a description the actual registry/menu contradicts (and…
  • Fix: Delete _BUNDLED_PLUGINS and derive bundled_plugins() from ExtensionRegistry(include_entry_points=False).discover() (reusing the same _plugin_from_record projection the dialog already uses), or drop bundled_plugins() entirely if only tests n…
  • Evidence: extensions.py:414 description="Source-localization menu surfaces with EEGPrep-native spherical DIPFIT workflows." vs plugin_menu.py:124 "description": "Source-localization menu surfaces and FieldTrip-…

20. k-means numeric kernel lives in the pop_clust wrapper; optimal_kmeans/robust_kmeans import it upward (layering inversion)

• functions/studyfunc/optimal_kmeans.py (lines 9-32)
  • Why: The canonical clustering numerics (_kmeans_labels, _squared_distances) are defined inside the user-facing GUI/history wrapper pop_clust.py, and the dedicated numeric modules optimal_kmeans.py and robust_kmeans.py reach back up into that wrapper to reuse them. That inverts the intended layering (sigprocfunc-grade numerics should not depend on a popfunc wrapper) and couples three modules through a p…
  • Failure mode: Any refactor of pop_clust (renaming _kmeans_labels, changing its return convention from labels+1, or moving the dialog code) silently breaks optimal_kmeans and robust_kmeans; an importer of optimal_kmeans drags in inputgui/Qt-spec imports transitively via pop_clust. The +1/0-based label convention baked into _kmeans_labels is also reused by robust_kmeans._ou…
  • Fix: Move the k-means kernel (_kmeans_labels, _squared_distances, center recompute) into a private numeric helper module (e.g. _cluster_kmeans.py) that pop_clust, optimal_kmeans, and robust_kmeans all import downward. Keep dialog/history glue in…
  • Evidence: from eegprep.functions.studyfunc.pop_clust import _kmeans_labels, _squared_distances

21. pop_chanplot reimplements cached-measure reading and plotting that std_readdata + std_measureplot already own

• functions/studyfunc/pop_chanplot.py (lines 192-381)
  • Why: std_readdata already reads both channel (changrp) and cluster/component caches, and _std_measureplot.std_measureplot already turns that into erp/spec line plots and ersp/itc images with identical axis labels and nanmean reductions. pop_chanplot instead carries a parallel cache reader (_cached_channel_groups, _data, _axis, _freq_axis) and a parallel plotter (_plot_cached_lines, _plot_cached_image,…
  • Failure mode: Two code paths compute the same STUDY figures; a fix to measure slicing, axis selection, or labeling in std_measureplot/std_readdata silently does not reach the GUI-facing pop_chanplot, so menu plots and std_*plot console output diverge. New measures or design-aware slicing must be implemented twice.
  • Fix: Make pop_chanplot a thin wrapper that resolves channels/components/measure/mode from the dialog, then calls std_readdata + std_measureplot for the figure; delete the bespoke cache/plot helpers.
  • Evidence: def _plot_cached_lines(groups: list[dict[str, Any]], measure: str, *, title: str) -> Any: ... ax.plot(x_axis, np.nanmean(data, axis=0), ...)

22. pop_clust reimplements outlier marking inline and never uses the existing robust_kmeans module, diverging from EEGLAB

• functions/studyfunc/pop_clust.py (lines 69-91,176-194)
  • Why: EEGLAB's pop_clust dispatches to robust_kmeans when outliers != Inf and records algorithm {'robust_kmeans', clus_num}. The port instead runs plain _kmeans_labels then a hand-rolled _mark_outliers, and always records algorithm=['Kmeans', clus_num]. The real robust_kmeans.py module is exported and tested but has zero runtime callers, so the package carries two different outlier algorithms: one live…
  • Failure mode: Users requesting outlier separation get a different partition than EEGLAB and incorrect provenance in STUDY.cluster.algorithm, undermining the parity promise. The dead robust_kmeans drifts from the live _mark_outliers over time (e.g. _outlier_rows uses a two-stage spread+guard test while _mark_outliers uses a single combined mask), so the tested path no long…
  • Fix: Have pop_clust dispatch to robust_kmeans (or kmeanscluster) per EEGLAB when outliers is finite, delete the parallel _mark_outliers, and label the result 'robust_kmeans' to match EEGLAB std_createclust provenance.
  • Evidence: if np.isfinite(outliers): ... labels = _mark_outliers(data, labels, centers, outliers) ... algorithm=["Kmeans", clus_num]

23. std_clustplot builds its history command with an ad-hoc f-string instead of the shared build_python_call used by every other std_*plot

• functions/studyfunc/std_clustplot.py (lines 43-44)
  • Why: All sibling plot wrappers build history through build_python_call / cluster_command from _study_utils (_std_measureplot.py:204, std_pacplot.py:72, std_preclust.py:89), which centralizes argument quoting and the (targets,...) = call rendering so console history is consistent and round-trips. std_clustplot instead hand-rolls `command = f"fig = std_clustplot(STUDY, ALLEEG, clusters={cluster_indices},…
  • Failure mode: Console history for std_clustplot will not match the formatting/quoting of other STUDY plot commands and bypasses build_python_call's escaping; if cluster_indices ever contains values whose repr is not valid Python (or the rendering convention changes), this line drifts independently and produces non-replayable history. It is a one-off legibility/consistency…
  • Fix: Use cluster_command/build_python_call from study_utils as the other std*plot wrappers do, passing python_literal(cluster_indices) and python_literal(measure).
  • Evidence: std_clustplot.py:43 command = f"fig = std_clustplot(STUDY, ALLEEG, clusters={cluster_indices}, measure={measure!r})" vs _std_measureplot.py:204 return build_python_call(targets, f"std_{datatype}plo…

24. Identical _trial_rows trialinfo normalizer copy-pasted across std_combtrialinfo, std_getindvar, std_selectdataset (and near-identical in std_gettrialsind)

• functions/studyfunc/std_combtrialinfo.py (lines 74-84)
  • Why: The 'normalize trialinfo into a list of row dicts' helper is byte-for-byte identical in std_combtrialinfo, std_getindvar, and std_selectdataset, and differs only by a leading 'trialinfo' unwrap in std_gettrialsind. This is a single canonical operation on the STUDY trialinfo data structure reimplemented four times.
  • Failure mode: Any future fix to trialinfo normalization (e.g. handling a new container shape, structured-array dtype, or None-row case) has to be applied in four files; missing one makes consumers like std_selectdataset and std_getindvar disagree on which trials a STUDY contains, producing inconsistent factor levels / dataset selections from the same trialinfo.
  • Fix: Move one canonical trialinfo_rows() helper into _study_utils (which already owns trialinfo_from_eeg and other trialinfo logic) and have std_combtrialinfo, std_getindvar, std_selectdataset, and std_gettrialsind import it. Keep the gettrialsi…
  • Evidence: std_combtrialinfo.py line 74-83 and std_getindvar.py line 117 and std_selectdataset.py line 94 are identical: if value is None: return [] ... return [row for row in value if isinstance(row, dict)]…

25. Design-matrix factor-matching logic (_trial_rows column-expander, _value_matches, _equal, _matching_rows) duplicated between std_limodesign and std_builddesignmat

• functions/studyfunc/std_limodesign.py (lines 78-99, 199-224)
  • Why: std_limodesign and std_builddesignmat both build STUDY design matrices and both independently implement the dict-of-columns->rows expander (_trial_rows) plus categorical level-matching (_value_matches/_matching_rows/_equal in limodesign vs _matching_level/_level_contains in builddesignmat). These are two copies of the same 'match a trial row's value against a factor level (possibly a list/array le…
  • Failure mode: The two design-matrix builders can drift: a fix to how a level that is itself a list/array is matched (e.g. numeric-vs-string coercion of factor values) applied to one builder but not the other yields different categorical encodings from std_limodesign vs std_builddesignmat for the same design and trialinfo, silently producing inconsistent LIMO vs standard d…
  • Fix: Extract the shared level-matching predicate (value matches level, including list/array levels) and the dict-of-columns row expander into one helper used by both std_limodesign and std_builddesignmat, so categorical encoding is defined once.
  • Evidence: std_limodesign.py line 211-216 _value_matches recurses over list/tuple levels; std_builddesignmat.py line 91-96 _level_contains does the same; std_limodesign.py line 78 _trial_rows (column expan…

26. std_pacplot._default_target duplicates _std_measureplot._default_target but diverges (extra ensure_study, drops datatype map)

• functions/studyfunc/std_pacplot.py (lines 90-96)
  • Why: The shared plot siblings (std_erpplot/specplot/erspplot/itcplot) route through _std_measureplot.std_measureplot, whose _default_target (lines 73-81) decides channels-vs-cluster-1 default by inspecting study.changrp for the measure-specific field. std_pacplot reimplements the identical heuristic in its own _default_target with the same control flow, just keyed on the literal field "pacdata". The tw…
  • Failure mode: Future edits to the default-target rule (e.g. supporting 'components' default or honoring design) must be made in two places; one will be missed, silently giving PAC plots a different default target than ERP/spec/ersp/itc plots. The redundant ensure_study deepcopy also doubles STUDY-copy cost on every PAC plot for no behavioral benefit.
  • Fix: Promote the default-target heuristic into a single shared helper in _std_measureplot (or _study_utils) parameterized by the measure field name, and have both std_measureplot and std_pacplot call it. Drop the redundant ensure_study in std_pa…
  • Evidence: std_pacplot.py:90-96 def _default_target(study, channels, clusters, components):\n if channels is not None or clusters is not None or components is not None:\n return channels, clusters\n prepared =…

27. Cached measure-axis resolution duplicated across std_readdata and std_preclust (drifting copies of the same read contract)

• functions/studyfunc/std_readdata.py (lines 233-246, 271-276, 308-312)
  • Why: std_readdata._cached_axis_values (resolve measureinfo.datasets/components, fallback to sets/comps unique order, then arange) and std_preclust._measure_axis implement the identical cache-axis contract against the same parent measure fields. Likewise std_readdata._axis_position and std_preclust._axis_position are duplicate lookups (differing only by an extra 'measure' label arg), and _unique_preserv…
  • Failure mode: The cache read contract (how datasets/components axes are recovered from a measure group) lives in two places. A future change to how std_precomp writes measureinfo (e.g. renaming a key or changing fallback order) must be mirrored in both std_readdata and std_preclust or the two readers will silently disagree about which dataset/component a cached row belong…
  • Fix: Hoist one canonical axis-resolution helper (and _axis_position, and _unique_preserving_order) into a shared module such as _study_utils or a small _measure_cache helper, and have std_readdata and std_preclust both call it. Delete the per-fi…
  • Evidence: std_readdata.py line 242: unique_values = np.asarray(_unique_preserving_order(values.tolist()), dtype=int); std_preclust.py line 207: identical line in _measure_axis; std_readdata.py line 308 def…

28. Three near-identical range-mask helpers across std_readdata, std_pac, and _std_measureplot with subtly different empty-axis behavior

• functions/studyfunc/std_readdata.py (lines 390-402)
  • Why: std_readdata._range_mask (390-402), std_pac._range_mask (430-440), and _std_measureplot._axis_mask (126-137) all implement the same '[min max] -> boolean mask, raise if nothing selected' contract. They differ in edge handling: std_readdata returns an empty bool array for an empty axis and an all-true mask for empty bounds; std_pac's version returns all-true for empty bounds but does not special-ca…
  • Failure mode: A range that std_measureplot._axis_mask accepts (or rejects) may be re-validated by std_readdata._range_mask with different semantics, so changing the bounds-validation rule (e.g. tolerance, inclusive/exclusive bounds, empty-axis) in one helper silently leaves the others inconsistent, giving different selection results depending on whether the user called st…
  • Fix: Consolidate into one range-mask helper (in _study_utils or std_readdata) returning a boolean mask, and have std_pac, _std_measureplot, and std_readdata import it; keep the single error message and edge-case policy in one place.
  • Evidence: std_readdata.py:397-401 if values.size != 2:\n raise ValueError("range filters must contain [min max]")\n mask = (axis >= values[0]) & (axis <= values[1])\n if not np.any(mask):\n raise ValueError("r…

29. Measure-field name map {erp:erpdata,...} duplicated in std_readdata and _std_measureplot._default_target

• functions/studyfunc/std_readdata.py (lines 323-327)
  • Why: The mapping from datatype to cached-array field name appears verbatim as {"erp": "erpdata", "spec": "specdata", "ersp": "erspdata", "itc": "itcdata"} in std_readdata._data_array (line 324) and again in _std_measureplot._default_target (line 78). The axis-field maps (_x_axis line 431, _y_axis line 436) are the canonical place these live. _std_measureplot reaching into the same literal to decide d…
  • Failure mode: If a new measure is added or a field is renamed (e.g. itcdata -> itc_data) in std_readdata, _std_measureplot._default_target's literal still references the old name; the default-target detection silently fails to find the field, defaulting to cluster 1 instead of channels, and users get the wrong groups plotted with no error.
  • Fix: Export a single MEASURE_DATA_FIELDS constant (or a small accessor) from std_readdata and import it in _std_measureplot._default_target instead of re-typing the dict literal.
  • Evidence: std_readdata.py:324 field = {"erp": "erpdata", "spec": "specdata", "ersp": "erspdata", "itc": "itcdata"}[measure] and _std_measureplot.py:78 field = {"erp": "erpdata", "spec": "specdata", "ersp": "…

30. Statistics package is a 1036-line _core.py mega-module fronted by 14 hollow re-export shims (inverted EEGLAB structure)

• functions/statistics/_core.py (lines 1-1036)
  • Why: All 14 public statistics functions (fdr, statcond, surrogdistrib, the *_cell tests, etc.), 6 result dataclasses, and ~30 private helpers live in one 1036-line _core.py. The 14 sibling files (fdr.py, statcond.py, anova1_cell.py, ...) are 5-line modules that only from ._core import and re-export. EEGLAB ships each of these as its own .m file with the real implementation; AGENTS.md asks to keep dir…
  • Failure mode: The on-disk layout mirrors EEGLAB filenames but the logic does not live where the filename promises, so 'where does statcond live' is misleading and the single file mixes unrelated concerns (FDR, surrogate CIs, t/ANOVA cell stats, resampling, a smoke-test harness). The inversion also forced a documented fragility in init.py (lines 6-9): importing a same-…
  • Fix: Move each function's implementation into its correspondingly named module (fdr.py, statcond.py, etc.) as EEGLAB does, keeping only genuinely shared private helpers (_condition_grid, _resampled_grid, _as_numeric_array, the dataclasses) in a…
  • Evidence: statistics/__init__.py:6-9 comment Import same-name thin modules before binding package callables. Without this, a later import ... can replace statistics.fdr with the submodule object.; fdr.py is 6…

31. Three different empirical-p-value conventions coexist with no shared definition

• functions/timefreqfunc/_pac_support.py (lines 445-450)
  • Why: The codebase has three separate notions of 'empirical p-value against surrogates': bootstat.exact_p_values (mean of surrogate_distance>=observed_distance about the surrogate mean, no +1), _pac_support._empirical_pvalue ((count(>=|obs|)+1)/(N+1), one-sided about zero), and statistics.stat_surrogate_pvals (sum(surr>=obs)/N with explicit two-tail folding). Each lives in a different module and is reac…
  • Failure mode: The same statistical question ('is this value significant vs its surrogate distribution?') yields materially different p-values depending on which time-frequency entry point produced it (PAC vs ERSP/ITC vs statcond), and there is no single place to audit or correct the convention. The +1/(N+1) bias correction in PAC but not in ERSP is an undocumented inconsi…
  • Fix: Pick one canonical empirical-p-value helper (statistics.stat_surrogate_pvals is the obvious owner given its tail handling) and have bootstat/_pac_support delegate to it, or document explicitly in code why each path needs a distinct conventi…
  • Evidence: _pac_support.py:450 return float((np.count_nonzero(values >= abs(observed)) + 1) / (values.size + 1)); bootstat.py:79 return np.nanmean(surrogate_distance >= np.expand_dims(distance, axis=0), axis=…

32. Canonical bootstat threshold helper is exported but never used; newtimef/newcrossf reimplement it

• functions/timefreqfunc/bootstat.py (lines 22-66)
  • Why: bootstat.py defines the canonical EEGLAB-parity bootstrap path (bootstat + bootstrap_threshold). A grep across the package shows the only consumers of bootstat/bootstrap_threshold are bootstat.py itself and the package init/re-exports; newtimef.py and newcrossf.py import only exact_p_values from this module and then roll their own threshold+resampling logic.
  • Failure mode: Three parallel implementations of 'percentile threshold over accumulated surrogates' drift independently. bootstrap_threshold sorts on axis 0 and uses int(round(N*alpha)); newtimef._thresholds_by_frequency transposes (1,0,2)->reshape and does the same round but with a per-frequency pooling; newcrossf._upper_thresholds_by_frequency duplicates the upper-only v…
  • Fix: Make newtimef and newcrossf call bootstat.bootstrap_threshold (it already supports bootside='both'/'upper' and complex magnitude) and route their per-naccu surrogate accumulation through it, deleting _thresholds_by_frequency and _upper_thre…
  • Evidence: bootstat.py:53 def bootstrap_threshold(surrogates: Any, *, alpha: float = 0.05, bootside: str = "both"); newtimef.py:504 def _thresholds_by_frequency(values, *, alpha, both); newcrossf.py:242 def…

33. _numeric_vector/_is_on/_colon_sequence duplicated and drifting across 5 timefreqfunc modules despite a canonical helper

• functions/timefreqfunc/newtimef.py (lines 655-701)
  • Why: popfunc/_pop_utils.py:57 already provides canonical parse_numeric_sequence (with start:stop range support) and is_on. _pac_support.py:12 correctly imports both. But newtimef.py, newcrossf.py, timefreq.py, and tf_cycle_calc.py each define their own local _numeric_vector/_is_on, and they have already diverged: newtimef/_tf_cycle_calc support colon ranges, while newcrossf.py:394 and timefreq.py do NO…
  • Failure mode: A user passing a MATLAB-style 'freqs' string like '2:2:50' works in newtimef but parses as a single bad float (raising ValueError) in newcrossf, even though both present the same EEGLAB-style string-vector interface. Future fixes to numeric parsing must be applied in 5 places or behavior silently diverges by entry point.
  • Fix: Replace the four local _numeric_vector/_is_on/_colon_sequence definitions with imports of parse_numeric_sequence/is_on from popfunc._pop_utils (wrapping in np.asarray(...).ravel() as _pac_support already does), and delete the local copies.
  • Evidence: _pop_utils.py:57 def parse_numeric_sequence(value, *, dtype=float) with _RANGE_TOKEN colon support; newcrossf.py:405 np.asarray([float(token) for token in text.replace(",", " ").split()]...) (no…

34. newtimef forks Benjamini-Hochberg FDR instead of calling canonical statistics.fdr used by its siblings

• functions/timefreqfunc/newtimef.py (lines 515-534)
  • Why: statistics/_core.py:104 provides the canonical fdr() (BH/BY with a tested mask). _pac_support.py:13 and correct_mc consumers (newcrossf path) import and use it via fdr(pvalues, alpha).mask. newtimef.py imports exact_p_values from bootstat but reimplements the BH threshold by hand in _fdr_threshold and applies pvalues<=threshold in _significance_mask.
  • Failure mode: newtimef's hand-rolled FDR only implements parametric BH (alpha*rank/N) with no nonparametric (Benjamini-Yekutieli) correction option and its own edge handling (returns 0.0 -> all-false). Any correction or tie/boundary fix made in the canonical fdr (the version other time-frequency code already trusts) does not reach newtimef, so mcorrect='fdr' significance…
  • Fix: In _significance_mask, call statistics.fdr(pvalues, alpha) and use .mask for the 'fdr' branch, deleting _fdr_threshold.
  • Evidence: newtimef.py:525 def _fdr_threshold(pvalues, alpha) ... accepted = values <= alpha * ranks / values.size; statistics/_core.py:104 def fdr(pvals, q=None, fdr_type="parametric"); _pac_support.py:25…

35. Bootstrap plumbing (_threshold_vector, _bootstrap_indices, _resample) duplicated between newtimef and newcrossf

• functions/timefreqfunc/newtimef.py (lines 275-281)
  • Why: newtimef.py and newcrossf.py independently maintain the same bootstrap support helpers. _threshold_vector is byte-identical (newtimef.py:548 vs newcrossf.py:275, verified with diff). _bootstrap_indices (newtimef.py:423 vs newcrossf.py:263) and the trial resamplers (_resample_trials vs _resample_pair, both branching on the same 'shuffle'/'shufftrials'/'rand'/'randall' tokens) are near-identical str…
  • Failure mode: boottype handling, baseboot index semantics, and threshold broadcasting must be kept in lock-step by hand across two ~400-700 line files. The brief's 'drifting duplicate logic' risk is concrete: newcrossf._bootstrap_indices already differs from newtimef's (different scalar-baseline branch) so a baseboot semantics fix applied to one will not match the other.
  • Fix: Extract the shared bootstrap helpers (_threshold_vector, baseboot index resolution, the boottype-dispatch resampler) into a single private timefreq bootstrap helper module that both newtimef and newcrossf import, alongside the bootstat cons…
  • Evidence: diff of newtimef.py:548-554 vs newcrossf.py:275-281 reports IDENTICAL; newtimef.py:423 def _bootstrap_indices(times, baseline, baseboot, baseln) vs newcrossf.py:263 def _bootstrap_indices(times, ba…

36. pop_prop_extended.py is a single 1376-line module mixing history glue, dipfit/classifier numerics, and Matplotlib dashboard rendering

• plugins/ICLabel/pop_prop_extended.py (lines 1-1376)
  • Why: One popfunc module owns the pop wrapper + dialog spec (thin glue, appropriate), plus ~25 Matplotlib plotting helpers (_plot_topography/_plot_classifier/_plot_activity/_plot_dipfit*/_render_dashboard), dipfit moment math (_dipfit_moments/_dipole_moment_ratio/_component_pvaf), event/epoch marker rendering, and rejection-state GUI controls. That is rendering + numerics + GUI state all in popfunc, wel…
  • Failure mode: The breadth makes the user-facing pop entry hard to locate among rendering internals, and concentrates Matplotlib/GUI concerns in popfunc where AGENTS.md expects thin history glue. Changes to plotting risk the history/return-com contract and vice-versa; the file is a magnet for further accretion.
  • Fix: Split the Matplotlib dashboard rendering and dipfit/pvaf numerics into separate modules (e.g. a viewprops rendering helper and a dipfit-stats helper), leaving pop_prop_extended as the thin wrapper + dialog spec + history command.
  • Evidence: def _render_dashboard(figure: Any) -> None: (~115-line function at line 443) def _plot_dipfit_moment(...) def _component_pvaf(...) def _add_rejection_controls(figure, dashboard) # GUI state mutation i…

37. Three divergent copies of the channel-removal + mask-update block

• plugins/clean_rawdata/clean_flatlines.py / clean_channels.py / clean_channels_nolocs.py (lines clean_flatlines 49-73; clean_channels 148-179; clean_channels_nolocs 101-143)
  • Why: The 'try pop_select; on failure drop ica metadata, cast to float32, slice channels, update clean_channel_mask' block is copy-pasted into all three clean_channels* functions, and the three copies have diverged in correctness-relevant ways.
  • Failure mode: Divergences: clean_flatlines has the walrus bug (above) and indexes EEG['chanlocs'][~removed] assuming an ndarray (line 66) while the other two wrap with np.asarray([...]); clean_channels updates the mask with no size guard (line 175) while clean_channels_nolocs guards on sum(mask) == len(removed_channels) (line 136) and uses np.logical_and. A fix or parit…
  • Fix: Extract a single private helper (e.g. in private/, alongside masks.py) remove_channels(EEG, removed_channels) handling pop_select+fallback+mask compositing, and call it from all three. This also fixes the divergent mask logic in one place…
  • Evidence: All three contain near-identical from eegprep import pop_select / EEG['data'] = np.asarray(EEG['data'], dtype=np.float32) / for field in ['icawinv','icasphere',...] / clean_channel_mask blocks…

38. firfilt plugin's canonical firws/firwsord are owned by clean_rawdata/private

• plugins/clean_rawdata/private/sigproc.py (lines 275-462 (firws, firwsord))
  • Why: firws.m and firwsord.m are EEGLAB firfilt-plugin functions. Here the authoritative implementations live inside the clean_rawdata plugin's private/ helper module, and the firfilt plugin merely re-exports them through one-line shims (firfilt/firws.py, firfilt/firwsord.py). This inverts plugin ownership and creates a hard runtime dependency from firfilt (and popfunc, guifunc) onto clean_rawdata's pri…
  • Failure mode: firfilt/_filtering.py does from eegprep.plugins.firfilt.firws import firws, which chains to from ..clean_rawdata.private.sigproc import firws. pop_resample.py and guifunc/qt.py also import firfilt.firwsord. If clean_rawdata is ever refactored, repackaged, or its private/ contents are treated as truly private (the AGENTS.md repo map calls private/ 'ports…
  • Fix: Move the canonical firws/firwsord (and the shared design_fir/design_kaiser/firws kernel helpers if they belong to firfilt) into the firfilt plugin, and have clean_rawdata import from firfilt, matching EEGLAB's actual function ownership; or…
  • Evidence: firfilt/firws.py: from ..clean_rawdata.private.sigproc import firws; firfilt/_filtering.py line 14: from eegprep.plugins.firfilt.firws import firws; pop_resample.py and guifunc/qt.py import firfil…

39. pipeline apply* duplicate the transforms _resample/_rereference/_clean/_epoch/_ica logic

• cli/commands/pipeline.py (lines 465-579)
  • Why: Every mutating pipeline step (_apply_resample, _apply_rereference, _apply_clean, _apply_epoch, _apply_ica) re-derives the same pop_* call and param mapping already encoded in transforms.py (_resample, _rereference, _clean, _epoch, _ica). E.g. _apply_clean rebuilds a key_map of snake_case->CamelCase clean_rawdata kwargs (L553-560) that is a strict subset of transforms.py `_c…
  • Failure mode: Two parallel mappings for the same operations mean parameter coverage, base-1 channel handling, and determinism flags must be maintained in two places. They are already inconsistent: transforms ICA supports pca, extended, --option KEY=VALUE, channel selection and reorder; pipeline ICA supports only seed/maxsteps/extended/lrate and no channels. Users…
  • Fix: Have pipeline steps call the same shared step-appliers as the transform subcommands (one canonical apply_resample/apply_rereference/apply_clean/apply_epoch/apply_ica), parameterized by a plain dict, rather than maintaining a second transl…
  • Evidence: pipeline.py L553 key_map = {"burst_criterion": "BurstCriterion", ...} vs transforms.py L380-388 "FlatlineCriterion": ... — same CamelCase targets reconstructed independently.

40. Stale 'temporary harness until shared CLI dispatcher is available' contract; per-module main() now diverges from the real CLI

• cli/commands/transforms.py (lines 1-7, 71-127)
  • Why: The module docstring says it is 'dispatcher-neutral... tests can use the module-level harness with python -m eegprep.cli.commands.transforms until the shared CLI foundation is available.' That foundation (cli/main.py) now exists and mounts register_subcommands. The leftover per-module main() (and the analogous ones in pipeline/qc/report/bids/migrate) print results with a fixed format (always J…
  • Failure mode: A maintainer reading the docstring believes the shared dispatcher is still missing and may build redundant scaffolding, or assume the per-module harness output is the canonical contract. Two output formats (module main() vs cli/main.py) for the same commands invite copy errors and confuse anyone debugging CLI output.
  • Fix: Update/remove the stale docstring claim and consolidate the per-module main() harnesses (or clearly mark them test-only) now that cli/main.py is the real entry point, so there is one documented output path.
  • Evidence: transforms.py L4-6 'until the shared CLI foundation is available'; main.py L20,123 already from eegprep.cli.commands.transforms import register_subcommands / register_subcommands(subparsers).

41. Dead run_main() duplicates main.py's exception-to-payload handling

• cli/core.py (lines 293-327)
  • Why: run_main has zero callers in src/ (verified by grep). It reimplements exactly the exception-handling cascade that cli/main.py:main inlines at L67-96: EEGPrepCLIError -> to_response, then getattr(exc,'code'/'message'/'path'/'suggestion') reconstruction, then UNEXPECTED_ERROR fallback with the same suggestion string 'Rerun with --verbose or file an issue if this is reproducible.'
  • Failure mode: Maintainers must keep two copies of the top-level error contract in sync; a fix to the live handler in main.py will not reach run_main and vice versa, and a reader cannot tell which is authoritative. It is pure carrying cost.
  • Fix: Delete run_main (and eprint if also unused), or make main.py call it so there is a single error-handling path.
  • Evidence: core.py L319-322 error = EEGPrepCLIError("UNEXPECTED_ERROR", str(exc), suggestion="Rerun with --verbose or file an issue if this is reproducible.") duplicated at main.py L88-91.

42. --json detection special-cased for qc's REMAINDER, fragile cross-module coupling

• cli/main.py (lines 228-231, 239 (qc.py))
  • Why: qc registers its args as nargs=argparse.REMAINDER into args.qc_args (qc.py L239), so the root parser never binds a top-level args.json for qc. To compensate, main.py _json_requested reaches into getattr(args, "qc_args", []) specifically (L231). The top-level JSON-output decision thus knows about one subcommand's internal attribute name.
  • Failure mode: Any future REMAINDER-style command (or a rename of qc_args) silently loses --json detection in the root error path, emitting human text where an agent expects JSON. The coupling is invisible from qc.py's side and easy to break.
  • Fix: Have qc parse its own --json and surface a uniform attribute (or return the json preference in the result), instead of main.py introspecting qc_args. Avoid REMAINDER for a command that needs first-class flag handling.
  • Evidence: main.py L231 return "--json" in (getattr(args, "qc_args", []) or []); qc.py L239 parser.add_argument("qc_args", nargs=argparse.REMAINDER).

43. extension_catalog.py glues two unrelated concerns (manager catalog vs curation CI validator) with disjoint consumers

• extension_catalog.py (lines 1-330, 333-1009)
  • Why: The 1039-line module is two modules in a trenchcoat. Lines 32-330 implement the runtime Extension Manager catalog (ExtensionCatalog/ExtensionCatalogEntry, load_extension_catalog, parse_extension_catalog, build_safe_install/update_commands) consumed ONLY by adminfunc/plugin_menu.py. Lines 333-1009 implement an entirely separate submission-curation CI validator (CatalogValidationIssue/Report/Options…
  • Failure mode: Reading or changing the manager catalog forces a maintainer to scroll past ~700 lines of unrelated curation-CI code (and vice versa); the shared file name hides that these are independent subsystems. The split obscures ownership and makes the catalog module the single biggest legibility cost in the extension stack.
  • Fix: Split into two modules, e.g. extension_catalog.py (manager loading/install commands, ~330 lines) and extension_catalog_validation.py (curation CI validator + main(), ~680 lines). Repoint the eegprep-validate-extension-catalog entry point an…
  • Evidence: CATALOG_KIND_MANAGER consumers: only plugin_menu.py imports parse/load/build_safe_*. CATALOG_KIND_CURATION half: 'def main' at line 987 wired to pyproject 'eegprep-validate-extension-catalog = "eegpre…

44. Canonical entry-point/version helpers re-implemented instead of imported from extensions.py

• extension_catalog.py (lines 910-923, 938-946, 964-966)
  • Why: extensions.py owns _entry_point_package_name (976-984), _select_entry_points (892-901), and _major_version (997-1002). extension_catalog.py re-implements all three. _entry_point_package_name (938-946) is byte-for-byte identical to the extensions.py version; _select_entry_points (910-923) is the same TypeError-fallback algorithm with the group hardcoded to EXTENSION_ENTRY_POINT_GROUP instead of par…
  • Failure mode: Three copies of entry-point/version logic must be kept in lockstep. If the entry-point metadata-reading contract changes (e.g. importlib.metadata API shift, or stripping epochs/local versions in _major_version), one copy gets fixed and the catalog validator silently keeps the old behavior, producing inconsistent compatibility verdicts between the registry an…
  • Fix: Import _entry_point_package_name and a parameterized _select_entry_points from extensions.py (promote them or wrap the existing ones), and reuse extensions._major_version (or expose a public api_version_supported helper). Delete the catalog…
  • Evidence: extensions.py:976-984 and extension_catalog.py:938-946 are identical; both: dist = getattr(entry_point, "dist", None); ... return str(dist_metadata.get("Name") or "").

45. Active-record predicate duplicated as both a record property and a module function

• extensions.py (lines 244-256, 935-945)
  • Why: ExtensionRecord.is_active (244-256) returns enabled and spec is not None and status in {BUNDLED, INSTALLED, CURATED}. _can_contribute(record) (935-945) encodes the exact same three-clause predicate as a standalone function. The only caller of each differs: extension_runtime.py:65 uses record.is_active, extensions.py:597 (_mark_duplicate_contributions) uses _can_contribute.
  • Failure mode: If the set of 'contributing' statuses changes (e.g. a new ACTIVE-but-flagged status is added), a maintainer can update one definition and leave the other, so duplicate-contribution detection and runtime activation would disagree about which records are live.
  • Fix: Delete _can_contribute and call record.is_active at extensions.py:597. The predicate is identical.
  • Evidence: is_active: self.enabled and self.spec is not None and self.status in {BUNDLED, INSTALLED, CURATED}; _can_contribute: record.enabled and record.spec is not None and record.status in {BUNDLED, INSTAL…

---

Generated by automated review; verify each finding from first principles before fixing.

[claude]

🤖 Plan:

• Read AGENTS.md and fix-issue / pull-request skills
• Actionability + duplicate-work preflight (no open PR targets [Fable 5 Findings] Phase 4 — Architecture: kill duplication, fix ownership & decompose sprawl #197)
• Spot-check key claims (canonical is_on/parse_numeric_sequence exist in _pop_utils.py; 9 _is_on private copies present; k-means upward-imports confirmed; qt.py 1292 LOC / 45 staticmethods; firws lives under clean_rawdata/private/; statistics _core.py 1036 LOC; pop_load_frombids.py 1202 LOC; pop_prop_extended.py 1376 LOC; extension_catalog.py 1039 LOC)
• Classify findings + propose tranches (below)
• Await human decision on sequencing / behavioral findings before implementation

Analysis

Issue #197 bundles 45 architecture findings across popfunc, guifunc, adminfunc, studyfunc, statistics, timefreqfunc, plugins/{clean_rawdata,firfilt,ICLabel}, cli/, and extensions*.py. This is firmly in "anything larger" territory: not one PR. Per the fix-issue workflow I am posting an approach for review rather than opening code.

The findings are real — random spot-checks confirmed: src/eegprep/functions/popfunc/_pop_utils.py:99 defines the canonical is_on while 9 modules (pop_runica, pop_export, pop_loadset, pop_newset, pop_comperp, pop_erpimage, headplot, newtimef, newcrossf) carry private _is_on copies; studyfunc/optimal_kmeans.py:9 and robust_kmeans.py:10 both from eegprep.functions.studyfunc.pop_clust import _kmeans_labels, _squared_distances (upward import from a popfunc); plugins/firfilt/firws.py:3 and firwsord.py:3 re-export from plugins/clean_rawdata/private/sigproc.py.

Proposed Approach

Land the 45 findings as 6 sequenced tranches, each its own PR, so review stays bounded and any regression is isolable. Categories D/E need design sign-off per item before code.

Tranche 1 — Canonical-helper imports (low risk, ~10 findings)
One PR. Delete private duplicates and import canonicals. Pure mechanical, no behavior change, easy to review by diff.

• Fix mutable default argument and incorrect file loading order #3 + coords not defined #6 + Matlab conversion fixes and new eegrej function #8 + Add visible search button to documentation navbar #33: route all 9 _is_on copies + 5 timefreq _numeric_vector/_colon_sequence copies through _pop_utils.is_on / parse_numeric_sequence. Move both helpers down into a shared module first (e.g. functions/miscfunc/_value_utils.py) so sigprocfunc/timefreqfunc are not importing upward from popfunc — that is itself one of the layering complaints.
• ds002680 issues #13: pop_topoplot._python_literal → _plot_utils.python_literal.
• Map release process based on EEGDash #24 + build(deps): bump torch from 2.5.1+cpu to 2.8.0 #29: hoist _trial_rows and the {erp:erpdata,...} map into _study_utils.
• Logo #41: delete dead cli/core.py::run_main.
• Add cart2topo coordinate conversion #45: replace extensions._can_contribute with record.is_active.
• Add cart2topo coordinate conversion #44: import _entry_point_package_name/_select_entry_points/_major_version from extensions.py in extension_catalog.py.

Tranche 2 — Single-helper extractions, plot/STUDY scope (~7 findings)
One PR per area, mechanical refactor with line-equivalence tests.

• refactor for packaging and test distribution #1 (rejection scaffolding in _eegplot_rejection.py ↔ pop_eegplot.py).
• EDF testing and MATLAB modification #7 (_lowpass_erp/_lowpass_stack → one axis-parameterized helper in pop_comperp.py).
• pop_biosig and eeg_checkset_strict_mode missing on clean eegprep install #10 (epoched-rejection scaffold across pop_rejkurt/pop_jointprob/pop_eegthresh/pop_rejspec/pop_rejtrend → _rejection.run_epoched_rejection).
• Issue with integer #11 (collapse pop_saveset.py:419-465 inline chanloc dtype onto _chanlocs_to_struct_array).
• bids_preproc multithreading on Mac #12 (pop_topochansel non-GUI branch → pop_chansel._selection_to_indices).
• MNE import and export #23 (std_clustplot history → build_python_call/cluster_command).
• Bug: bids_preproc() on a single .edf fails due to README.md in montages directory #26 + because of the numpy restriction with lower, we can not use on python>=3.13 #27 + Remove numpy<2 restriction to enable Python 3.13+ compatibility #28 (one canonical _default_target, _axis_position/_unique_preserving_order, _range_mask in _std_measureplot or _study_utils).

Tranche 3 — ICA / clean_rawdata ownership fixes (~3 findings)
One PR. Extracts shared private helpers, no semantic change.

• Pipeline issue #5 (one finalize_ica_fields(EEG, sortcomps, posact) consumed by eeg_runica.py, eeg_amica.py, eeg_picard.py). Spec hints this also fixes a posact-invariant drift; verify before/after numerically.
• Fix test cases and improve MATLAB parity #37 (one remove_channels(EEG, removed_channels) for clean_flatlines.py, clean_channels.py, clean_channels_nolocs.py). Resolves the divergent mask logic in a single place.
• NEMAR data quality plots #20 (k-means kernel: move _kmeans_labels/_squared_distances from pop_clust.py to studyfunc/_cluster_kmeans.py; pop_clust/optimal_kmeans/robust_kmeans all import downward). Pre-req for Documentation: add MNE examples #22.

Tranche 4 — Timefreq bootstrap / statistics unification (~4 findings)
One PR. Each change is a delete-and-import; numerical regression tests against current outputs required.

• Add changelog and contributing similar to EEGDash #25 (factor-matching predicate shared between std_limodesign and std_builddesignmat).
• Search bar within the webpage #32 (newtimef/newcrossf _thresholds_by_frequency/_upper_thresholds_by_frequency → bootstat.bootstrap_threshold).
• Updating the documentation #34 (newtimef._fdr_threshold → statistics.fdr).
• Preprocessing pipeline in User Guide not updated to the current functions' definitions #35 (extract _threshold_vector/_bootstrap_indices/_resample_* into one timefreqfunc/_bootstrap.py).

Tranche 5 — Large-module decompositions (one PR each, ~6 findings)
Each is a 200–700 LOC mechanical split. Do these after Tranches 1–4 so we're not moving code that's about to be deleted.

• Pop epoch #9: extract NEO raw reader + montage inference out of pop_load_frombids.py (1202 LOC) into sigprocfunc/a reader module.
• ds006695 #17: demote QtDialogRenderer (1292 LOC file, 45 staticmethods, zero instance state) to module functions; split per-dialog validators into siblings.
• Creating documentation and update eegprep functions #30: fan functions/statistics/_core.py (1036 LOC) out across the 14 already-existing fdr.py/statcond.py/anova1_cell.py/... shims, keeping only shared private helpers in _core.py. Removes the documented __init__.py:6-9 import-order fragility.
• Add Claude Code GitHub Workflow #36: split plugins/ICLabel/pop_prop_extended.py (1376 LOC) into popfunc glue + a viewprops rendering helper + a dipfit-stats helper.
• real data here #38: relocate firws/firwsord from plugins/clean_rawdata/private/sigproc.py into the firfilt plugin (matches EEGLAB ownership; clean_rawdata imports downward).
• Set up parity testing harness for agentic development #43: split extension_catalog.py (1039 LOC) into extension_catalog.py (manager, ~330 LOC) + extension_catalog_validation.py (curation CI, ~680 LOC); repoint the eegprep-validate-extension-catalog entry point.
• Dashboard for NEMAR #19: derive plugin_menu.bundled_plugins() from ExtensionRegistry(...).discover(); delete _BUNDLED_PLUGINS dict (already drifted vs extensions.py:414).

Tranche 6 — Coupling / stale-contract cleanups (~5 findings)
One PR.

• '1CPU' does not seem to work #14 (menu_actions.dispatch 25 single-name elif branches → if base in _SIMPLE_POP_ACTIONS).
• Channel type is not always a string (sometimes it is an empty array or None) #15 (move _is_eeg_selection/_history_only_command/pop-result decoders into one shared module; fixes the silent STUDY-result drop in the GUI path).
• Quickstart #40 (remove the stale "until shared CLI dispatcher is available" docstrings now that cli/main.py exists; consolidate per-module main() harnesses).
• docs: fix quickstart mismatch, add logo, harden CI (closes #40, #41) #42 (have qc parse its own --json instead of main.py reaching into args.qc_args).
• New Homepage #39 (CLI pipeline._apply_* → reuse transforms._resample/_rereference/_clean/_epoch/_ica).

Needs Human Decision Before Coding

These findings change observable behavior — they are parity/correctness calls, not refactors. Each needs a 1-line ruling before I touch them:

• Refactor #2 — boundary detection. Canonical eeg_findboundaries uses string startswith('boundary') ∪ numeric -99 (gated on option_boundary99). _event_utils.is_boundary_event uses numeric -1 and exact 'boundary'; pop_resample._is_boundary_event uses ungated -99. Per EEGLAB the right answer is "route everything through eeg_findboundaries" — confirm? (This may belong in Phase 3 [Fable 5 Findings] Phase 3 — Correctness bugs, silent failures & misleading contracts #196 rather than [Fable 5 Findings] Phase 4 — Architecture: kill duplication, fix ownership & decompose sprawl #197; it reads as a correctness bug.)
• Resolve conflict between lincuan:patch-1 and sccn:develop in OctaveWrapper #4 — component_activations. _rejection.component_activations recomputes from weights@sphere (order='F'); _plot_utils.component_activations trusts stored icaact when present (order='C'). Pick one. Does rejection scoring need to ignore stored icaact (sign-flips / posact reordering)?
• List of errors -- to discuss #18 — ConsoleEegh.__call__. Currently bypasses session.add_history and skips notify_changed. Routing through add_history is correct per AGENTS.md but will fire listeners on every eegh call — confirm desired.
• Documentation: add MNE examples #22 — pop_clust outlier dispatch. EEGLAB dispatches to robust_kmeans when outliers != Inf; the port runs plain _kmeans_labels + a hand-rolled _mark_outliers and labels the result 'Kmeans'. The robust_kmeans.py module exists and is exported but never called at runtime. Fix restores EEGLAB parity but changes partitions for current consumers — confirm acceptable.
• Merge pull request #30 from sccn/setup-documentation Creating documentation and update eegprep functions #31 — empirical p-value convention. Three live conventions: bootstat.exact_p_values (no +1), _pac_support._empirical_pvalue (+1/(N+1) one-sided), statistics.stat_surrogate_pvals (sum/N two-tail). Pick one canonical (spec proposes stat_surrogate_pvals) and document why each remaining caller needs anything different.

Cross-Phase Note

#198 (Phase 5) covers "dead-code cleanup"; #196 (Phase 3) covers "Correctness bugs, silent failures & misleading contracts". Some items overlap:

• Refactor #2 (boundary detection) and List of errors -- to discuss #18 (history not firing listeners) read as Phase 3 correctness.
• Logo #41 (dead run_main) reads as Phase 5 dead-code.

Confirm whether to migrate these or land under #197 anyway.

Relevant Code

• src/eegprep/functions/popfunc/_pop_utils.py#L57-L105 — canonical parse_numeric_sequence, is_empty_value, is_on
• src/eegprep/functions/studyfunc/optimal_kmeans.py#L9 / robust_kmeans.py#L10 — upward import from pop_clust (NEMAR data quality plots #20)
• src/eegprep/plugins/firfilt/firws.py#L3 / firwsord.py#L3 — ownership inversion (real data here #38)
• src/eegprep/functions/guifunc/qt.py (1292 LOC, 45 staticmethods) — ds006695 #17
• src/eegprep/functions/statistics/_core.py (1036 LOC) + __init__.py:6-9 documented import fragility — Creating documentation and update eegprep functions #30
• src/eegprep/plugins/ICLabel/pop_prop_extended.py (1376 LOC) — Add Claude Code GitHub Workflow #36
• src/eegprep/extension_catalog.py (1039 LOC, two concerns in one file) — Set up parity testing harness for agentic development #43, Add cart2topo coordinate conversion #44

Ask

Please confirm: (a) the 6-tranche sequencing is the right shape, (b) the 5 behavioral findings' policies above, (c) whether #2/#18/#41 should move to their natural phases. Once these are settled I'll start with Tranche 1 (one PR, ~10 findings, zero behavior change) and iterate.