-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Broader support of the SNIRF file format and enable reading GowerLab data #10555
Conversation
Thanks for reporting this issue @samuelpowell. @larsoner the file Sam shared is 4MB when compressed, but 24 when expanded. It will be stored in the testing repo as a non compressed file. But we distribute the data as a compressed file. So is it the compressed or raw data size that we want to minimise? |
Yes I can make a smaller file by removing some tiles, but I'd just like to finish up initial work on https://github.com/Gowerlabs/lumomat and issue a release, that way I can be sure that the test file has defined link to a version of the conversion script so that any future bugs or changes can be managed. I will endeavour to get this done by the end of tomorrow. Does that make sense? |
Makes sense. Thanks @samuelpowell |
Thanks @rob-luke, I'll come back to you with something more suitable for CI asap. |
@samuelpowell here is the current status of reading the Gowerlabs data Read the SNIRF file (using gowerlabs default output format, not mne-style)import mne
import os.path as op
raw = mne.io.read_raw_snirf("lumomat-1-1-0.snirf", preload=True)
# <RawSNIRF | lumomat-1-1-0.snirf, 216 x 274 (27.3 s), ~669 kB, data loaded> Visualise sensor locations# Set some useful values
subjects_dir = op.join(mne.datasets.sample.data_path(), 'subjects')
plot_kwargs = dict(subjects_dir=subjects_dir,
surfaces="brain", dig=True, eeg=[],
fnirs=['sources', 'detectors'], show_axes=True,
coord_frame='head', mri_fiducials=True) coreg = mne.coreg.Coregistration(raw.info, "fsaverage", subjects_dir, fiducials="estimated")
# This initialises the coregistration, but the transformation (coreg.trans) is just initialised to identity
mne.viz.plot_alignment(raw.info, trans=coreg.trans, subject="fsaverage", **plot_kwargs) This figure shows the fsaverage head and its landmarks as red, blue, and green diamonds. CoregistrationThere is a coregistration gui that can be used. But for simplicity and reproducibility I just use the command line: coreg.fit_fiducials(lpa_weight=1., nasion_weight=1., rpa_weight=1.)
mne.viz.plot_alignment(raw.info, trans=coreg.trans, subject="fsaverage", **plot_kwargs) The sensors now appear over the brain, rather than floating in space.
And indeed, the median distance has decreased. So scaling the generic MRI may be a useful step.
We now see that the data is well aligned to the brain space. # I find this new way of plotting a little easier to use, so you can also plot the sensors using...
brain = mne.viz.Brain('gowerlabsdemodata', subjects_dir=subjects_dir, background='w', cortex='0.5', alpha=0.3)
brain.add_sensors(raw.info, trans=coreg.trans, fnirs=['sources', 'detectors']) Apply this transformation to the underlying data structureTo be addressed in a follow up PR. Questions@samuelpowell does the position of these sensors look correct? Were they placed on the left front of the head in this formation? If so, I will request a review from the other devs. |
@@ -238,11 +259,12 @@ def test_snirf_nirsport2_w_positions(): | |||
# nirsite https://github.com/mne-tools/mne-testing-data/pull/86 | |||
# figure 3 | |||
allowed_distance_error = 0.005 | |||
distances = source_detector_distances(raw.info) | |||
assert_allclose(distances[::2][:14], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the ordering of the channels is now changed, I test specific source detector pairs, rather than an indexed channel.
@rob-luke bravo! |
Our caps have 'default' layouts which are digitised on a phantom of an appropriate size. I have asked as to which phantom would have been used for this cap, and will let you know here. But indeed I expect that it is simply a matter of scaling. |
Excellent news @samuelpowell . Thanks for getting back to me so quickly.
Great, we can address later how we want to deal with this. But from the example above we can see that its something that can already be done within MNE.
First we need to wait until mne-tools/mne-testing-data#96 is merged, then I will ping the others (and you Sam) for a review. |
I understand that @RJCooperUCL applies a simple affine transform with suitable results. It appears that When pinged I'll pull the PR and attempt to break your code, and will subsequently remove 'mne-style' output. There may be a delay as I am away until Thursday. Thanks again for this! |
doc/changes/latest.inc
Outdated
@@ -51,6 +51,8 @@ Enhancements | |||
|
|||
- Add ``'voronoi'`` as an option for the ``image_interp`` argument in :func:`mne.viz.plot_topomap` to plot a topomap without interpolation using a Voronoi parcelation (:gh:`10571` by `Alex Rockhill`_) | |||
|
|||
- Add support for reading data from Gowerlabs devices to :func:`mne.io.read_raw_snirf` (:gh:`10555` by `Robert Luke`_ and `Samuel Powell`_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samuelpowell are you ok with acknowledging your assistance here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that’s fine, thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rob-luke this should be :newcontrib:
and moved to the top of the new list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise looks reasonable but tests are failing
doc/changes/latest.inc
Outdated
@@ -51,6 +51,8 @@ Enhancements | |||
|
|||
- Add ``'voronoi'`` as an option for the ``image_interp`` argument in :func:`mne.viz.plot_topomap` to plot a topomap without interpolation using a Voronoi parcelation (:gh:`10571` by `Alex Rockhill`_) | |||
|
|||
- Add support for reading data from Gowerlabs devices to :func:`mne.io.read_raw_snirf` (:gh:`10555` by `Robert Luke`_ and `Samuel Powell`_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rob-luke this should be :newcontrib:
and moved to the top of the new list
mne/io/snirf/tests/test_snirf.py
Outdated
@@ -85,33 +106,33 @@ def test_snirf_basic(): | |||
# Test channel naming | |||
assert raw.info['ch_names'][:4] == ["S1_D1 760", "S1_D1 850", | |||
"S1_D9 760", "S1_D9 850"] | |||
assert raw.info['ch_names'][24:26] == ["S5_D13 760", "S5_D13 850"] | |||
# assert raw.info['ch_names'][24:26] == ["S5_D8 760", "S5_D8 850"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cruft, remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gross 😞 , thanks for finding this mess.
@larsoner @agramfort could you pleaser review, this is good from my end. |
@@ -459,6 +459,9 @@ def __init__(self, fname, saturated, preload=False, verbose=None): | |||
annot = Annotations(onset, duration, description, ch_names=ch_names) | |||
self.set_annotations(annot) | |||
|
|||
sort_idx = np.argsort(self.ch_names) | |||
self.pick(picks=sort_idx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum. This will make a copy of the data buffer but more importantly this is not consistent with the other readers which read the channels in the order they appear in the file on disk. I prefer to avoid doing magic in readers. Relevant functions down the road should deal with files with channels present in different orders, eg combine_evoked grand_average etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will make a copy of the data buffer
If preload=False
it won't because we can pick without loaded data now. This means people who really need to save this 2x memory can do read_raw_nirx(...).load_data()
instead. But really NIRS data tends to be pretty small so I don't think this will be an issue in practice.
more importantly this is not consistent with the other readers which read the channels in the order they appear in the file on disk.
I agree with this idea in principle, but it will require a big refactoring of our existing code, which assumes and requires that the frequency pairings are essentially properly "interlaced" to work correctly.
So to me I would like to add a comment # TODO: Remove this reordering
, which would give us some months to properly craft order-agnostic code, which I think will be a bigger undertaking involving coordination between MNE-Python and MNE-NIRS and changes in probably 5-10 public functions...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rob-luke I realized this is problematic. Let's say we have this arrangement with the more-or-less standard "logical channel numbering":
S1 -- 1 -- D1 -- 2 -- S2
| | |
3 4 5
| | |
D3 -- 6 -- S3 -- 7 -- D4
The native file will be written as ['S1_D1 760', 'S1_D2_850', 'S2_D1_760', 'S2_D1_850', 'S1_D3_760', 'S1_D3_850', ...]
, i.e., a pair ordering of [S1_D1, S2_D1, S1_D3, S3_D1, S2_D4, S3_D3, S3_D4]
, and people will naturally expect these pairs to be associated with channel numbers 1 through 7. But this pick(argsort(ch_names))
will result in a reordered pair ordering of [S1_D1, S1_D3, S2_D1, S2_D4, S3_D1, S3_D3, S3_D4]
, so channels 1-7 from before now show up in the order [1, 3, 2, 5, 4, 6, 7]
.
Rather than this pick(argsort(...))
it seems like it would make sense to reorder the channels such that the original "logical channel" ordering is preserved, but such that pairs are still immediately adjacent. We could start by making sure that the first N
channels are all fNIRS channels, and that they are paired. Non-fNIRS channels go after that in channels N+1:
. Sound good?
Eventually like we said before we could/should relax this by making the channel logic smarter everywhere, but that's a bigger undertaking. All we need to do to fix this immediate/lpressing "logical channel" reordering problem I think is .pick(a_better_order)
where a_better_order
comes from a few lines of pairing logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree that there is a standard logical channel ordering. Even for the trivial example which you illustrate I would assign the channel numbers differently to you.
You have assigned
- Channel 1: s1 d1
- Channel 2: s2 d1
- Channel 3: s1 d3
I would have thought the logical ordering was
- Channel 1: s1 d1
- Channel 2: s1 d3
- Channel 3: s2 d1
I also don't think that the channel number has any meaning. And it's the pair naming that should be used.
That's why I went for a simple sort. It will always be the same and can be easily described.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure it can differ by system or setup, but to me the acquisition system sets the "standard order" when it writes data to disk in some order. So the idea is just to respect the order as written to disk as much as possible until we can remove any ordering restrictions whatsoever.
FWIW we used to have this order problem with MEEG data, too, and removing it made the code cleaner in the end. So I'm looking forward to removing the paired-order restriction for fNIRS, as the cleanest long term solution is just to keep the order each manufacturer uses in the first place, or a user does with reorder_channels, etc. Currently reorder_chaanels is problematic for fNIRS and it shouldn't be!
What makes fnirs different from eeg for example ? Why is it more of a problem with fnirs ?
|
There are channel pairs that are always processed jointly in a standard pipeline that goes from intensity -> optical density -> chromophore. For example, the first four channels in a raw intensity could be
then these get converted to optical density (using their pairings), and then finally to chromophore (which are still paired) like
So it would be possible in principle to deal with a non-paired ordering for example by wavelength like
but in practice no manufacturer has done this (or something more random) so the code in many places checks/assumes channels are in pair order already. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough …
Thank you @agramfort and @larsoner |
* upstream/main: MRG: Add "highlight" parameter to Evoked.plot() to conveniently highlight time periods (mne-tools#10614) MRG: Allow to pass array of "average" values to plot_evoked_topomap() (mne-tools#10610) fix: snirf length units (mne-tools#10613) minor, doc: fix subplot titles in tutorial (mne-tools#10607) Display averaged time period in Evoked topomap title (mne-tools#10606) MAINT: Fix for pydata-sphinx-theme [skip azp][skip actions][circle deploy] (mne-tools#10605) DOC: report.add_html in tutorial (mne-tools#10603) Broader support of the SNIRF file format and enable reading GowerLab data (mne-tools#10555) MRG: Recommend mamba instead of libmamba for installation (mne-tools#10597) Fix dev documentation warning [skip azp][skip actions] (mne-tools#10599) FIX cmap (mne-tools#10593) [ENH, MRG] Add interpolate bridged electrodes function (mne-tools#10587)
In relation to the Gowerlabs LUMO output:
Also, the 'useful' ordering depends on how the data is used:
Given the above I would suggest that there is no 'correct' ordering, and that one will have to apply a permutation depending on use (as indeed is being done here for spectroscopy). I'm not yet familiar enough with the internal working of MNE / MNE-NIRS to suggest how that is best implemented, but if you have an ordered enumerated list of channels and their metadata (e.g. source index, detector index, and wavelength) then it's simple to sort as required. Doing so based upon the contents of a string descriptor may be more challenging. |
Reference issue
Fixes #10538
What does this implement/fix?
The result of these three improvements is that we can read files from the Gowerlab manufacturer.
Changes
np.argsort(raw.ch_names)
. This meant that many tests I had that used index had to have the indexs changed. However, I never address channels by index in my research code, I always useraw.pick(['some channel names'])
. Moving forward, I will use channel name indexing rather than int indexing in the tests I write.Additional information