fix fnirs channel name cropping in topomap combination

[JohnGriffiths]

Reference issue

Resolves a bug discovered whilst reviewing #11064

@rob-luke @larsoner

What does this implement/fix?

Explain your changes.

While testing out the #11064 I was having a recurrent issue that was quite subtle and tricky to pin down:

Essentially, it looks like this line

mne-python/mne/viz/topomap.py

Line 170 in aef4966

overlapping_set = np.insert(overlapping_set, 0,

will lead to a cropping of the added channel name, if it is longer than the channel names of the other ones in the list, and consequently than the numpy array datatype will permit.

To properly reproduce this you would need to add a breakpoint at the above line, run the function, and monitor for when it attempts to combine channel names with differing lengths as described above. I don't have an example for that full use case, but here is the plain numpy for the scenario I have encountered, using the same variable names as the linked code:

import numpy as np

# Here are two examples of the channel name being added to the combined list,
# one that will work and one that will not work.
# In the mne code these correspond to  `chs[chan_idx]['ch_name']`
new_chname_bad = 'S10_D50 hbo'
new_chname_good = 'S5_D50 hbo'
# This list is produced one line above the incrimating line
# https://github.com/mne-tools/mne-python/blob/aef49669fe1bdf19221e03e85cf961671508e0bb/mne/viz/topomap.py#L168
overlapping_set = ['S5_D50 hbo', 'S9_D50 hbo']
overlapping_set_orig_goodchan = np.insert(overlapping_set, 0, (new_chname_good))
overlapping_set_orig_badchan = np.insert(overlapping_set, 0, (new_chname_bad))
print(overlapping_set_orig_goodchan)
>> ['S5_D50 hbo', 'S5_D50 hbo', 'S9_D50 hbo']

print(overlapping_set_orig_badchan)
>> ['S10_D50 hb', 'S5_D50 hbo', 'S9_D50 hbo']
## NOTE THE CROPPED CHANNEL NAME 'S10_D50 hb' - THIS IMMEDIATELY LEADS TO ERRORS IN SUBSEQUENT FUNCTION CALLS  

# This is the proposed fix

# ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
overlapping_set = np.array(overlapping_set).astype('<U20')  # The only line edit in this PR :) 
# ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

# ...repeating the above now

overlapping_set_fixed_goodchan = np.insert(overlapping_set, 0, (new_chname_good))
overlapping_set_fixed_badchan = np.insert(overlapping_set, 0, (new_chname_bad))
print(overlapping_set_fixed_goodchan)
>> ['S5_D50 hbo', 'S5_D50 hbo', 'S9_D50 hbo']

print(overlapping_set_fixed_badchan)
>>['S10_D50 hbo', 'S5_D50 hbo', 'S9_D50 hbo']

Additional information

Not sure if the fix proposed is the ideal; setting datatype to '<U20' is just something relatively minimal that appears to work.

TBH the fact that numpy insert will mess with element contents like it does above seems pretty brittle and surprising to me.

[welcome]

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

[agramfort]

I would even use lists here to avoid issues cause 20 might be too small too for some people

[drammock]

agree that the better fix is to keep overlapping_set as a list until after the insertion (list.insert() ought to work right?), and only then cast to array (if casting to array is even necessary?)

[JohnGriffiths]

Agree with this. I had been thinking the same thing, but had aimed to keep things as close as possible to original code.

Have updated the PR, now it aggregates a list in the loop, and converts to an array after the loop is finished.

[larsoner]

@JohnGriffiths looks reasonable, just two small remaining things:

1. We try to add a regression test with each fix when possible. We should adapt your MWE above into mne/channels/tests/test_channels.py a small new test_fnirs_renaming()
2. Changes should be accompanied by latest.inc update to let people know what was fixed

I'll push a quick commit for these so that we can get this change into 1.2, which we're releasing imminently

[larsoner]

@JohnGriffiths can you check the "allow edits by maintainers" box? Then I can push my commit and we can merge

[larsoner]

(or fetch my branch and upstream, locally merge / rebase with upstream/main, then cherry-pick larsoner@34a26ca)

[drammock]

@larsoner please merge this with [circle deploy] when it's ready.

[larsoner]

... let's get this in early 1.3 rather than last minute 1.2. No need to hold up the release process I think

[larsoner]

(we can always backport easily to a 1.2.1)