larsoner commented 2 years ago

I don't like that fold_channel_specificity doesn't always give an entry for each channel, instead silently returning tables that are empty sometimes. This PR adds support for going to the next-best entry in the distance matrix in the case that the best-matched source and best-matched detector pair (or its reverse) is not listed in the fOLD table.

Outdated / wrong ideas

However, while implementing this, I found something inconsistent. The `standard_1005` montage is loaded, then transformed from head to MRI. However, this montage is not in head coordinates until it's applied or you do `montage = mne.channels.montage.transform_to_head(montage)`. The fact that this worked so well up to this point thus strikes me as odd. For now I've added a `coord_frame` arg and `montage` arg so you can control these things a bit -- `coord_frame='mri'` is where you assume the montage is already in MRI coords and transform the `info['chs']['loc']` using fsaverage's trans. `coord_frame='head'` (new) calls `montage = transform_to_head(montage)` and doesn't transform `info['chs']['loc']`. Using the `montage='standard_1005', coord_frame='mri'` code I now get (instead of two empty entries out of the 52 channels): ``` /home/larsoner/python/labsn/fnirs_av/av.py:744: RuntimeWarning: No fOLD table entry for best matching source/detector pair P5/PO7 (RMS distance to the channel positions was 9.1 mm), using next smallest available src/det pairing P7/PO7 (RMS distance 29.6 mm). Consider setting your channel positions to standard 10-20 locations using raw.set_montage if your pair does show up in the tables. specs = mne_nirs.io.fold_channel_specificity( /home/larsoner/python/labsn/fnirs_av/av.py:744: RuntimeWarning: No fOLD table entry for best matching source/detector pair P6/PO8 (RMS distance to the channel positions was 8.1 mm), using next smallest available src/det pairing P8/PO8 (RMS distance 30.0 mm). Consider setting your channel positions to standard 10-20 locations using raw.set_montage if your pair does show up in the tables. specs = mne_nirs.io.fold_channel_specificity( ``` Changing to `montage='standard_1020', coord_frame='head'` I get a *lot* more misses (and using `standard_1005` almost all miss the 1005 locations and go to something approximate): ``` /home/larsoner/python/labsn/fnirs_av/av.py:744: RuntimeWarning: No fOLD table entry for best matching source/detector pair TP7/M1 (RMS distance to the channel positions was 16.1 mm), using next smallest available src/det pairing TP9/TP7 (RMS distance 19.5 mm). Consider setting your channel positions to standard 10-20 locations using raw.set_montage if your pair does show up in the tables. specs = mne_nirs.io.fold_channel_specificity( /home/larsoner/python/labsn/fnirs_av/av.py:744: RuntimeWarning: No fOLD table entry for best matching source/detector pair P5/PO7 (RMS distance to the channel positions was 16.0 mm), using next smallest available src/det pairing P7/PO7 (RMS distance 25.3 mm). Consider setting your channel positions to standard 10-20 locations using raw.set_montage if your pair does show up in the tables. specs = mne_nirs.io.fold_channel_specificity( /home/larsoner/python/labsn/fnirs_av/av.py:744: RuntimeWarning: No fOLD table entry for best matching source/detector pair O1/O9 (RMS distance to the channel positions was 15.6 mm), using next smallest available src/det pairing PO9/PO7 (RMS distance 35.4 mm). Consider setting your channel positions to standard 10-20 locations using raw.set_montage if your pair does show up in the tables. specs = mne_nirs.io.fold_channel_specificity( /home/larsoner/python/labsn/fnirs_av/av.py:744: RuntimeWarning: No fOLD table entry for best matching source/detector pair Iz/O10 (RMS distance to the channel positions was 16.4 mm), using next smallest available src/det pairing Iz/Oz (RMS distance 56.8 mm). Consider setting your channel positions to standard 10-20 locations using raw.set_montage if your pair does show up in the tables. specs = mne_nirs.io.fold_channel_specificity( /home/larsoner/python/labsn/fnirs_av/av.py:744: RuntimeWarning: No fOLD table entry for best matching source/detector pair Iz/O9 (RMS distance to the channel positions was 16.6 mm), using next smallest available src/det pairing Iz/Oz (RMS distance 58.7 mm). Consider setting your channel positions to standard 10-20 locations using raw.set_montage if your pair does show up in the tables. specs = mne_nirs.io.fold_channel_specificity( /home/larsoner/python/labsn/fnirs_av/av.py:744: RuntimeWarning: No fOLD table entry for best matching source/detector pair O2/O10 (RMS distance to the channel positions was 14.7 mm), using next smallest available src/det pairing PO10/PO8 (RMS distance 40.6 mm). Consider setting your channel positions to standard 10-20 locations using raw.set_montage if your pair does show up in the tables. specs = mne_nirs.io.fold_channel_specificity( /home/larsoner/python/labsn/fnirs_av/av.py:744: RuntimeWarning: No fOLD table entry for best matching source/detector pair P6/PO8 (RMS distance to the channel positions was 12.4 mm), using next smallest available src/det pairing P8/PO8 (RMS distance 25.0 mm). Consider setting your channel positions to standard 10-20 locations using raw.set_montage if your pair does show up in the tables. specs = mne_nirs.io.fold_channel_specificity( /home/larsoner/python/labsn/fnirs_av/av.py:744: RuntimeWarning: No fOLD table entry for best matching source/detector pair TP8/M2 (RMS distance to the channel positions was 14.5 mm), using next smallest available src/det pairing TP10/TP8 (RMS distance 19.7 mm). Consider setting your channel positions to standard 10-20 locations using raw.set_montage if your pair does show up in the tables. specs = mne_nirs.io.fold_channel_specificity( ``` @rob-luke I'm not sure the right thing to do here, but what appears to me to be the inconsistency in handling of coordinate frames here is bothering me. I thought by converting the montage to head coordinates things would work better, but it seems like they work worse...

