Skip to content
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

FIX: missing channels/fiducials can be np.nan #11634

Merged
merged 5 commits into from
Apr 25, 2023

Conversation

mmagnuski
Copy link
Member

@mmagnuski mmagnuski commented Apr 18, 2023

This essentially fixes the problem I encountered in #11610 in the context of localizing iEEG channels and microelectrodes on a high-resolution CT scan (for you, GitHub: fixes #11610).
The problem was that all my channel positions localized in the ieeg GUI were NaN'ed when using apply_volume_registration_points to transfer point positions from high resolution CT space to MRI coords. One of the last steps in apply_volume_registration_points was using compute_native_head_t, and this function assumes that fiducials are not present only when at least one of the fiducial positons is None, while in my case they were al np.nan. So np.nan positions were treated as valid fiducial positions and used to NaNihilate my valid channel positions. :)

I assume here that using np.nan for no channel position information is now the default in mne (info objects created via mne.create_info have info['chs'][idx]['loc'] as all np.nans by default), and that compute_native_head_t should treat nans as no positios as well, but maybe that assumption is not correct (for example maybe mne.io._digitization._get_fid_coords should return None for fiducials with np.nan positions).

So this fixes the problem I was experiencing, but mabe some other fix would be preffered.

@larsoner
Copy link
Member

Yes nan should be treated as invalid. Any easy way to add a test here?

For fiducials usually I think we try to use None if they are not present rather than nan. But if that's a harder fix this one is already an improvement

@mmagnuski
Copy link
Member Author

I'll try adding a test today. I'll also take a look if it could be fixed so that fiducials are None.

@alexrockhill
Copy link
Contributor

Looks good, once the test gets added

@mmagnuski
Copy link
Member Author

sorry for the delay, I'll finish this during this weekend.

@mmagnuski
Copy link
Member Author

Here is the cause of the problem, the code below is a modified version of how ieeg gui saves channel positions into info:

import mne
import numpy as np
from mne.io._digitization import _get_fid_coords

channels = list('ABCDEF')
info = mne.create_info(channels, 1000, ch_types='seeg')

ch_pos = [info['chs'][ch_idx]['loc'][:3] for ch_idx in range(len(channels))]
montage_kwargs = dict(ch_pos=dict(), coord_frame='head')
for ch_idx, ch in enumerate(channels):
    montage_kwargs['ch_pos'][ch] = ch_pos[ch_idx]

new_montage = mne.channels.make_dig_montage(**montage_kwargs)
info.set_montage(new_montage)

# using _get_fid_coords on new_montage returns None:
fid_coords, coord_frame = _get_fid_coords(new_montage.dig, raise_error=False)
print(fid_coords)
print(coord_frame)

# {'nasion': None, 'lpa': None, 'rpa': None}
# None

# however when we get the montage back from info (after it was applied to it)
# we get some estimated fiducials - calculated from the channels somehow
# but when some channels do not have positions (are np.nan) these fiducials
# get calculated as np.nan too
recovered_montage = info.get_montage()
fid_coords, coord_frame = _get_fid_coords(recovered_montage.dig, raise_error=False)
print(fid_coords)
print(coord_frame)

# {'nasion': array([nan, nan, nan]),
# 'lpa': array([nan, nan, nan]),
# 'rpa': array([nan, nan, nan])}

# (FIFFV_COORD_HEAD)

I understand that channels are transformed into head coordinates when applying the montage, but I don't know how this is done when there are no fiducials and how the fiducials are calculated from channel positions then.
I can use the example above as a test unless you the way fiducials are calculated when recovering montage is incorrect (including np.nan for example).

@mmagnuski
Copy link
Member Author

Ok, it seems that _ensure_fiducials_head when calculating head radius from available channel positions, does not exclude NaN positions, I can fix that.

@mmagnuski
Copy link
Member Author

Ok, I pushed a fix. I'll later add a test based on the example I posted here earlier today. The fix I proposed first may not be necessary now, but it may be good to leave it just in case there are other ways to get NaN fiducials (then I'll add a test for it too).

@mmagnuski
Copy link
Member Author

I get unrelated test failures:

