Skip to content

Add standalone DIPFIT source-localization parity - #169

Merged
suraj-ranganath merged 3 commits into
feature/eeglab-full-standalone-completionfrom
phase/160-dipfit-source-parity
Jun 7, 2026
Merged

Add standalone DIPFIT source-localization parity#169
suraj-ranganath merged 3 commits into
feature/eeglab-full-standalone-completionfrom
phase/160-dipfit-source-parity

Conversation

@suraj-ranganath

Copy link
Copy Markdown
Member

Add native spherical DIPFIT fitting, coordinate transforms, electrode realignment, atlas loading boundary, leadfield generation, plotting, and batch/manual wrapper behavior for standalone workflows. Update help, docs, the parity matrix, visual evidence, and tests while keeping MRI/BEM/LORETA external-backend limitations explicit.

Closes #160

@claude

claude Bot commented Jun 7, 2026

Copy link
Copy Markdown

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


Code review

  • Overall assessment: Needs changes — one edge-case correctness bug (channel/map desync) plus a parity concern in the atlas loader. The core spherical fitting, coordinate transforms, dipplot, and history/console wiring look solid and well-tested for the equal-channel case.
  • Highest-risk area: prepare_forward_data channel↔ICA-map alignment.
  • Merge recommendation: Needs changes.

Blocking

None.

Important

1. prepare_forward_data desyncs electrode positions from ICA map rows when ICA ran on a channel subsetsrc/eegprep/plugins/dipfit/_fitting.py:161-168

map_rows = _map_rows_for_channels(EEG, maps, chansel, len(chanlocs))
positions = _channel_positions([chanlocs[index - 1] for index in chansel])
...
selected_maps = maps[map_rows, :][:, [component - 1 for component in selected_components]]
labels = [str(chanlocs[index - 1].get("labels") or index) for index in chansel]

When icawinv has one row per ICA channel rather than per chanloc (maps.shape[0] != len(chanlocs)), _map_rows_for_channels (line 383) keeps only the chansel channels that are present in icachansind, but positions and labels are still built from the full chansel. If any coordinate-bearing channel in chansel is not in icachansind (a common case — ICA fit on EEG channels while EOG/mastoid still carry locations), selected_maps ends up with fewer rows than positions. That feeds np.linalg.lstsq(leadfield, data) in _fit_positions_to_topography with mismatched shapes (leadfield is N×3 from positions, data is length M < N), which raises a ValueError instead of fitting.

Why it matters: this breaks gridsearch/nonlinear/multifit for any dataset where EEG.dipfit.chansel (defaulting to all located channels) is a superset of icachansind.

Suggested fix: derive the matching channel subset together with map_rows and use it for positions/labels, e.g. have _map_rows_for_channels also return the chansel entries it kept, then build positions/labels from that filtered list so all three stay row-aligned.

Fix this →

Nits

2. Redundant double threshold conversion in dipfit_gridsearchsrc/eegprep/plugins/dipfit/_fitting.py

dipfit_gridsearch calls dipfit_reject(models, _threshold_fraction(reject)), but dipfit_reject already applies _threshold_fraction internally. It happens to be idempotent for inputs in [0, 100], but it's confusing and would mis-scale a hypothetical RV% > 100. Pass reject through raw.

Test gaps

3. No coverage for icawinv rows ≠ chanloc count. _known_dipole_eeg sets icachansind = arange(nbchan), so every test exercises the maps.shape[0] == chanloc_count branch and never hits the desync in finding #1. Add a fit test where icachansind is a strict subset of the coordinate channels (e.g. drop a couple of located channels from ICA) and assert the gridsearch/nonlinear fit still succeeds.

EEGLAB parity notes

4. Possible one-voxel offset in load_afni_atlassrc/eegprep/plugins/dipfit/load_afni_atlas.py

voxel_centers = indices * int(downsample) + (int(downsample) - 1) / 2.0
homogeneous = np.column_stack([voxel_centers + 1.0, np.ones(indices.shape[0])])
xyz = homogeneous @ np.asarray(image.affine, dtype=float).T

image.affine (nibabel) maps 0-based voxel indices to world coordinates, so the + 1.0 (mimicking MATLAB/AFNI 1-based indexing) introduces a systematic ~1-voxel shift in the returned MNI points. Worth validating against a known atlas voxel before relying on these coordinates; if confirmed, drop the + 1.0. Lower confidence since this is an explicit, rarely-hit boundary helper.


Checked: spherical leadfield/fit math, coordinate transforms (mni2tal/headcoordinates/traditionaldipfit/warps), dipfit_reject empty-model contract, pop_* history/console commands, pop_dipplot MRI/quiver rendering and _mri import, the thin re-export wrappers, and EEGLAB-style external-limitation messaging. No import/reference errors found; the _mri module and all referenced helpers exist.
· phase/160-dipfit-source-parity

@suraj-ranganath

Copy link
Copy Markdown
Member Author

🤖 Specification for the Phase 3 DIPFIT/source-localization implementation.

Problem
EEGPrep exposed DIPFIT menus and limitation placeholders, but issue #160 required audit-approved standalone workflows instead of runtime dependence on EEGLAB, FieldTrip, or fake localization outputs. The missing behavior was concentrated in src/eegprep/plugins/dipfit/_fieldtrip_workflows.py:65, src/eegprep/plugins/dipfit/pop_dipplot.py:24, and absent coordinate/fitting helpers now added under src/eegprep/plugins/dipfit/.

