Skip to content

[PAC] Classify PAC time-frequency helpers - #153

Merged
suraj-ranganath merged 2 commits into
feature/eeglab-core-parity-completionfrom
followup/148-pac-time-frequency-helpers
Jun 7, 2026
Merged

[PAC] Classify PAC time-frequency helpers#153
suraj-ranganath merged 2 commits into
feature/eeglab-core-parity-completionfrom
followup/148-pac-time-frequency-helpers

Conversation

@suraj-ranganath

Copy link
Copy Markdown
Member

Expose PAC and STUDY PAC entry points with clear unsupported limitations instead of untested analysis math, and tighten std_readpac for EEGPrep-owned pacdata caches with range slicing. Part of epic #131 and stacked under PR #152. Closes #148

@claude

claude Bot commented Jun 6, 2026

Copy link
Copy Markdown

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


Code review

  • Overall assessment: Safe to merge.
  • Highest-risk area: std_readpac range-slicing / kwargs tightening in std_readdata.py.
  • Merge recommendation: Safe to merge.

This PR classifies the PAC surface honestly: pac, pac_cont, std_pac, and std_pacplot become thin entry points that raise a shared NotImplementedError instead of shipping untested phase-amplitude-coupling math, while std_readpac gains real timerange/freqrange slicing over EEGPrep-owned pacdata caches. This matches the repo's established pattern for unported external surfaces (e.g. the LIMO entry points) and keeps familiar EEGLAB names discoverable without faking analysis behavior.

Blocking

None.

Important

None.

Nits

  • src/eegprep/functions/studyfunc/std_readdata.py:147-156pactimes/pacfreqs and the resulting masks are taken from first (the first group) and then applied to all groups via _slice_pac_cache. If groups ever carry heterogeneous axes, the others raise ValueError: PAC cache shape... rather than slicing per-group. This is reasonable defensive behavior for a shared-axis cache and is correctly shape-checked — just worth a one-line comment noting the "all groups share the first group's PAC axes" assumption so a future reader doesn't mistake it for a bug.

Test gaps

None blocking. Coverage is solid: stub NotImplementedErrors on sample_data, lazy-export identity (eegprep.pac is pac, etc.), cache read + 2-D slicing, missing-cache, malformed-shape, and unknown-option paths are all exercised. (Optional: the _range_mask "range filter does not include any cached measure samples" path isn't hit for PAC specifically, but it's shared, already-covered logic.)

EEGLAB parity notes

  • The external_dependency_skip / implemented reclassification in eeglab_core_parity_matrix.json is accurate: std_readpac genuinely reads deterministic EEGPrep-owned caches, while the PAC compute/plot helpers correctly stay skips since .icapac/.datpac sidecar semantics and PAC math are unported. user_facing_surface switching helper/apipython_api is consistent with sibling entries.
  • The tightened **kwargs rejection in std_readpac is a behavior change (previously silently ignored). Verified no existing caller breaks — tests/test_study_measures.py:259-267 only passes the named clusters/components params, and std_readpac has no other in-repo callers.

Checked correctness of the boolean-mask slicing (values[..., freq_mask, :][..., time_mask] over the (…, freq, time) layout), the malformed-shape guard, lazy/eager export wiring, and CLAUDE.md/AGENTS.md adherence (minimal stubs, docstrings present, docs + parity matrix updated). Note: I couldn't execute pytest in this sandbox (command approval restrictions), so test results above are from static analysis of the test cases against the implementation.

@suraj-ranganath
suraj-ranganath merged commit 00eeedc into feature/eeglab-core-parity-completion Jun 7, 2026
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