mne-tools / mne-nirs

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

Correct regression in time base quality metrics #531

Closed alexk101 closed 6 months ago

alexk101 commented 7 months ago

In previous version of this software, there used to be some assumptions about the ordering of channels. This was removed at some point in the main mne package, but the changes were never propagated here. This commit corrects this behaviour and attempt to follow the structure of upstream changes.

Reference issue

Fixes #530 .

What does this implement/fix?

Previous version made the assumption that the different wavelengths for a channel were adjacent. This was changed in the upstream version of mne-python and never updated here. This caused some kinds of data to be parsed incorrectly, leading to missing entries in time base quality functions. This corrects this incorrect behavior.

Additional information

At the present, I haven't made any changes to the testing of these functions, as it is a faulty assumption to say that there will never be zero entries for a channel. More thought should be given as to how this is tested by a maintainer to avoid future regression, though this fix should prevent any regardless.

Additionally, the old order of channels being adjacent results in different behavior in the function

https://github.com/mne-tools/mne-nirs/blob/3ba42cd47bb4f5e6bc6c15d4677bb2b2177cb6ae/mne_nirs/visualisation/_plot_quality_metrics.py#L11

Channels used to be plotted with wavelength adjacent, as show below.

old_behaviour

The new behavior due to the previously mentioned upstream changes to channel ordering is shown below.

new_behaviour

Personally, I don't find this very ergonomic, as the channels with differing wavelength are no longer adjacent, even though their values are the same. This behavior isn't technically incorrect, but perhaps undesirable given the purpose of this being to evaluate signal quality (these are different datasets being plotted btw, but that isn't relevant to this point).

larsoner commented 6 months ago

Thanks @alexk101 !