Added bad channels for kilosort >= 4.0.14

[chrishalcrow]

Hopefully fixes #3310

Kilosort introduced bad_channels in 4.0.14; this PR includes it too.

[JoeZiminski]

Thanks @chrishalcrow! this looks good, but could we wait for #3276 to be merged? I think it's good to go, there are some other changes required for the newer version e.g. save_preprocesed_recording kwarg was renamed, maybe we can take care of all these here? @alejoe91

Also @alejoe91 I wonder if it's possible to merge #3085 then we can add a few new tests to cover the cases that have arisen since the new version. Maybe we can split these tests such that the lightweight ones that check the kilosort function kwarg orders (e.h. here) are included in the spikeinterface CI tests, and the heavier tests that check actual outputs are run elsewhere.

Further to the discussion here, I'm in two minds about reverting to positional kwargs, we can compensate elsewhere (e.g. testing kwarg order, testing outputs), but there is always the risk of tests diverging over the years. However, maintaining the KS4 tests and adding conditionals for each new version will a) be super messy b) may itself lead to bugs when it becomes very long and hard to maintain. So i'm not sure there is a good solution. For now I'm happy to either use positionals arguments and then test the argument order of the KS4 functions is as expected, but in general I think this wrapper will take a lot of care as the risk of bugs is very high considering pace of KS4 development and how much of the KS4 internals are exposed in the wrapper. In the short-medium term I think we should try and work with the KS4 team to introduce functions that skip preprocessing etc. into the KS4 codebase, if they are happy to. That way we only need to worry about wrapping the top-level function. I wonder what people think about this.

[alejoe91]

Thanks @chrishalcrow! this looks good, but could we wait for #3276 to be merged? I think it's good to go, there are some other changes required for the newer version e.g. save_preprocesed_recording kwarg was renamed, maybe we can take care of all these here? @alejoe91

Also @alejoe91 I wonder if it's possible to merge #3085 then we can add a few new tests to cover the cases that have arisen since the new version. Maybe we can split these tests such that the lightweight ones that check the kilosort function kwarg orders (e.h. here) are included in the spikeinterface CI tests, and the heavier tests that check actual outputs are run elsewhere.

Further to the discussion here, I'm in two minds about reverting to positional kwargs, we can compensate elsewhere (e.g. testing kwarg order, testing outputs), but there is always the risk of tests diverging over the years. However, maintaining the KS4 tests and adding conditionals for each new version will a) be super messy b) may itself lead to bugs when it becomes very long and hard to maintain. So i'm not sure there is a good solution. For now I'm happy to either use positionals arguments and then test the argument order of the KS4 functions is as expected, but in general I think this wrapper will take a lot of care as the risk of bugs is very high considering pace of KS4 development and how much of the KS4 internals are exposed in the wrapper. In the short-medium term I think we should try and work with the KS4 team to introduce functions that skip preprocessing etc. into the KS4 codebase, if they are happy to. That way we only need to worry about wrapping the top-level function. I wonder what people think about this.

What about dropping support for versions<4.0.15?

[alejoe91]

@JoeZiminski goot to merge #3085 first, but see my comment there

[JoeZiminski]

Oh yes of course, that's such a good solution, I wish I'd thought of that 😆

I'm not familiar with the current release cycle and not sure if it's feasible, but would it be possible to:

• merge Add kilosort4 wrapper tests #3085 (pending my response)
• make a patch release

Then we can say everything SI < 0.101.1 supports kilosort <= 4.0.12 and everything 0.101.1 (and onwards, for the time being) supports KS >= 4.0.14? (or maybe just 4.0.16, depending on what changes were made, if it is requested we can add in some backward compatibility stuff to extend down to 4.0.13 if needed, but I'd imagine most people would be happy to start with 4.0.16).

[chrishalcrow]

(...but I'd imagine most people would be happy to start with 4.0.16).

I agree with this; and support us not-supporting old versions of kilosort 4.0.x.

[zm711]

I will also throw my weight behind not supporting old versions of KS4. It seems like they are still actively working on bugs, which means that supporting old versions are just us supporting buggy versions. This CUDA error seems important to users so I would says something like only supporting the minor releases maybe (let's see how they do with minor releases) instead of supporting each point release (maintain only the current point release for any particular minor).

[alejoe91]

Ok, to sum up we should:

• add a check for KS>=4.0.16 and raise an error for older versions. @chrishalcrow can you take care of that?

• modify the CI action for testing against old KS versions and only test against newer versions. @JoeZiminski can you do this?

• update KS4 docker version to 4.0.16 (I can take care of this!)

Sounds good?

[chrishalcrow]

This branch has conflicts after #3276 was merged, and our new solution goes beyond what this PR is about, so I'll close it and start a new PR.

[JoeZiminski]

Thanks this sounds good @alejoe91, can we also make a patch release (before merging those PRs) of SI 0.101.1 so it is easy to differentiate when KS < 4.0.16 was supported?