[Epic] Complete standalone EEGLAB parity - #174
Conversation
[Parity] Add core matrix contracts
# Conflicts: # .notes/implementation-notes.html
Complete clean_rawdata and FIRFilt parity
…tics [ICLabel] Complete viewprops diagnostics parity
…ompletion' into phase/160-dipfit-source-parity # Conflicts: # .notes/implementation-notes.html
Add standalone DIPFIT source-localization parity
…ompletion' into phase/161-study-pac-limo # Conflicts: # .notes/implementation-notes.html
Implement STUDY PAC and LIMO design workflows
…ompletion' into phase/162-large-dataset-storage # Conflicts: # .notes/implementation-notes.html # src/eegprep/plugins/clean_rawdata/pop_clean_rawdata.py
[Storage] Implement large-dataset storedisk semantics
Build standalone EEGPrep Sphinx manual and tutorials
…-hardening [Parity] Close final standalone evidence
|
Claude finished @suraj-ranganath's task in 10m 43s —— View job Code review
BlockingNone. Important1. 2. Clearing the reject field crashes grid search — Nits
Test gaps
EEGLAB parity notes (verify against reference — could not confirm without the MATLAB checkout)
Areas explicitly checked and found correct: time-frequency window centering / FFT normalization, |
|
🤖 Epic closeout detailsThis PR is stacked on PR #152. Please do not merge this PR until #152 has merged into Parent epic: #157, Constituent phases
What this epic adds
Verification evidencePhase 8 closeout verification on the integrated epic branch reported:
Visual parity evidenceGUI side-by-side images are attached in follow-up comments on this PR. The attached groups cover the GUI-producing phases in this epic:
Accepted non-goalsThese are explicit final-epic boundaries, not forgotten implementation work:
|
|
🤖 AttachmentsICLabel and viewprops GUI visual parity
DIPFIT GUI visual parity
FIRFilt GUI visual parity
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| probe = rotation @ np.asarray([0.0, 0.0, 1.0, 0.0]) | ||
| ry = np.arcsin(np.clip(probe[0], -1.0, 1.0)) | ||
| rx = -np.arctan2(probe[1], probe[2]) | ||
| rx_matrix = _dipfit_axis_rotation([rx, 0.0, 0.0]) | ||
| ry_matrix = _dipfit_axis_rotation([0.0, ry, 0.0]) | ||
| rz_matrix = np.linalg.inv(ry_matrix) @ np.linalg.inv(rx_matrix) @ rotation | ||
| z_probe = rz_matrix @ np.asarray([1.0, 0.0, 0.0, 0.0]) | ||
| rz = np.arcsin(np.clip(z_probe[1], -1.0, 1.0)) |
There was a problem hiding this comment.
🤖 Severity: important — homogenous2traditional recovers the rotation angles incorrectly.
The DIPFIT rotation built by traditionaldipfit (lines 162-166) has third column [cx*sy, sx, cx*cy] and second row [-sz*cx, cz*cx, sx]. The correct closed-form inverse is:
rx = arcsin(R[1,2])=arcsin(sx)ry = arctan2(R[0,2], R[2,2])=arctan2(cx*sy, cx*cy)rz = arctan2(-R[1,0], R[1,1])=arctan2(sz*cx, cz*cx)
This code instead computes ry = arcsin(probe[0]) = arcsin(cx*sy) (wrong whenever rx != 0) and rx = -arctan2(probe[1], probe[2]) = -arctan2(sx, cx*cy) (sign-flipped — a pure +x rotation comes back as -rx). The _dipfit_axis_rotation correction step is then fed the wrong rx/ry, so it cannot recover the input: traditionaldipfit(homogenous2traditional(M)) != M for non-trivial rotations.
Why it matters: this is a public helper exported in __all__; it silently returns a 9-parameter vector that does not reproduce the input transform. (No internal callers today, so low blast radius, but the result is wrong.)
Suggested fix — use the direct decomposition and drop the corrective machinery:
| probe = rotation @ np.asarray([0.0, 0.0, 1.0, 0.0]) | |
| ry = np.arcsin(np.clip(probe[0], -1.0, 1.0)) | |
| rx = -np.arctan2(probe[1], probe[2]) | |
| rx_matrix = _dipfit_axis_rotation([rx, 0.0, 0.0]) | |
| ry_matrix = _dipfit_axis_rotation([0.0, ry, 0.0]) | |
| rz_matrix = np.linalg.inv(ry_matrix) @ np.linalg.inv(rx_matrix) @ rotation | |
| z_probe = rz_matrix @ np.asarray([1.0, 0.0, 0.0, 0.0]) | |
| rz = np.arcsin(np.clip(z_probe[1], -1.0, 1.0)) | |
| rotation = unshifted @ np.linalg.inv(scale(scales)) | |
| rx = np.arcsin(np.clip(rotation[1, 2], -1.0, 1.0)) | |
| ry = np.arctan2(rotation[0, 2], rotation[2, 2]) | |
| rz = np.arctan2(-rotation[1, 0], rotation[1, 1]) | |
| return np.asarray([tx, ty, tz, rx, ry, rz, *scales], dtype=float) |
Add a roundtrip test against traditionaldipfit with non-zero rx/ry/rz.
| reject = result.get("reject", "") | ||
| count = component_count(EEG) | ||
| components = one_based_indices(select, limit=count, default_all=True) | ||
| out = run_gridsearch(EEG, component=components, xgrid=xgrid, ygrid=ygrid, zgrid=zgrid, reject=reject) |
There was a problem hiding this comment.
🤖 Severity: important — clearing the "Rejection threshold RV(%)" field crashes grid search.
When the GUI field is cleared, reject = result.get("reject", "") yields "" (line 93) and is passed straight into run_gridsearch(..., reject=""). dipfit_gridsearch only guards if reject is not None: (_fitting.py:77), so "" passes through to dipfit_reject(models, "") → _threshold_fraction("") → float(np.asarray("", dtype=float)...), which raises ValueError: could not convert string to float: ''.
Note line 103 already special-cases reject not in (None, "") for the history string, but the actual call on line 96 does not — so the default "40" works and only a cleared field bites.
Suggested fix — normalize blank to None before the call:
| out = run_gridsearch(EEG, component=components, xgrid=xgrid, ygrid=ygrid, zgrid=zgrid, reject=reject) | |
| out = run_gridsearch(EEG, component=components, xgrid=xgrid, ygrid=ygrid, zgrid=zgrid, reject=(reject or None)) |
(or treat reject in (None, "") as "no rejection" inside dipfit_gridsearch).
2 participants
Complete the final standalone EEGLAB parity epic across final audit, bundled plugin parity, DIPFIT/source workflows, STUDY/PAC, storage semantics, ICLabel diagnostics, Sphinx documentation, and closeout evidence. This is stacked on PR #152 and must not merge before #152.
Fixes #157