FAILED mne/forward/tests/test_make_forward.py::test_make_forward_solution_ctf[testing_data-openmeeg] - AttributeError: module 'openmeeg' has no attribute 'Geometry'
FAILED mne/forward/tests/test_make_forward.py::test_make_forward_solution_openmeeg[3] - AttributeError: module 'openmeeg' has no attribute 'Geometry'
FAILED mne/forward/tests/test_make_forward.py::test_make_forward_solution_openmeeg[1] - AttributeError: module 'openmeeg' has no attribute 'Geometry'

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

LGTM marking for merge when green, which it should be once #11644 lands and I update the PR :)

@larsoner larsoner enabled auto-merge (squash) April 24, 2023 18:30
@mmagnuski
Copy link
Member Author

ok, I'll add a whatsnew bugfix entry today.

@larsoner larsoner merged commit 57f5ce3 into mne-tools:main Apr 25, 2023
@larsoner
Copy link
Member

Ahh sorry I enabled squash+merge before seeing your comment. Feel free to do so in a follow-up PR @mmagnuski , and thanks for the fix!

larsoner added a commit to larsoner/mne-python that referenced this pull request Apr 25, 2023
* upstream/main: (152 commits)
  FIX: missing channels/fiducials can be np.nan (mne-tools#11634)
  use py3.10 in precommit config (mne-tools#11648)
  MAINT: Unify GH Actions pytest (mne-tools#11644)
  MRG: Rename "Discourse" link in top navigation to "Forum" [ci skip] (mne-tools#11649)
  ENH: Add support for Harmonic Field correction (mne-tools#11536)
  Add pre-commit (mne-tools#11541)
  BUG: Fix bug with paths (mne-tools#11639)
  MAINT: Report download time and size (mne-tools#11635)
  MRG: Allow retrieval of channel names via make_1020_channel_selections() (mne-tools#11632)
  Fix index name in to_data_frame()'s docstring (mne-tools#11457)
  MAINT: Use VTK prerelease wheels in pre jobs (mne-tools#11629)
  ENH: Allow gradient compensated data in maxwell_filter (mne-tools#10554)
  make test compatible with future pandas (mne-tools#11625)
  Display SVG figures correctly in Report (mne-tools#11623)
  API: Port ieeg gui over to mne-gui-addons and add tfr gui example (mne-tools#11616)
  MAINT: Add token [ci skip] (mne-tools#11622)
  API: One cycle of backward compat (mne-tools#11621)
  MAINT: Use git rather than zipball (mne-tools#11620)
  ENH: Speed up code a bit (mne-tools#11614)
  [BUG, MRG] Don't modify info in place for transform points (mne-tools#11612)
  ...
@mmagnuski
Copy link
Member Author

No, problem, I will. Thanks @larsoner! 🚀

larsoner added a commit to larsoner/mne-python that referenced this pull request Apr 25, 2023
* upstream/main: (117 commits)
  FIX: missing channels/fiducials can be np.nan (mne-tools#11634)
  use py3.10 in precommit config (mne-tools#11648)
  MAINT: Unify GH Actions pytest (mne-tools#11644)
  MRG: Rename "Discourse" link in top navigation to "Forum" [ci skip] (mne-tools#11649)
  ENH: Add support for Harmonic Field correction (mne-tools#11536)
  Add pre-commit (mne-tools#11541)
  BUG: Fix bug with paths (mne-tools#11639)
  MAINT: Report download time and size (mne-tools#11635)
  MRG: Allow retrieval of channel names via make_1020_channel_selections() (mne-tools#11632)
  Fix index name in to_data_frame()'s docstring (mne-tools#11457)
  MAINT: Use VTK prerelease wheels in pre jobs (mne-tools#11629)
  ENH: Allow gradient compensated data in maxwell_filter (mne-tools#10554)
  make test compatible with future pandas (mne-tools#11625)
  Display SVG figures correctly in Report (mne-tools#11623)
  API: Port ieeg gui over to mne-gui-addons and add tfr gui example (mne-tools#11616)
  MAINT: Add token [ci skip] (mne-tools#11622)
  API: One cycle of backward compat (mne-tools#11621)
  MAINT: Use git rather than zipball (mne-tools#11620)
  ENH: Speed up code a bit (mne-tools#11614)
  [BUG, MRG] Don't modify info in place for transform points (mne-tools#11612)
  ...
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.

ENH: improving high resolution CT iEEG electrode localization example
3 participants