[Parity] Implement Phase 7 long-tail helpers - #150
Conversation
|
Claude finished @suraj-ranganath's task in 6m 53s —— View job Code review
I reviewed the diff against I checked imports/symbol resolution ( BlockingNone. Important
Nits
Test gaps
EEGLAB parity notes
|
|
🤖 Specification for the Phase 7 parity diff. Problem Approach Key code def _history_command(options: dict[str, Any], match_maps: np.ndarray) -> str:
pieces: list[str] = []
history_options = dict(options)
if "dataset" in history_options:
history_options.pop("dataset", None)
history_options.pop("alleeg", None)
history_options["matchcomps"] = match_maps
for key in ("dataset", "matchcomps", "corrthresh", "rejflag", "nomatcherror"):
if key in history_options:
value = history_options[key]
pieces.extend([format_history_value(key), format_history_value(value, cell_for_sequence=None)])
return "EEG = pop_findmatchingcomps(EEG, " + ", ".join(pieces) + ");"Tests |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| centered = values - np.mean(values, axis=0, keepdims=True) | ||
| std = np.std(values, axis=0, ddof=1 if values.shape[0] > 1 else 0) | ||
| std[std == 0] = np.inf | ||
| return np.sum(centered**4, axis=0) / (std**4) / values.shape[0] - 3.0 |
There was a problem hiding this comment.
Severity: important (EEGLAB parity)
This does not match EEGLAB's kurt.m, which uses population (biased) moments:
data = mean(data.^4) ./ (mean(data.^2).^2) - 3; % == N*sum(x^4)/sum(x^2)^2 - 3Here np.std(..., ddof=1) uses the sample variance (/(N-1)), so the result is scaled by ((N-1)/N)**2 relative to EEGLAB — a systematic deviation (small for large N, larger for short windows). Since this is a parity port, the moment estimator should match exactly.
| centered = values - np.mean(values, axis=0, keepdims=True) | |
| std = np.std(values, axis=0, ddof=1 if values.shape[0] > 1 else 0) | |
| std[std == 0] = np.inf | |
| return np.sum(centered**4, axis=0) / (std**4) / values.shape[0] - 3.0 | |
| centered = values - np.mean(values, axis=0, keepdims=True) | |
| m2 = np.mean(centered**2, axis=0) | |
| m2[m2 == 0] = np.inf | |
| return np.mean(centered**4, axis=0) / m2**2 - 3.0 |
| bins = max(1, round(flat.size / 5)) | ||
| if flat.size == 0: |
There was a problem hiding this comment.
Severity: nit (EEGLAB parity)
EEGLAB's realproba.m defaults bins to 1000, not round(N/5). This only affects direct callers (the entropy_rej path always passes discret, default 1000), but for parity the default should match EEGLAB.
| bins = max(1, round(flat.size / 5)) | |
| if flat.size == 0: | |
| if bins is None: | |
| bins = 1000 |
| raw_data = data_3d(EEG) | ||
| data = raw_data.reshape(int(EEG.get("nbchan", 0) or raw_data.shape[0]), -1, order="F") | ||
| acts = eeg_getica(EEG) | ||
| acts_2d = acts.reshape(acts.shape[0], -1, order="F") | ||
| winv = np.asarray(EEG.get("icawinv", []), dtype=float) | ||
| if winv.ndim != 2 or winv.size == 0: | ||
| weights = np.asarray(EEG.get("icaweights", []), dtype=float) | ||
| sphere = np.asarray(EEG.get("icasphere", []), dtype=float) | ||
| winv = np.linalg.pinv(finite_matmul(weights, sphere)) | ||
| selected_chans = _selected_channels(data.shape[0], chans=chans, omitchans=omitchans) |
There was a problem hiding this comment.
Severity: important (EEGLAB parity / correctness)
data is built from all nbchan rows, but winv (from EEG.icawinv) and the component activations correspond to EEG.icachansind. EEGLAB's eeg_pvaf indexes the channel data through EEG.icachansind (EEG.data(EEG.icachansind,:)). When ICA was decomposed on a channel subset (e.g. excluding EOG/ref), winv has fewer rows than nbchan, so winv[selected_chans, :] and winv[np.ix_(selected_chans, art)] will index out of range or misalign with data_selected, producing wrong PVAF or an IndexError.
The current test only covers the full-channel identity case, so this gap isn't caught. Suggest restricting the channel data to EEG.icachansind before computing variance, and mapping selected_chans/omitchans relative to that index set, mirroring EEGLAB.
1 participant
Add Phase 7 legacy pop wrappers, ICA and rejection helper aliases, help resources, tests, and parity matrix triage for long-tail core EEGLAB coverage. Remaining time-warping, exact correct_mc, PAC, and STUDY long-tail items are tracked in follow-up issues #146-#149.
Fixes #138
Part of #131