Skip to content

Add support for volumetric source spaces#15

Merged
wmvanvliet merged 11 commits into
AaltoImagingLanguage:mainfrom
ruuskas:vol_src
Dec 2, 2025
Merged

Add support for volumetric source spaces#15
wmvanvliet merged 11 commits into
AaltoImagingLanguage:mainfrom
ruuskas:vol_src

Conversation

@ruuskas

@ruuskas ruuskas commented Nov 25, 2025

Copy link
Copy Markdown
Contributor

We haven't discussed this previously, but this came up when working with a student. Would you consider merging?

  • select_vertices_in_sensor_range, restrict_forward_to_vertices, restrict_src_to_vertices, and restrict_forward_to_sensor_range now work with volumetric source spaces.
  • added a simple unittest to verify the above

I have checked that all tests still run locally.

- select_vertices_in_sensor_range, restrict_forward_to_vertices, restrict_src_to_vertices, and restrict_forward_to_sensor_range now work with volumetric source spaces.
- added a simple unittest to verify the above

@wmvanvliet wmvanvliet left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Wonderful to see work on volume source spaces! I'm happy to add this.

However, some of this code doesn't really spark joy. My issue is with the pattern of constantly using an if-statement to check whether we have 1 or 2 hemispheres. This branching makes the code somewhat tedious to read and reason about. I would be happier if some of the branching was replaced by code that works on an arbitrary number of hemispheres, so the same code just works regardless of whether we area dealing with a surface or volume source space.

Comment thread conpy/forward.py Outdated
Comment thread tests/test_forward.py Outdated
Comment thread tests/test_forward.py Outdated
Comment thread tests/test_forward.py Outdated
@ruuskas

ruuskas commented Nov 26, 2025 via email

Copy link
Copy Markdown
Contributor Author

ruuskas and others added 8 commits November 29, 2025 22:14
Co-authored-by: Marijn van Vliet <w.m.vanvliet@gmail.com>
Co-authored-by: Marijn van Vliet <w.m.vanvliet@gmail.com>
Co-authored-by: Marijn van Vliet <w.m.vanvliet@gmail.com>
Co-authored-by: Marijn van Vliet <w.m.vanvliet@gmail.com>
@ruuskas

ruuskas commented Nov 29, 2025

Copy link
Copy Markdown
Contributor Author

This should now be better and possibly work with arbitrary number of source spaces.

@wmvanvliet

Copy link
Copy Markdown
Collaborator

Much cleaner!

@wmvanvliet wmvanvliet left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Very nice work @ruuskas. Thanks!

@wmvanvliet
wmvanvliet merged commit bbe80ec into AaltoImagingLanguage:main Dec 2, 2025
2 checks passed
@ruuskas

ruuskas commented Dec 2, 2025

Copy link
Copy Markdown
Contributor Author

Thanks @wmvanvliet!

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.

2 participants