Fix: save a copy of group ids in CommonReferenceRecording

[oaaij-gnahz]

Hi,

I found a potential bug and fix while using common_reference.

Problem scenario: In my original recording object, channel IDs are the same as channel indices. I use common_reference after a combination of channel_slice and split_by while keeping the original IDs. Then when re-loading my latest recording object from the JSON file, the ids_to_indices function of the CommonReferenceRecording object would raise error suggesting the channel ID is not found.

My guess for the cause: After channel_slice, the channel IDs and indices are different. When initializing CommonReferenceRecording, the groups property is directly modified to reflect indices. Then when re-loading from kwargs, it would run the function ids_to_indices again, but this time the input property groups reflects indices and it should really reflect IDs. This causes error on some occasion (in my case, split_by is called after channel_slice so that may complicate the IDs of each channel), although most of the usual cases would not elicit the error.

I upload my fix here, treating groups property similarly as ref_channel_ids. Would you take a look?

Thanks!

[alejoe91]

Thanks @oaaij-gnahz

I'm having a hard time understanding exactly how behavior is changed. It seems you have only renamed groups to groups_inds, or am I missing something?

[oaaij-gnahz]

Thanks @oaaij-gnahz

I'm having a hard time understanding exactly how behavior is changed. It seems you have only renamed groups to groups_inds, or am I missing something?

Hi @alejoe91
Thanks for the reply! I applied the changes you suggested.

I think the difference that this commit makes is: when the CommonReferenceRecording gets created, previously it saves the group indices to the kwargs of the object; now it saves the group labels to the kwargs. I think the latter is correct, because if I were to create the recording object from the kwargs in a JSON file, then I should know the group labels, which the init function of CommonReferenceRecording will convert to indices using ids_to_indices. Otherwise (before this change), ids_to_indices will receive indices instead of label/IDs as input, which may cause incorrectness. That is based on my understanding, and the error on my end did disappear after the change. Does that makes sense?

[alejoe91]

@oaaij-gnahz same name change should apply to all other groups_inds!

[alejoe91]

Yep, makes sense! I left another comment!

[oaaij-gnahz]

I applied the same name change to all other groups_inds. Apologies for the oversight. Thanks for pointing that out!