Fix aggregate_channels merging of channel ids with different string sub type #3740
Conversation
|
The error comes from the token limit : P |
| chan_id in parents_chan_ids for chan_id in self._channel_ids | ||
| ), "ChannelSliceRecording : channel ids are not all in parents" | ||
| is_channel_in_parent = [chan_id in parents_chan_ids.tolist() for chan_id in self._channel_ids.tolist()] | ||
| assert all(is_channel_in_parent), "ChannelSliceRecording : channel ids are not all in parents" |
There was a problem hiding this comment.
Could be helpful to list them? Something like
channels_not_in_parents = [chan_id for chan_id in self._challen_ids.tolist() if chan_id not in parents_chan_ids.tolist()]
assert len(channels_not_in_parents) == 0, f"ChannelSliceRecording : channel ids {channels_not_in_parents} are not all in parent ids {parents_chan_ids}"
Or maybe this is totally overwhelming for large dense probes?
There was a problem hiding this comment.
I think it would be good to add the channel list in the error message
There was a problem hiding this comment.
Good idea. I will add in the next commit.
| assert list(aggregated_recording.get_channel_ids()) == ["0", "1", "2", "3", "4"] | ||
|
|
||
|
|
||
| def test_channel_aggregation_with_string_dtypes_of_different_size(): |
There was a problem hiding this comment.
Lovely test!
Would be nice to have a docstring with a brief description.
There was a problem hiding this comment.
Added a docstring. Let me know what you think.
|
Works for me, and looks less hacky than my proposed solution. Just made some doc-y comments. |
| ), "ChannelSliceRecording : channel_ids are not unique" | ||
|
|
||
| sampling_frequency = parent_recording.get_sampling_frequency() | ||
| assert sampling_frequency > 0, "Sampling frequency must be positive." |
There was a problem hiding this comment.
why not move it to BaseRecording.__init__?
There was a problem hiding this comment.
It is already there, I must have push tab with the new copilot feature. Removing.
4 participants
This will fix @chrishalcrow bug on #3733.
The current check for
all_channels_have_same_typeis too strict. I added a test.