Skip to content

Conversation

@arsalanfiroozi
Copy link
Contributor

Uses this logic:

  1. Find the closest vertices in the structural mesh for each coordinate
  2. Find corresponding vertices in the target space using sphere files
  3. Use mapped vertices as the new position of the previous coordinates.

@gavinmischler
Copy link
Collaborator

Looks pretty good! A couple comments

  1. You will need to add an import statement for this new function into the localization.init.py` file so that it can be imported from the module
  2. It's difficult to write tests for this, but would it be possible to add a brief test here which demonstrates the use of this function and verifies that the output doesn't fail and is the desired shape (like if you give it (10,3) shape for input, the output is also (10,3) and no errors are produced)? You can take a look at this to see how to download fsaverage data within a test, and then you can just transform between like pial and inflated or something, since it doesn't matter.
inflated_coords = src_to_dst(coords, './fsaverage/surf/lh.pial', './fsaverage/surf/lh.sphere.reg', './fsaverage/surf/lh.inflated', './fsaverage/surf/lh.sphere.reg')

@arsalanfiroozi
Copy link
Contributor Author

I added the test and fixed the missed function call in the init file.

tree_elecs = cKDTree(lh_verts_sub)
_, mapping_indices_elecs = tree_elecs.query(coords, k=1)

print(f"#Electrodes in LH: {np.sum(mapping_indices_elecs < lh_threshold)}, RH: {np.sum(mapping_indices_elecs >= lh_threshold)}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fine to have a print statement, but can you add a verbose argument to the function then? verbose should be an int and if it's greater than 0 then do this print statement

Copy link
Collaborator

Choose a reason for hiding this comment

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

also set the default as verbose=0

@gavinmischler
Copy link
Collaborator

Just the one comment about a verbose argument, and then I think it looks good! The docs build is failing but I think it's an unrelated issue which I just solved with a separate PR, so just pull in the latest changes from main, add the verbose argument, and then we can make sure the docs build and all the other tests pass

@arsalanfiroozi
Copy link
Contributor Author

Thanks. Just added.

@gavinmischler
Copy link
Collaborator

Nice, make sure to add a docstring for the verbose argument. Then it looks good!

@gavinmischler
Copy link
Collaborator

Oh sorry one more thing to get it to render inside the docs. Can you add it to this .rst file using the ..autofunction command just like the conversion functions for MNI and fsaverage?

@arsalanfiroozi
Copy link
Contributor Author

arsalanfiroozi commented Jul 5, 2025

Oh, sorry a lot of details I am not aware of! Just did

@arsalanfiroozi
Copy link
Contributor Author

Gavin the failure is not because of my test. It's because of test_mni152_fsaverage_conversions that I guess I didn't change anything threre.

@arsalanfiroozi
Copy link
Contributor Author

My bad! I fixed it. Let's see if it now works

@gavinmischler
Copy link
Collaborator

No worries, super weird. Let's see if it works now

Copy link
Collaborator

@gavinmischler gavinmischler left a comment

Choose a reason for hiding this comment

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

Awesome! Merge it whenever you're ready!

@gavinmischler gavinmischler merged commit 4a7745f into naplab:main Jul 9, 2025
4 checks passed
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