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

MRG: Add support for NIRS to plot_topomap #7063

Merged
merged 16 commits into from
Nov 18, 2019

Conversation

rob-luke
Copy link
Member

Reference issue

Addresses issue #7057

What does this implement/fix?

Enables plotting of topomaps for NIRS data, specifically hbo and hbr signals.

@rob-luke
Copy link
Member Author

rob-luke commented Nov 14, 2019

The images display correctly but the code is throws the error...

RuntimeWarning: Channel names are not unique, found duplicates for: {'S1', 'S4', 'S5', 'S7', 'S3', 'S8', 'S6', 'S2'}. Applying running numbers for duplicates.

@codecov
Copy link

codecov bot commented Nov 15, 2019

Codecov Report

Merging #7063 into master will decrease coverage by <.01%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master    #7063      +/-   ##
==========================================
- Coverage   89.74%   89.74%   -0.01%     
==========================================
  Files         442      442              
  Lines       77786    77788       +2     
  Branches    12620    12621       +1     
==========================================
  Hits        69808    69808              
- Misses       5169     5170       +1     
- Partials     2809     2810       +1

@rob-luke
Copy link
Member Author

There is some difference in how the positions are plotted in the topomap vs sensor inset.
This is best viewed in the plot_joint image here.

The inset has the correct looking locations, but the topoplot seems to expand everything out to fill the head. @larsoner is this a known issue? Does it seem NIRS specific? Should I try and address it here?

@agramfort
Copy link
Member

agramfort commented Nov 15, 2019 via email

@larsoner
Copy link
Member

#3987 #5472 #6304

Maybe I'll see if I can make some headway on this today

@larsoner
Copy link
Member

(but not as part of this PR, don't worry about it here)

mne/viz/topomap.py Outdated Show resolved Hide resolved
mne/channels/channels.py Outdated Show resolved Hide resolved
@rob-luke rob-luke changed the title WIP: Add support for NIRS to plot_topomap MRG: Add support for NIRS to plot_topomap Nov 16, 2019
@rob-luke
Copy link
Member Author

@larsoner and @agramfort this PR is ready for review. Thanks

# The naming for nirs is very specific and should not be modified
clean_ch_names = info['ch_names']
else:
clean_ch_names = _clean_names(info['ch_names'])
Copy link
Member

Choose a reason for hiding this comment

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

_clean_names is used in a number of places. I would rather avoid the branching here to make sure it's consistent with other places. How specific are NIRS channel names? What ch name breaks it?

Copy link
Member Author

@rob-luke rob-luke Nov 16, 2019

Choose a reason for hiding this comment

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

The name given by the device manufacturer is in the format of source number then detector number, so for example “S1 D2” is the channel between source one and detector two. We then appended the light frequency or chomaphore. So currently the MNE channel names are in the format S1 D2 760.

The problem was that the spaces were getting removed and the first characters were being kept, so we would have several channel names that were modified to be “S1” for example (because source one may have channels to several detectors). And that caused the error listed in the first comment of this PR.

But we can change the way channel names are stored. We could make it S1-D2-760 Or S1D2-760 etc. I think this would solve the problem, but not sure if it will break other things. What do you think?

Copy link
Member Author

@rob-luke rob-luke Nov 16, 2019

Choose a reason for hiding this comment

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

Sorry that’s wrong, the format is currently S1-D2 760. Another example is S12-D3 850

https://github.com/mne-tools/mne-python/blob/master/mne/io/nirx/tests/test_nirx.py#L37

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I have push an attempt to change the naming to S1_D3 760. This way the source-detector pair is read as a single word, so stays unique without modifying clean_names. The raw.plot doesnt look as good, but I think thats ok. Thoughts?

@rob-luke rob-luke changed the title MRG: Add support for NIRS to plot_topomap WIP: Add support for NIRS to plot_topomap Nov 17, 2019
@rob-luke
Copy link
Member Author

@larsoner & @agramfort this is ready for another round of review. Thanks

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

besides these redundant computations LGTM

tutorials/preprocessing/plot_70_fnirs_processing.py Outdated Show resolved Hide resolved
@rob-luke rob-luke changed the title WIP: Add support for NIRS to plot_topomap MRG: Add support for NIRS to plot_topomap Nov 17, 2019
@larsoner
Copy link
Member

@rob-luke you have a conflict, can you rebase?

@larsoner
Copy link
Member

Example looks good, +1 for merge after conflict resolution

https://16844-1301584-gh.circle-artifacts.com/0/dev/auto_tutorials/preprocessing/plot_70_fnirs_processing.html

@larsoner
Copy link
Member

Resolved via merge commit, will merge once CIs come back happy

@larsoner larsoner merged commit 3091cac into mne-tools:master Nov 18, 2019
@larsoner
Copy link
Member

Thanks @rob-luke

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.

None yet

3 participants