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

Enable fNIRS in coreg gui #10081

Closed
wants to merge 1 commit into from
Closed

Conversation

rob-luke
Copy link
Member

@rob-luke rob-luke commented Dec 3, 2021

Reference issue

Conversation started over in mne-tools/mne-nirs#419 and specifically this comment from @larsoner mne-tools/mne-nirs#419 (comment)

However, when I ran the coreg guy with fNIRS data no sensors were displayed.

What does this implement/fix?

Enables plotting of fNIRS sources and detectors in the coreg gui

Additional information

I am totally new to the whole coregistration concept. So please let me know if there is more I should look at.
Also I am new to guis, how do I test this change in the CI?

@rob-luke
Copy link
Member Author

rob-luke commented Dec 3, 2021

@GuillaumeFavelier @larsoner could you please review

@GuillaumeFavelier
Copy link
Contributor

Hi @rob-luke , I'm +1 to add support for fNIRS if there is a need for it. I just wonder how to integrate it. For now we have one checkbox for each element to display (HSP, HPI, EEG), should we add one for fNIRS? Or are those always associated with EEG? Also, do we want to display them by default?

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.

Looks like a good start, and a reasonable direction!

It needs a test. One easy way to test this might be to take one of our existing coreg functions and set_channel_types to change the EEG channels to fNIRS ones. On main you should see that ._actors['eeg_channels'] is None and here it's not None. If that hack doesn't work, using some fNIRS file from testing would also work. (Maybe this is just cleaner anyway.)

@larsoner
Copy link
Member

larsoner commented Dec 3, 2021

I thought about maybe having a separate fNIRS box for the weights for fNIRS, but really we should probably just change that to be "Sensors". In dig we don't have a separate entry for fNIRS dig points (they are called EEG even though they're not), and we might as well always show them if they're present I think, just like we do for EEG. fNIRS needs to be close to the scalp like EEG.

It'll get trickier with OPMs because you do typically want them up against the scalp, but the technology doesn't really require it (unlike fNIRS, where your signal will degrade much faster with distance from the scalp IIUC). But for now I think just adding fNIRS to what gets shown the same way EEG does is a good start.

sensor_opacity=self._defaults["sensor_opacity"],
orient_glyphs=self._orient_glyphs, surf=self._head_geo)
eeg_actors = eeg_actors["eeg"]
sens_actors = actors["eeg"]
sens_actors.append(actors["fnirs"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sens_actors.append(actors["fnirs"])
sens_actors.extend(actors["fnirs"])

This could deal with the issues with SetVisibillity()

@rob-luke
Copy link
Member Author

rob-luke commented Dec 3, 2021

Thanks @GuillaumeFavelier and @larsoner

I also see there is substantial and consistent work being done on the Coreg. Would it be better for this small change to be done via one of @GuillaumeFavelier PRs? Or maybe I should wait until development has stabilised? Or I should continue here? Any preference?

@larsoner
Copy link
Member

larsoner commented Dec 3, 2021

I also see there is substantial and consistent work being done on the Coreg. Would it be better for this small change to be done via one of @GuillaumeFavelier PRs? Or maybe I should wait until development has stabilised? Or I should continue here? Any preference?

I'm fine with you adding fNIRS support things here, I don't mind a bad rebase/merge or two along the way in #10015 at least :)

@larsoner
Copy link
Member

larsoner commented Dec 6, 2021

I don't mind a bad rebase/merge or two along the way in #10015 at least :)

Looks like it didn't even create a merge conflict. I think if you fix this then we can probably merge:

mne/gui/tests/test_coreg_gui.py:113: in test_coreg_gui_pyvista
    coreg = coregistration(inst=raw_path, subject='sample',
<decorator-gen-535>:24: in coregistration
    ???
mne/gui/__init__.py:195: in coregistration
    return CoregistrationUI(
mne/gui/_coreg.py:203: in __init__
    self._set_lock_fids(fid_accurate)
mne/gui/_coreg.py:223: in _set_lock_fids
    self._lock_fids = bool(state)
/usr/share/miniconda/envs/mne/lib/python3.9/site-packages/traitlets/traitlets.py:606: in __set__
    self.set(obj, value)
/usr/share/miniconda/envs/mne/lib/python3.9/site-packages/traitlets/traitlets.py:595: in set
    obj._notify_trait(self.name, old_value, new_value)
/usr/share/miniconda/envs/mne/lib/python3.9/site-packages/traitlets/traitlets.py:1219: in _notify_trait
    self.notify_change(Bunch(
/usr/share/miniconda/envs/mne/lib/python3.9/site-packages/traitlets/traitlets.py:1229: in notify_change
    return self._notify_observers(change)
/usr/share/miniconda/envs/mne/lib/python3.9/site-packages/traitlets/traitlets.py:1266: in _notify_observers
    c(event)
mne/gui/_coreg.py:352: in _lock_fids_changed
    self._set_sensors_visibility(self._lock_fids)
mne/gui/_coreg.py:605: in _set_sensors_visibility
    actor.SetVisibility(state)
E   AttributeError: 'list' object has no attribute 'SetVisibility'

@rob-luke
Copy link
Member Author

Closed in favour of #10122

@rob-luke rob-luke closed this Dec 12, 2021
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.

3 participants