-
Notifications
You must be signed in to change notification settings - Fork 20
Add support for volumetric source spaces #15
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.
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
Conversation
- 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
There was a problem hiding this comment.
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.
|
I agree on the code quality. This was something I quickly figured out on a
flight but I can try to think of a more elegant way to implement this now
that it works. So still WIP.
|
Co-authored-by: Marijn van Vliet <[email protected]>
Co-authored-by: Marijn van Vliet <[email protected]>
Co-authored-by: Marijn van Vliet <[email protected]>
Co-authored-by: Marijn van Vliet <[email protected]>
|
This should now be better and possibly work with arbitrary number of source spaces. |
|
Much cleaner! |
wmvanvliet
left a comment
There was a problem hiding this comment.
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!
|
Thanks @wmvanvliet! |
We haven't discussed this previously, but this came up when working with a student. Would you consider merging?
I have checked that all tests still run locally.