Skip to content

ENH: Speed up forward computations for iterative fitting - #13407

Merged
larsoner merged 8 commits into
mne-tools:mainfrom
larsoner:fwd
Sep 8, 2025
Merged

ENH: Speed up forward computations for iterative fitting#13407
larsoner merged 8 commits into
mne-tools:mainfrom
larsoner:fwd

Conversation

@larsoner

@larsoner larsoner commented Sep 3, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Todo

  • Fix bug where points weren't checked for being in the interior of our spheres
  • Simplify code by making a _CheckInsideSphere with equivalent __call__ and query
  • Add more efficient iterative forward code
  • Get tests for sphere checking passing
  • Add tests for iterative forward code

cc @wmvanvliet

Relevant for #13074

wmvanvliet
wmvanvliet reviewed Sep 4, 2025
View reviewed changes

@wmvanvliet wmvanvliet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to have this implemented in its proper place! Some suggestions for clearer names.

Comment thread mne/forward/_make_forward.py Outdated
Comment thread mne/surface.py Outdated
Comment thread mne/surface.py Outdated
Comment thread mne/surface.py
Comment thread mne/surface.py Outdated
@larsoner
larsoner marked this pull request as ready for review September 4, 2025 15:51
drammock
drammock approved these changes Sep 5, 2025
View reviewed changes

@drammock drammock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushed a fix (hopefully) for pip-pre. Diff looks good, just one question/comment. Thanks @larsoner

Comment thread mne/forward/_make_forward.py Outdated
Comment on lines +992 to +994
src = _ensure_src(src).copy()
for s in src:
transform_surface_to(s, "head", self.mri_head_t)

@drammock drammock Sep 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see some lines in this method (such as these) that look similar/identical to stuff happening in _prepare_for_forward. I'm assuming you've already done what you can to DRY, and there's just enough that is different to make it hard/impossible to abstract out a helper func. But mentioning it here in case you missed the redundancy / just forgot to go back and refactor those bits.

@larsoner larsoner Sep 8, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay made it a little more DRY by adding a src._transform_to(...) at least! There were maybe half a dozen places it turns out we used that for s in src: transform_surface_to(s, ...) pattern

@drammock drammock mentioned this pull request Sep 8, 2025
Merged
@larsoner
larsoner enabled auto-merge (squash) September 8, 2025 14:32
@larsoner
larsoner merged commit bb8d760 into mne-tools:main Sep 8, 2025
32 checks passed
@larsoner
larsoner deleted the fwd branch September 8, 2025 15:46
@wmvanvliet wmvanvliet mentioned this pull request Sep 11, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wmvanvliet wmvanvliet wmvanvliet approved these changes
@drammock drammock drammock approved these changes
@agramfort agramfort Awaiting requested review from agramfort agramfort is a code owner
@dengemann dengemann Awaiting requested review from dengemann
@alexrockhill alexrockhill Awaiting requested review from alexrockhill

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@larsoner @wmvanvliet @drammock