Bug introduced with added EEG default caps (in July 2022)

[rcassani]

Bug

Pipelines using the process process_channel_addloc() generated before 30-Jun-2022 may point to incorrect EEG default cap if executed with Brainstorm prior 30-Jun-2022.

Description

This bug was introduced with the commits f6b2140 and b2ce2e3, which added additional EEG default caps.

The process_channel_addloc() generates a list with the EEG defaults from the info retrieved by bst_get('EegDefaults') process_channel_addloc#L30. Then, when the pipeline script is generated, the index of the select default is saved (only the index), which would be OK if the list were static or templates were added at the end, but this is not the case after the above mentioned commits, as they added EEG default caps in the middle of the list.

Example

Script generated in May 2022:

% Process: Add EEG positions
sFiles = bst_process('CallProcess', 'process_channel_addloc', sFiles, [], ...
    'channelfile', {'', ''}, ...
    'usedefault',  48, ...  % ICBM152: ASA 10-05 343
    'fixunits',    1, ...
    'vox2ras',     1, ...
    'mrifile',     {'', ''}, ...
    'fiducials',   []);

In May 2022, indeed the index 48 made reference to the ICBM152: ASA 10-05 343 cap. In current Brainstorm, the index 48 points to the ICBM152: 10-20 19, and ICBM152: ASA 10-05 343 is located at ix=54.

Reproduce

1. Checkout Brainstorm at d218370 or before

2. Create a Pipeline with the process Import > Channel file > Add EEG positions, select ICBM152: ASA 10-05 343:

   % Process: Add EEG positions
   sFiles = bst_process('CallProcess', 'process_channel_addloc', sFiles, [], ...
       'channelfile', {'', ''}, ...
       'usedefault',  48, ...  % ICBM152: ASA 10-05 343
       'fixunits',    1, ...
       'vox2ras',     1, ...
       'mrifile',     {'', ''}, ...
       'fiducials',   []);
   

3. Checkout Brainstorm at b2ce2e3 or after

4. Add a checkpoint in process_channel_addloc.m#L166

5. The desired EEG default file is not ICBM152: ASA 10-05 343 but ICBM152: 10-20 19. The default ICBM152: ASA 10-05 343 is not at index 54, it has been shifted by 6 places (3 items added in the Collin27 dir and 3 items added in the ICBM125 dir).

Potential solutions

Some potential solutions with their pros and cons

1. For options of type combobox, export the name instead of the index. If needed, we can find the index from the list created in the process. Besides process_channel_addloc, there are other 5 process that use combobox inputs: process_adjust_coordinates, process_import_channel, process_snapshot, process_ft_timeliockstatistics and process_headmodel.

   • +1 The option will be a variable and not only a comment
   • -1 Pipelines generated before this fix, using this process will not work

2. When generating the pipeline for combobox we can export export value{1} (choice) and value{2} (all options)

   • +1 Index will be related to its own list
   • -1 Pipelines generated before this fix, using this process will not work
   • -1 The code will contain an extremely long cell array (around 130 elements)

   [EDIT] Option 2b could be export value{1} == 1 and value{2} a cell array with only the selected choice. This option would share have the same pro y the first con from above.

3. Hardcode bst_get('EegDefaults') to sort the EEG defaults, placing the new ones at the end of the list.

   • +1 Will work with previously generated pipelines
   • -1 Hardcoded, and need to be updated with any new EEG cap

I think the Solution 1 sounds good, but won't be back compatible, the user will need to update their pipeline, although the info to update is already there. For example, it will change from:
'usedefault', 48, ... % ICBM152: ASA 10-05 343
to
'usedefault', 'ICBM152: ASA 10-05 343'

What do you think of all of this?

[ftadel]

Hardcoding is too heavy.
The scripts using integers are necessarily buggy (either now or in the future), therefore they should be fixed.

I chose a lighter variant of your solution 1:
I changed the type of the option from "combo" to "combobox_label", with no support of the previous index version (since there is no way to know when it was generated). Now it returns an error when using the old version of the script, asking to edit the script.
This will be annoying for some people maybe, but better than having uncaptured errors.

Commit: 142ae20

For the other processes that use the "combobox" option type: the list is included in the process, so there is no similar risk.
Over the years, I am progressively replacing all the "combobox" with "combobox_label", whenever I make changes to the processes...

If I missed something and this is not a satisfying solution, please reopen the issue.