Skip to content

Implement STUDY PAC and LIMO design workflows - #170

Merged
suraj-ranganath merged 3 commits into
feature/eeglab-full-standalone-completionfrom
phase/161-study-pac-limo
Jun 7, 2026
Merged

Implement STUDY PAC and LIMO design workflows#170
suraj-ranganath merged 3 commits into
feature/eeglab-full-standalone-completionfrom
phase/161-study-pac-limo

Conversation

@suraj-ranganath

Copy link
Copy Markdown
Member

Add standalone PAC computation and STUDY PAC cache/read/plot workflows, plus LIMO-compatible design-matrix export and explicit source/PAC limitation boundaries. Update Phase 4 parity rows, docs, help resources, and regression coverage for advanced STUDY workflows.

Closes #161

@claude

claude Bot commented Jun 7, 2026

Copy link
Copy Markdown

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


Code review

  • Overall assessment: Safe to merge — solid, well-structured implementation. No blocking issues found.
  • Highest-risk area: Numerical parity of the new pac/pac_cont/STUDY PAC math (covered only by shape/finiteness assertions, not value-level checks).
  • Merge recommendation: Safe to merge after considering the test-gap note below.

I reviewed the full diff against origin/feature/eeglab-full-standalone-completion, traced the new compute_pac/compute_pac_cont data flow, the STUDY cache/read/plot round-trip, the LIMO design-matrix construction, and the neighbor/interp helpers. I verified that all imported helpers resolve (numeric_vector, component_activations, eeg_epoch_data, channel_labels, parse_key_value_args, is_on, and fdrFDRResult.mask). I could not run the test suite — pytest/uv commands required interactive approval in this environment, so correctness below is from static analysis only.

Blocking

None.

Important

None.

Nits

  • Duplicate _merged_chanlocs helperstd_interp.py and std_prepare_neighbors.py each define a byte-identical _merged_chanlocs. AGENTS.md lists "Parallel implementations of existing helpers" as a failure pattern and asks to search for an existing implementation before writing a utility. Consider hoisting it into _study_utils.py (where as_alleeg_list/ensure_study already live) and importing it in both modules.
    src/eegprep/functions/studyfunc/std_interp.py & src/eegprep/functions/studyfunc/std_prepare_neighbors.py

  • std_interp records only the last dataset's target_locstarget_locs is assigned per-iteration inside the dataset loop and then read once after the loop to populate study["etc"]["eegprep"]["std_interp"]["channels"]. When chans is a label/index selection and datasets have differing channel sets, the per-dataset _union_locs result differs, so the recorded metadata reflects only the final dataset's union rather than the requested set. When chans is None this is harmless (constant each iteration). Low impact (metadata only), but slightly misleading. Consider recording the requested channel set explicitly instead of the loop-final target_locs.
    src/eegprep/functions/studyfunc/std_interp.py (the study.setdefault(... )["std_interp"] block)

Test gaps

  • No value-level PAC assertion. test_pac_computes_frequency_pair_grid_for_epoched_data and the STUDY-cache test check shapes, dtype, and finiteness, and _coupled_trials() deliberately builds amplitude coupled to a 6 Hz phase — but nothing asserts the coupling is actually detected. Since parity of the modulation/correlation math is the whole point of porting pac, a single assertion that PAC magnitude at the coupled (amp≈30 Hz, phase≈6 Hz) pair exceeds an uncoupled pair (e.g. phase-shuffled or a non-coupled phase frequency) would turn these from "it runs" into "the math is meaningful" and guard against silent regressions in _pac_from_tfdata. Fix this →

EEGLAB parity notes

  • The PAC modulation formula in _pac_from_tfdata (mod = sum(amp * exp(i*phase)) over trials, no trial normalization) could not be checked against the EEGLAB oracle — pac.m/pac_cont.m are not present under src/eegprep/eeglab/, so I deferred on value-level parity. If a reference is available, a one-off MATLAB/Octave parity check (or a hand-computed expected value) for the mod and corrsin/corrcos branches would close this out. The explicit NotImplementedError boundaries for latphase, bootstrap significance, STUDY component PAC, source-dependent std_dipplot/std_dipoleclusters, and silent interp/rmcomps are clear and well-tested — good adherence to the "fail clearly at the boundary" convention.

The STUDY cache → std_readpacstd_pacplot round-trip, return_com history strings (validated with ast.parse in the tests), and the LIMO categorical/continuous/split-reg matrix construction all look correct and consistent with EEGPrep's in-memory measure contract.
· branch phase/161-study-pac-limo

@suraj-ranganath

Copy link
Copy Markdown
Member Author

🤖 Spec for the >500 LOC Phase 4 diff.

Problem: Phase #161 left advanced STUDY PAC/statistics/LIMO workflows classified as optional or missing in docs/parity/eeglab_final_parity_matrix.json:819, docs/parity/eeglab_final_parity_matrix.json:859, and docs/parity/eeglab_final_parity_matrix.json:891. EEGPrep had no real standalone std_pac/std_readpac/std_pacplot path, no practical pac/pac_cont implementation, and LIMO-compatible design generation was only an external-toolbox boundary.

Approach: Add real PAC computation in src/eegprep/functions/timefreqfunc/_pac_support.py:60 and src/eegprep/functions/timefreqfunc/_pac_support.py:164, wire public pac/pac_cont wrappers, build STUDY cache/read/plot workflows in src/eegprep/functions/studyfunc/std_pac.py:24, src/eegprep/functions/studyfunc/std_readdata.py:116, and src/eegprep/functions/studyfunc/std_pacplot.py:15, add LIMO design matrix export in src/eegprep/functions/studyfunc/std_limodesign.py:16, and add standalone neighbor/interpolation plus explicit source-backend boundaries in src/eegprep/functions/studyfunc/std_prepare_neighbors.py:17 and src/eegprep/functions/studyfunc/std_interp.py:18.

Key code:

result = compute_pac(
    phase_data,
    amplitude_data,
    srate,
    freqphase=freqphase,
    freqamp=freqamp,
    method=method,
    timesout=timesout,
)
group["pacdata"] = np.abs(result.pac)
group["pactimes"] = result.times
group["pacfreqs"] = result.ampfreqs

This is the core STUDY cache contract: downstream std_readpac/std_pacplot read EEGPrep-owned real PAC magnitude arrays rather than emulating external files.

Tests: Focused PAC and STUDY coverage is in tests/test_pac_time_frequency_helpers.py:76 and tests/test_study_measures.py:265. The parity row updates are covered by tests/test_eeglab_final_parity_matrix.py and tests/test_eeglab_parity_matrix.py, and GUI/help evidence remains covered by tests/test_visual_parity.py.

…ompletion' into phase/161-study-pac-limo

# Conflicts:
#	.notes/implementation-notes.html
@suraj-ranganath
suraj-ranganath merged commit b3e2932 into feature/eeglab-full-standalone-completion Jun 7, 2026
@suraj-ranganath
suraj-ranganath deleted the phase/161-study-pac-limo branch June 7, 2026 10:44
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