Add DetectAndRemoveBadChannelsRecording and DetectAndInterpolateBadChannelsRecording classes

[chrishalcrow]

Part of plan to class-ifty all preprocessing steps.

This PR introduces the very verbose detect_and_interpolate_bad_channels function which will detect then interpolate bad channels, and detect_and_remove_bad_channels which will detect then remove bad channels.

The idea is that these can be used in a standard pipeline => can be chained like any other preprocessing step.

Users can now use

recording_with_bad_channels_removed = detect_and_remove_bad_channels(raw_recording)

and can chain with other steps:

preprocessed_recording = whiten(detect_and_interpolate_bad_channels(bandpass_filter(raw_recording)))

The user can specify the bad channels, to skip the detect step. Idea is that this argument can be used when you load from dict. NOTE: When you load a saved recording from dict, it will not recompute the bad channels. This is a good feature.

Most difficult thing was the kwargs. RemoveBadChannelsRecording shares much of the same signature as detect_bad_channels, and I didn't want to duplicate the default parameters, but did want to save them in the RemoveBadChannelsRecording._kwargs. Ended up using inspect from signature, as is also done in the motion module. Happy to discuss different implementations.

[JoeZiminski]

This looks good to me, few suggestions / questions. Really love this as it solves a big problem for me!

[JoeZiminski]

Is there anything to worry about channel ordering here? I guess not (and this is a question for ChannelSliceRecording tests anyways. But as I have no understanding of how the ordering works I thought worth asking 😆

[chrishalcrow]

The ordering of the channel ids?

[JoeZiminski]

erm like order of the channels on the recording itself (I'm not sure how exactly this is represented 😅 ). But like the default order when you do plot_traces without order_channel_by_depth

[alejoe91]

Let's figure out how to deal with precomputed kwargs before merging this

[samuelgarcia]

We need a strong refactoring to separate kwargs in 2 parts, the parametrsation and caching some heavy computation.
This is on my todo list since 3 years now...
Lets do it before.

[h-mayorquin]

We need a strong refactoring to separate kwargs in 2 parts, the parametrsation and caching some heavy computation. This is on my todo list since 3 years now... Lets do it before.

+1 on discussing this.

Sam mentioned it on the last meeting. Would be good to bring it back.

[JoeZiminski]

Just a quick thought on this, there is the interpolate_bad_channels class, but that takes a list of channel ids. Given the function in this PR (and in general) you might expect it to detect-and-interpolate bad channels. I wonder if it makes sense to change that to interpolate_channels() (similar to recording.remove_channels() and then have remove_bad_channels and interpolate_bad_channels as convenience functions to detect-and-remove. However, this might be a confusing change re: backwards compatibility.

[alejoe91]

Just a quick thought on this, there is the interpolate_bad_channels class, but that takes a list of channel ids. Given the function in this PR (and in general) you might expect it to detect-and-interpolate bad channels. I wonder if it makes sense to change that to interpolate_channels() (similar to recording.remove_channels() and then have remove_bad_channels and interpolate_bad_channels as convenience functions to detect-and-remove. However, this might be a confusing change re: backwards compatibility.

Yes it makes total sense @JoeZiminski

What about having two classes:

• DetectAndRemoveBadChannels (function detect_and_remove_bad_channels)
• DetectAndInterpolateBadChannel (function: detect_and_interpolate_bad_channels)

They are a bit verbose, but it binds the bad_channel_ids of the remove/interpolate part to the detection.

@chrishalcrow what do yo think?

[chrishalcrow]

Just a quick thought on this, there is the interpolate_bad_channels class, but that takes a list of channel ids. Given the function in this PR (and in general) you might expect it to detect-and-interpolate bad channels. I wonder if it makes sense to change that to interpolate_channels() (similar to recording.remove_channels() and then have remove_bad_channels and interpolate_bad_channels as convenience functions to detect-and-remove. However, this might be a confusing change re: backwards compatibility.

Yes it makes total sense @JoeZiminski

What about having two classes:

• DetectAndRemoveBadChannels (function detect_and_remove_bad_channels)
• DetectAndInterpolateBadChannel (function: detect_and_interpolate_bad_channels)

They are a bit verbose, but it binds the bad_channel_ids of the remove/interpolate part to the detection.

@chrishalcrow what do yo think?

I think the verboseness makes it very clear that the function is dong two things, which is great! Thanks for the suggestion @JoeZiminski

So you'd use detect_bad_channel when you're playing with parameters and getting things going, then use detect_and_remove_bad_channels once your pipeline is a bit more settled. I think it's a nice workflow.

[chrishalcrow]

Hello @JoeZiminski and @alejoe91 - I've added a detect_and_interpolate_bad_channels function. At the moment, this does a very similar thing to detect_and_remove_bad_channels but calls the InterpolateBadChannels class rather than ChannelSlice.

[JoeZiminski]

Nice! That looks great @chrishalcrow cheers this will be super useful. I had a quick look at the code and looks good, I've got a slight preference to refactor the shared code:

        # get the default parameters from `detect_bad_channels`, and update with any user-specified parameters.
        sig = signature(detect_bad_channels)
        updated_detect_bad_channels_kwargs = {k: v.default for k, v in sig.parameters.items() if k != "recording"}
        updated_detect_bad_channels_kwargs.update(detect_bad_channels_kwargs)

        if bad_channel_ids is None:
            bad_channel_ids, channel_labels = detect_bad_channels(recording=recording, **detect_bad_channels_kwargs)
        else:
            channel_labels = None

        self._main_ids = recording.get_channel_ids()

into a new function on detect_bad_channels.py which is shared between both, but not very strong on that.

[chrishalcrow]

Nice! That looks great @chrishalcrow cheers this will be super useful. I had a quick look at the code and looks good, I've got a slight preference to refactor the shared code:

        # get the default parameters from `detect_bad_channels`, and update with any user-specified parameters.
        sig = signature(detect_bad_channels)
        updated_detect_bad_channels_kwargs = {k: v.default for k, v in sig.parameters.items() if k != "recording"}
        updated_detect_bad_channels_kwargs.update(detect_bad_channels_kwargs)

        if bad_channel_ids is None:
            bad_channel_ids, channel_labels = detect_bad_channels(recording=recording, **detect_bad_channels_kwargs)
        else:
            channel_labels = None

        self._main_ids = recording.get_channel_ids()

into a new function on detect_bad_channels.py which is shared between both, but not very strong on that.

I've refactored (and moved) the gross updated_detect_bad_channels_kwargs code. I think the other bits are helpful to keep for understandability.

[chrishalcrow]

Hello, I think this is ready @alejoe91

[chrishalcrow]

Hello, I now really think this is ready. @alejoe91
Docs failing because of gin. None of my doc changes actually get run.

[alejoe91]

Suggested change

	rec_interpolated_channels = detect_and_interpolate_bad_channels(remove_channel_ids=bad_channel_ids)
	rec_interpolated_channels = detect_and_interpolate_bad_channels(recording=rec)

[alejoe91]

@chrishalcrow looks perfect! Just something of fin the docs and the merge conflict and ready to merge on my end