Skip to content

ENH: Speed up forward computations for iterative fitting #13407

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Sep 8, 2025
Merged

ENH: Speed up forward computations for iterative fitting #13407

merged 8 commits into from
Sep 8, 2025

Conversation

@larsoner
Copy link
Member

@larsoner larsoner commented Sep 3, 2025 •
edited
Loading

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
Copy link
Contributor

@wmvanvliet wmvanvliet left a comment

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.

@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
Copy link
Member

@drammock drammock left a comment

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

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)
Copy link
Member

@drammock drammock Sep 5, 2025

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.

Copy link
Member Author

@larsoner larsoner Sep 8, 2025

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 dengemann is a code owner

@alexrockhill alexrockhill Awaiting requested review from alexrockhill alexrockhill is a code owner

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