mne-tools / mne-nirs

Process Near-Infrared Spectroscopy Data in MNE
https://mne.tools/mne-nirs/
BSD 3-Clause "New" or "Revised" License
83 stars 35 forks source link

ENH: Allow setting ch_names #452

Closed larsoner closed 2 years ago

larsoner commented 2 years ago

It's nice to be able to remap the ch_names that get plotted. I'm not convinced this is the best API. Maybe instead we should have:

ch_names : str | dict
    Channel names to plot. Can be ``'numeric'`` (default) to use ``['1', '2', ...]``,
    ``'short'`` to use ``info['ch_names'][::2]`` (e.g., ``['S1_D2', 'S2_D1', ...]``),
    or a dict to provide a mapping from ``info['ch_names'][::2]`` (keys) to the
    desired names.

@rob-luke WDYT?

codecov[bot] commented 2 years ago

Codecov Report

Merging #452 (dfb56f5) into main (a83acab) will increase coverage by 0.01%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #452      +/-   ##
==========================================
+ Coverage   93.33%   93.34%   +0.01%     
==========================================
  Files          65       65              
  Lines        2670     2675       +5     
  Branches      340      343       +3     
==========================================
+ Hits         2492     2497       +5     
  Misses        120      120              
  Partials       58       58              
Impacted Files Coverage Δ
mne_nirs/visualisation/_plot_3d_montage.py 99.01% <100.00%> (+0.04%) :arrow_up:
mne_nirs/visualisation/tests/test_visualisation.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a83acab...dfb56f5. Read the comment docs.

rob-luke commented 2 years ago

I love it! I can see myself setting src_det_names=None and then using a dict to map S2_D3 = "Planum Temporale" and S5_D2 = "Inferior Frontal Gyrus" etc.

Would this allow new line characters and/or setting S7_D3 = "", i.e. removing channel names for certain channels?

larsoner commented 2 years ago

Removing definitely works. Newlines I'll have to check

rob-luke commented 2 years ago

Newlines I'll have to check

If not, no need to add it here, just make a new issue and we can tackle it later.

larsoner commented 2 years ago

Newlines work -- I used src_det_names=defaultdict(lambda: 'a\nb'):

Screenshot from 2022-03-01 19-13-13

So do you think the API should be as described at the top of the issue, or as it's implemented in the code?

rob-luke commented 2 years ago

So do you think the API should be as described at the top of the issue, or as it's implemented in the code?

I prefer as described at the top of the issue. I think that ch_names ="short" is much more approachable to a new user than lambdas or dict(zip(str(ii), name.split()[0]) for ii, name in enumerate(raw.ch_names[::2], 1)))

larsoner commented 2 years ago

Yes but shouldn't we teach them this beautiful one-liner :)

I'll push a fix tomorrow (EST)

larsoner commented 2 years ago

Ready for review/merge from my end @rob-luke

rob-luke commented 2 years ago

Wonderful, thanks @larsoner