Implements workarounds for https://github.com/mne-tools/mne-python/issues/10321 along the way

larsoner commented 2 years ago

... I'm also going to see if I can get the actual ideal 10-20 mapping for these data so I can use raw.set_montage(...) with fold_channel_specificity(raw, coord_frame='head', montage='standard_1020) and make sure I get the correct channel pairings.

larsoner commented 2 years ago

... okay I think I get why the mri coordinate business worked -- it's because standard_1020 is in MNI (fsaverage's MRI) coordinates, modulo a small scale-factor problem (https://github.com/mne-tools/mne-python/issues/10321). Tomorrow I should be able to simplify this PR under that assumption, thereby (hopefully) getting rid of the coord_frame business.

lgtm-com[bot] commented 2 years ago

This pull request fixes 2 alerts when merging ea1ba844149a677fcea010b81ecac0a82eb14d10 into 47e02da88286fef2502487e171bb07629e32f99a - view on LGTM.com

fixed alerts:

2 for Unused local variable

codecov[bot] commented 2 years ago

Codecov Report

Merging #445 (af1a11d) into main (455cd85) will increase coverage by 0.04%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #445      +/-   ##
==========================================
+ Coverage   95.98%   96.03%   +0.04%     
==========================================
  Files          65       65              
  Lines        2568     2646      +78     
  Branches      333      339       +6     
==========================================
+ Hits         2465     2541      +76     
- Misses         49       50       +1     
- Partials       54       55       +1

Impacted Files	Coverage Δ
mne_nirs/io/fold/_fold.py	`94.53% <100.00%> (+0.34%)`	:arrow_up:
mne_nirs/io/fold/tests/test_fold.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 455cd85...af1a11d. Read the comment docs.

larsoner commented 2 years ago

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

lgtm-com[bot] commented 2 years ago

This pull request fixes 2 alerts when merging a8a9d5a21361f4ae5eca49a9122198567f0b2c1f into 47e02da88286fef2502487e171bb07629e32f99a - view on LGTM.com

fixed alerts:

2 for Unused local variable

larsoner commented 2 years ago

FYI I have now used this with two different NIRx montages using the NIRx locations and one Hitachi montage using standard_1020 locations and all seems to be correct, both in terms of the nearest-electrodes detected, and then the substitute choices (with warnings) made when the pairing is not found in the fOLD files

rob-luke commented 2 years ago

Hi @larsoner

I have thought about this for a few days and have a few hesitations to raise. But I think these could be solved by adding an on_missing parameter.

First, my concerns. Having a channel location (midpoint between source and detector (SD)) that was identical would not necessarily indicate that the replacement channel was sensitive to the same brain regions as the original channel. This is because the sensitivity profile is based on the position of the sources and detectors, rather than the channel point. Some examples where an identical channel location would result in different sensitivity profiles:

If you have a source and detector that are placed very close to each other, these "short channels" will not be sensitive to any region of the brain, and only measure superficial signal changes. If a user had a short channel placed on C5, it could be replaced by a fOLD SD pair between T7 and C3. But the replacement channel would have a completely different profile to the original channel.
A similar situation would happen between long channels of 3 vs 5 cm. So if C3-T7 was replaced by C1-T7 then the depth profile, and thus anatomical sensitivity would be different.
Orientation of the sources and detectors affects the sensitivity to brain regions. For example, if you had a SD pair between FC5 and CP5 then the channel would be located around C5. If this SD pair was not in the fOLD data then it may be replaced with SD pair C3-T7, which would also have a channel around C5. But these may have different profiles.

Also just conceptually, if I query the fOLD data for a pair and that pair does not exist I would expect it to return nothing. Because there is nothing that matched my query. I know you throw a warning, but I have witnessed users glaze straight over warnings, and also sometimes the person writing the paper or doing the stats with the table isnt the same as who did the analysis.

However, I can see the benifit of this functionality for advanced users that understand the subtle difference and for quick checking. I would use this myself when exploring my data. So I think it should be an option.

So I suggest adding an on_missing parameter with the following options:

'empty' (default): Provides the current behaviour, returns nothing if there is nothing in the database that matches the query.
'nearest': Functionality from this PR. Takes the nearest channel if nothing matches the query.

I like that a warning is thrown. Will the fact that the data has been replaced by the nearest channel be marked somehow in the output table? Maybe add a request_source and requested_detector column with the original query? Or just an asterix on the source and detector name or something?

Thoughts? Am I overcomplicating this?

larsoner commented 2 years ago

What you say makes sense... I think I'll add a new param, and also new column entries for the nearest-channel in addition to the used-channels.

As far as the param name goes, perhaps missing=... rather than on_missing, since in MNE-Python it's pretty standard for on_* to just control whether an error, warning, or info is emitted rather than controlling behavior? interpolation='nearest' would be another option, with None being the main behavior of yielding potentially empty results. This would at least match how the channel-level interpolation works, which I guess is nice?

rob-luke commented 2 years ago

Yeah that sounds good. Thanks

larsoner commented 2 years ago

@rob-luke I took a stab at a variant of what we discussed, ready for another round of review

lgtm-com[bot] commented 2 years ago

This pull request fixes 2 alerts when merging 96bbeeb3fa7e581d2262c9e9cdc98c2443b8f607 into 47e02da88286fef2502487e171bb07629e32f99a - view on LGTM.com

fixed alerts:

2 for Unused local variable

lgtm-com[bot] commented 2 years ago

This pull request fixes 2 alerts when merging 35e8567c497927e5bc6214c8834543e8b1eca61b into 455cd8512b7d2b8b6a6a66609bfc64562041730e - view on LGTM.com

fixed alerts:

2 for Unused local variable

larsoner commented 2 years ago

Finally green!

lgtm-com[bot] commented 2 years ago

This pull request fixes 2 alerts when merging af1a11d9909a2a6162319628358c798cb5a08163 into 455cd8512b7d2b8b6a6a66609bfc64562041730e - view on LGTM.com

fixed alerts:

2 for Unused local variable

rob-luke commented 2 years ago

Thanks @larsoner this is an excellent addition and I look forward to using it myself.

mne-tools / mne-nirs

BUG: Ensure we always get at least some entry #445

Codecov Report