BUG: Fix bug with fNIRS reordering #10630
Conversation
rob-luke
left a comment
There was a problem hiding this comment.
Thanks @larsoner. This approach is much nicer than the simple argsort that I did. However it is more complex and slightly opinionated. Could you please document the ordering in the fNIRS docs?
sorry you had to undo my test changes.
|
@larsoner thank you for the ping. I can't round trip the data as we don't ever read it back in ourselves. But in any case, we would never make an assumption about ordering on disk - we often have to resort channels based use case as per #10555 (comment). |
|
Pushed a commit to document the stuff that is opinionated/temporary. Hopefully later this week I can make a PR to remove any ordering requirement! In the meantime I'll merge once green to keep things moving, then update mne-tools/mne-nirs#463 |
|
Awesome. Thanks @larsoner. And I'm excited to see your solution to the whole issue! |
* upstream/main: (24 commits) Use less memory when loading EDF file (mne-tools#10638) BUG: fix ipython console accessibility after MNEQtBrowser in Spyder (mne-tools#10637) WIP: Fix mne.time_frequency.multitaper Nyquist adjustment slightly incorrect (mne-tools#10541) BUG: Fix bug with fNIRS reordering (mne-tools#10630) MNT: PyQt6 for pip pre 3.10 (mne-tools#10636) CI: Fix conda (mne-tools#10628) MRG: Update installer links to point to 1.0.3_0 (mne-tools#10622) [BUG, MRG] fix info write access (mne-tools#10626) [MRG] Fixed group_by titles in evoked_plot. (mne-tools#10618) [BUG, MRG] Replace image_interp with interpolation in topomap plot arguments (mne-tools#10617) MAINT: Fix timeout (mne-tools#10624) Simplify mne-tools#10619 (mne-tools#10620) Drop EEG rejection thresholds when replacing EEG with CSD channels (mne-tools#10619) Add get_montage support for fNIRS (mne-tools#10611) ENH: Improve make_head_surface options (mne-tools#10592) [ENH, MRG] Add citation for intracranial JOSS paper (mne-tools#10616) [ENH] Add sys info for mne-icalabel (mne-tools#10615) 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) ...
Fixes the ordering issue I mentioned in #10555. Desired orders were pulled from
maint/1.0where the test passes, and fails onmain(after #10555). These orders shouldn't ever really need to be read or modified so they are# noqa: E501'ed on long lines.I also restored a bunch of the modified test lines from #10555.
@rob-luke feel free to merge if you're happy
@samuelpowell you might want to reprocess your files if you have read-write round-tripped them after #10555 (e.g., after doing Beer-Lambert or other processing), especially because a naive string sort does not even produce the (presumably) desired result: