[Epic] Fable 5 Findings — whole-codebase code-quality remediation#206
Conversation
Part of the Fable 5 Findings epic (#193); addresses issue #198. Standalone/runtime hygiene: - Move developer/parity tooling out of the installed wheel into tools/: visual_capture.py -> tools/visual_parity/, stage_comparison.py -> tools/eeg_bids/, iclabel_net_load_py_measures.py -> tools/iclabel/ (refactored to import the canonical ICLabelNet, removing the float64/float32 model drift). pop_reref_helper.py -> scripts/. - Make eeglabcompat import side-effect-free (lazy temp dir), so importing core modules no longer creates a temp directory at import time. - Resolve the packaged montages dir via importlib.resources instead of a __file__ dirname chain in pop_load_frombids. Dependencies & docs: - Drop the redundant eeglabio optional extra that duplicated the mandatory dependency. eeglabio stays required (used transitively via mne.export for .set writing); installation docs updated to match. - Remove conda-forge install instructions for this unpublished pre-release; make the EEGobj example in contributing.rst actually runnable. - Fix the README docs badge and correct CI branch triggers (main -> develop). Dead code: - Remove _currentset_list (menu_actions), _reject_unsupported_options (pop_newcrossf), the unused withindex parameter (pop_chansel), the custom_randperm no-op wrapper (runica; hoist `import time`), an unraisable except in eegrej, and the unread top-level STUDY['preclust'] seed (_study_utils).
Part of the Fable 5 Findings epic (#193); addresses issue #194. - Remove assertion-swallowing try/except->skipTest wrappers across test_clean_artifacts, test_clean_drifts, test_eeg_picard, test_ICL_feature_extractor, and test_eeg_mne2eeg so assertions run instead of converting failures into skips. Genuine optional-dependency skips are kept as explicit @skipUnless guards. - Deduplicate the per-module create_test_eeg fixtures onto the shared tests/fixtures.py fixture. - Tighten the eeg_autocorr_fftw vs eeg_autocorr cross-check from rtol/atol=0.1 to 1e-4/1e-7 (documented: the only divergence is the single-precision FFT cast in eeg_autocorr). - Add a real (unmocked) calc_projector test that runs the actual sphericalSplineInterpolate and validates channel mapping/assembly. - Replace the ASR numpy-error-injection log-string tests with real degenerate-input tests asserting finite, sane cleaned output. - Assert real clustering behavior (shared labels within tight pairs, far point isolated) in the study clustering helper test. test_eeg_picard_different_random_states now xfails honestly: eeg_picard converges identically regardless of random_state (tracked under the epic for Phase 2/3); marked xfail rather than re-hidden.
…iants Part of the Fable 5 Findings epic (#193); addresses issue #195. Make side-effect-free functions actually side-effect-free by copying at entry, and fix the ICA sign-normalization invariant break. - Deep-copy EEG at entry in eeg_runica, eeg_amica, load_amica_model, eeg_picard, pop_select, pop_headplot, pop_clean_rawdata, clean_windows and clean_asr; copy the data array at entry in runica and clean_asr; build pop_saveset's MATLAB structures from copies so the caller keeps 0-based urchan/urevent; deep-copy the clean_artifacts high-pass snapshot. - eeg_rpsd no longer reseeds the global NumPy RNG (uses a local RandomState). - coords_to_mm / coords_RAS_to_ALS no longer mutate the caller's array. - eeg_from_data no longer silently transposes a tall 2-D array on a shape>shape guess; it raises and asks for nbchan when orientation is ambiguous. - posact sign-flips now negate the matching icaweights row and icawinv column directly instead of recomputing icaweights via pinv(icawinv), which had folded the non-identity sphere into the weights; icasphere is left untouched, preserving icawinv == pinv(icaweights @ icasphere) and icaact == icaweights @ icasphere @ data. eeg_amica and eeg_picard now flatten epoched data in Fortran order to match eeg_runica and EEGLAB. - Adds regression tests asserting inputs are unchanged and the ICA invariants hold.
Part of the Fable 5 Findings epic (#193); addresses issue #196. Replace silent fallbacks with real dispatch, make ignored options take effect or fail loudly, and fix index/boundary errors — each with a test that reproduces the bug first. - Loading: pop_loadset dispatches HDF5 vs scipy by the actual file instead of swallowing every error into an HDF5 retry; pop_fileio dispatches .mat by content instead of silently importing a broken .set as a raw array; pop_loadset_h5 drops the hard-coded emoji fixture and decodes by one rule. - Indexing/events: pop_select chantype selection no longer crashes (eeg_decodechan return unpacked) and the console zero-bases pop_select numeric channels so 1-based history replays correctly; eeg_eegrej no longer drops first/last events by latency; pop_editeventvals epoched latency display/edit is symmetric (ms<->points) and no longer raises. - sigproc: epoch() boundary check fixed (no negative-index slice); eegrej() shifts boundary latencies by base region spans; runamica() cleans its temp dir on failure without swallowing the error. - Session: console CURRENTSET reassignment guarded by a regression test; IMPLEMENTED_ACTIONS now has a test enforcing it matches dispatch routing; GUI dataset commit applies history only after the abort check; the headplot menu action commits its spline edit through the session. - CLI: pipeline gains the negative-edge filter guard the subcommand has; --quiet/--verbose/--no-progress are wired; the self-describing schema covers all required params and stops marking --output required under --overwrite; bids dispatches by format and propagates exit codes. - ASR/clean: clean_flatlines walrus precedence fixed; asr_calibrate raises on unsupported sample rates; clean_artifacts/clean_asr/asr_process no longer swallow failures and re-insert ignored channels per EEGLAB. - Other: newtimef _is_on uses correct whitelist semantics and its dead params fail loudly; compute_pac passes through title/vert/newfig; std_precomp recompute and std_makedesign delfiles take effect; EEGobj raises on unknown attributes; gen_derived_fpath honors ${root}; ICLabel validates icawinv before dereferencing it.
The mutation regression test snapshotted icaweights via list(), which breaks when the shared tests/fixtures.py fixture (Phase 1) provides a pre-ICA dataset with icaweights=None. Assert the caller's ICA fields are the exact same objects after the call instead, which is what 'does not mutate' means and works for both a None and an array-valued icaweights.
clean_asr now returns a fresh dataset; reword the three comments that still described it mutating EEG in place (AGENTS.md: delete stale comments in touched code).
|
Claude finished @suraj-ranganath's task in 7m 28s —— View job Code review — PR #206 (Fable 5 Findings epic)
I reviewed the full diff ( The vast majority of the −2337 lines are genuinely dead: BlockingNone. ImportantNone — but one doc gap is close to the line:
Nits
Test gapsNone that block. Coverage for the new behaviors is solid: EEGLAB parity notes
Checked correctness/regressions, EEGLAB parity, data-structure compatibility, changed-behavior tests, and AGENTS.md/CLAUDE.md compliance. |
# Conflicts: # tests/test_utils_asr.py
…ing) Part of the Fable 5 Findings epic (#193); addresses issue #197. Built on the integration of Phases 1/2/3/5 so the deduped code keeps those fixes. Behavior-preserving consolidations (existing tests pass unchanged): - studyfunc: extract the k-means kernel into _cluster_kmeans.py so pop_clust, optimal_kmeans and robust_kmeans import it downward (layering inversion fixed); share unique_preserving_order, trialinfo_rows, default_measure_target and the MEASURE_DATA_FIELDS map. - timefreq: newtimef/newcrossf use the canonical statistics.fdr and the shared bootstrap/threshold helpers instead of forked copies; _is_on imports the canonical whitelist predicate. - popfunc: remove the duplicated _is_on copies (import _pop_utils.is_on), pop_topoplot reuses the shared python_literal, pop_topochansel delegates to pop_chansel, pop_comperp collapses the repeated lowpass calls, pop_saveset serializes chanlocs through the single _chanlocs_to_struct_array, and the near-identical pop_rejkurt/pop_jointprob share their rejection plumbing. - gui/cli/extensions: menu_actions collapses the per-name dispatch branches; the pop-result interpretation contract is shared; the dead run_main() is removed; extension_catalog splits its manager and curation halves (extension_catalog_validation.py) and reuses the canonical entry-point helpers; plugin_menu derives bundled metadata from the registry. - plugins: firws/firwsord move to the firfilt plugin (correct ownership) with clean_rawdata importing them. Findings that would change observable behavior (figure titles, history strings, cluster partition/provenance, error strings) or need files outside this scope are intentionally left for a follow-up, as are the four large-file decompositions (pop_load_frombids, statistics/_core, pop_prop_extended, QtDialogRenderer) — those are tracked as Phase 4b.
|
🤖 Integration status update: all five phase PRs are merged into fable5/epic in the planned order (#200 → #202 → #203 → #201 → #204). I resolved the expected ASR test conflict by keeping all coverage and the fail-loud component-selection assertion, rebased #204 onto the composed epic branch, and fixed additional verified issues found during review: FDR now ignores non-finite p-values for the BH denominator while masking those positions false, clean_artifacts no longer mutates callers missing EEG['etc'], Picard maps seed to random_state, pop_newset help documents Edit description, and the final parity matrix validates against the committed snapshot when the vendored EEGLAB checkout is sparse. Local verification passed: ruff check, ruff format --check, ty check, ./pre-commit.py --changed-from origin/develop, focused integration suites, and EEGPREP_SKIP_MATLAB=1 uv run --no-sync pytest -m "not slow" (2023 passed, 249 skipped, 16 deselected, 1 expected xfail). |
|
🤖 Final integration status: refreshed CI is green after normalizing BIDS derived output paths for Windows. All checks now pass on PR #206: documentation build, pre-commit, Ruff/ty, Ubuntu Python 3.10/3.11/3.12, macOS Python 3.12, and Windows Python 3.12. Merge state is CLEAN. |
2 participants
Complete the Fable 5 code-quality remediation epic by merging phases #200, #202, #203, #201, and #204 into fable5/epic. Adds integration fixes for FDR non-finite parity, clean_artifacts copy-on-entry, Picard seed mapping, pop_newset help, and final parity matrix validation. This is the advisor-gated umbrella PR to develop and supersedes #199.
Part of #193