ENH: Don't require specific order for fNIRS

[larsoner]

• Push commit that should be green that does some refactoring to use zip(picks[::2], picks[1::2])
• Change _check_channels_ordered to return picks even if they aren't in matched pairs
• Add tests that operating on reordered channels is the same as original order:
  • optical density
  • SCI
  • TDDR
  • beer_lambert

cc @rob-luke I'll ping you when this is ready to go

[larsoner]

Okay @rob-luke I think this one is ready. I still think we could do some better refactoring of _check_channels_ordered, _channel_frequencies, _channel_chromophore, and _validate_nirs_info -- I think eventually ideally unifying within just _validate_nirs_info -- but for now this is probably good enough.

[rob-luke]

Wow!! That was quick @larsoner

I'm currently in transit to the US and won't be able to review this for a few days. When's the 1.1 release scheduled for? I'd love to review, but will I be holding things up?

[larsoner]

Tentatively the end of the month but we haven't really discussed it, so this week or next should work!

[larsoner]

Argh I removed a bunch of the nominal channel stuff and it broke hitachi... I'll have to fix it.

[larsoner]

@rob-luke this one is good to go

[rob-luke]

This is amazing @larsoner, thanks for tackling this.

Something we will want to support in the future is NIRS data with more than two frequencies of light. This is beyond the scope of this PR. But I wanted to raise it in advance incase it would change any of your design decisions here. I read the code with this in mind and nothing appears to block future extension.

With this PR can we now remove the following documentation?

mne-python/tutorials/io/30_reading_fnirs_data.py

Lines 28 to 35 in 5bddef8

	.. note:: To provide a consistent interface across different measurement
	devices and file types, MNE-Python uses a standard ordering of
	channels. MNE-Python internally orders channels by ascending source
	number. When there are multiple channels with the same source number,
	they are ordered by ascending detector number. The ordering of
	channels is done automatically when data is imported. Therefore,
	the ordering of channels within MNE-Python may be different to what
	was provided by the hardware vendor.

mne-python/tutorials/io/30_reading_fnirs_data.py

Lines 189 to 197 in 5bddef8

	# .. warning:: The channels must be ordered in haemoglobin pairs, such that for
	# a single channel all the types are in subsequent indices. The
	# type order must be 'hbo' then 'hbr'.
	# The data below is already in the correct order and may be
	# used as a template for how data must be stored.
	# If the order that your data is stored is different to the
	# mandatory formatting, then you must first read the data with
	# channel naming according to the data structure, then reorder
	# the channels to match the required format.

[rob-luke]

This argument is introduced but not used. I assume it is from an earlier implementation version and should be removed?

Suggested change

	def _channel_frequencies(info, *, throw_errors=True):
	def _channel_frequencies(info, *):

[larsoner]

Something we will want to support in the future is NIRS data with more than two frequencies of light. This is beyond the scope of this PR. But I wanted to raise it in advance incase it would change any of your design decisions here. I read the code with this in mind and nothing appears to block future extension.

I think in the long term we should abandon the channel-name-based stuff altogether. It occurred to me while making this PR that we currently only use 9 entries in ch['loc'] (right?), so we could maybe use the last three entries to store the pairing (somehow) and the frequency. Or maybe better (less redundant) we could probably use np.allclose on a scipy.spatial.pdist of the [ch['loc'][:9] for ch in chs] to pair automatically based on locations, then all we need to store is the frequencies in ch['loc'][9] or something.

But yes I think this is future compatible in the sense that this moves us toward one centralized place where pairings are identified and returned in a way that you can just operate on zip(picks[::2], picks[1::2]) and be operating on pairs. 👍

I'll push a commit to hopefully take care of the above comments then merge when green!

[larsoner]

Not enough sleep this weekend apparently... we already store the frequency, so maybe my next PR should be this automatic pairing based on chs['loc'] to avoid using channel names completely. Then people can name channels however they want in the future, which would be nice.

[larsoner]

In it goes, thanks for the quick review @rob-luke ! There might be some adjustments necessary at the MNE-NIRS end, do you have time to look there?

[rob-luke]

There might be some adjustments necessary at the MNE-NIRS end, do you have time to look there?

I do, will do today.

On another note @larsoner, NIRx have changed the way they store triggers in their latest software version. I have been sent a file with their new format. I will open a PR today for this too, so that we can get this in for 1.1.