Approach
src/eegprep/plugins/dipfit/_fitting.py:55 adds deterministic spherical leadfield grid search, nonlinear refinement, RV rejection, source-point leadfields, and outside-head rejection for practical standalone ICA dipole workflows. src/eegprep/plugins/dipfit/_coordinates.py:253 adds MNI/Talairach/SPM transforms and electrode realignment helpers. src/eegprep/plugins/dipfit/_fieldtrip_workflows.py:65 wires the native backend into pop_dipfit_gridsearch, pop_dipfit_nonlinear, pop_multifit, pop_dipfit_batch, pop_dipfit_manual, and pop_leadfield, while leaving MRI/BEM/LORETA source-analysis paths as explicit external-backend limitations.

Key code

def leadfield_matrix(electrodes: np.ndarray, sources: np.ndarray) -> list[np.ndarray]:
    """Return one average-referenced 3-column leadfield per source point."""
    source_points, _ = _as_positions(sources)
    return [_average_reference(_unit_moment_leadfield(electrodes, point)) for point in source_points]


def dipfit_gridsearch(
    EEG: dict[str, Any],
    component: Any = None,
    xgrid: Any = None,
    ygrid: Any = None,
    zgrid: Any = None,
    *,
    reject: float | None = None,
) -> dict[str, Any]:

Tests
Focused coverage in tests/test_dipfit_workflows.py:190 validates dialog defaults, GUI-prefill regression behavior, known synthetic spherical-source fitting, batch/manual aliases, leadfield generation, RV rejection, transform/realignment helpers, plotting options, explicit limitation paths, menu dispatch, and sample-data loadability. Verification also ran Ruff, ty, non-slow pytest, matrix tests with expected local EEGLAB-reference skips, visual parity captures, and GUI Agent inspection.

@suraj-ranganath

Copy link
Copy Markdown
Member Author

Attachments

DIPFIT visual parity screenshots

pop_dipfit_settings_side_by_side.png pop_dipfit_gridsearch_side_by_side.png
pop_dipfit_settings_side_by_side.png pop_dipfit_gridsearch_side_by_side.png
pop_dipfit_nonlinear_side_by_side.png pop_dipplot_side_by_side.png
pop_dipfit_nonlinear_side_by_side.png pop_dipplot_side_by_side.png
pop_multifit_side_by_side.png pop_leadfield_side_by_side.png
pop_multifit_side_by_side.png pop_leadfield_side_by_side.png
pop_dipfit_loreta_side_by_side.png pop_dipfit_headmodel_eegprep.png
pop_dipfit_loreta_side_by_side.png pop_dipfit_headmodel_eegprep.png

@suraj-ranganath

suraj-ranganath commented Jun 7, 2026

Copy link
Copy Markdown
Member Author

🤖 Validation correction: initialized the EEGLAB submodule in the phase worktree plus the matrix-covered nested references (clean_rawdata, clean_rawdata/manopt, dipfit, firfilt, ICLabel, ICLabel/viewprops, and tutorial_scripts). uv run --no-sync python -m tools.eeglab_final_parity_matrix --json now reports ok: true, 180 expected EEGLAB paths, and zero errors. uv run --no-sync pytest tests/test_eeglab_final_parity_matrix.py -q now passes with 10 passed. No source changes were needed for this correction.

@suraj-ranganath

Copy link
Copy Markdown
Member Author

🤖 Addressed the Claude review findings in 5a689c5.

  • Fixed ICA subset alignment in prepare_forward_data: _map_rows_for_channels now returns both selected ICA map rows and the kept chansel, and positions/labels are built from that filtered channel set so maps, electrodes, and labels stay row-aligned.
  • Removed the redundant reject-threshold conversion in dipfit_gridsearch; it now passes the raw reject value to dipfit_reject.
  • Added regression coverage for icawinv rows not matching chanlocs: the new test adds located non-ICA channels, keeps them in dipfit.chansel, and verifies gridsearch plus nonlinear fitting still succeed.
  • Confirmed and fixed the AFNI/NIfTI affine parity issue by removing the MATLAB-style +1 voxel offset before applying nibabel’s 0-based affine. Added a known-voxel atlas test for the returned MNI coordinates.

Validation after the fix:

  • uv run --no-sync pytest tests/test_dipfit_workflows.py -q -> 20 passed
  • uv run --no-sync ruff check .
  • uv run --no-sync ruff format --check .
  • uv run --no-sync ty check
  • uv run --no-sync pytest tests/test_eeglab_final_parity_matrix.py tests/test_runtime_eeglab_independence.py tests/test_visual_parity.py -q -> 36 passed, 42 subtests passed
  • uv run --no-sync python -m tools.eeglab_final_parity_matrix --json -> ok true, 180 expected EEGLAB paths, zero errors
  • EEGPREP_SKIP_MATLAB=1 uv run --no-sync pytest -m "not slow" -q -> 1797 passed, 234 skipped, 11 deselected
  • ./pre-commit.py --fix and ./pre-commit.py on staged files -> OK

…ompletion' into phase/160-dipfit-source-parity

# Conflicts:
#	.notes/implementation-notes.html
@suraj-ranganath
suraj-ranganath merged commit 0ef71b4 into feature/eeglab-full-standalone-completion Jun 7, 2026
@suraj-ranganath
suraj-ranganath deleted the phase/160-dipfit-source-parity branch June 7, 2026 10:42
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