Add kilosort4 wrapper tests

[JoeZiminski]

This PR makes some refactorings and small changes to kilosort4.py (Kilosort4 Wrapper) and adds tests for the wrapper across all released versions.

Changes to kilosort4.py

• Changes the _default_params based on version to always match kilosort's DEFAULT_SETTINGS.
• All KS4 functions are called via kwargs rather than position arguments.
• Removed keep_good_only and scaleproc which I think were left over from earlier KS versions but are not implemented in KS4.
• KS4 .__version__ attribute is 4 for all versions < 4.0.10 so I used importlib.version for versioning here.

CI tests

• The CI tests use a python script to grab the versions of all kilosort released on pypi and set them in a matrix to run tests over, inspired by @tabedzki related PR on auto-building the docker images.
• Tests for running KS4 natively vs. through SpikeInterface wrapper are added (details on these in the module header). These are stored in .github/scripts so are not part of the test suite. The CI script can be adjusted to run on cron job only. I ran into problems with the module tagging in conftest.py because the script is not a submodule of the package src, so added a try/except. Maybe there is a better solution (e.g. running the script from a different location) happy to adjust this.

Some points for discussion

• Possible kwargs to fully expose: shift, scale, tmin, tmax, save_preprocesed_copy.
• docker is not tested, but I think as the docker code is not related to how failthfully the wrapper is, I think thats okay not to test it?
• save_extra_vars (KS4) is save_extra_kwargs in SpikeInterface, should it be renamed for consistency?

KIlosort versions <= 4.0.4

For some reason on the tests in CI, there are completely wrong results between SI wrapper vs. KS4 in 4.0.4. Also, versions <4.0.4 seem to be susceptible to a bug ValueError: negative axis 0 index: -2147483648 which looks like a KS4-side overflow error that's since fixed. Also, as all KS4 DEFAULT_SETTINGS changes that are a little tricky to handle (see below comment to Alessio) are <=4.0.4 I suggest we just don't support versions earlier than 4.0.5?

Some TODOs:

• KS4 next version will deprecate duplicate_spike_bins for duplicate_spike_ms, also mention save_preprocesed_copy typo.
• could test a multi-segment recording to check concatenation is OK.

[JoeZiminski]

(In quotes, Alessio original message ported from #3073)

[On shift and scale adding to BinaryFiltered] Honestly, I was a bit torn about including these two, simply because in > SI we have better scaling management, so why would someone that launch KS4 from SI want to use scale and shift there? >Maybe, let's just remove them!

Yes I can definitely see the advantage in not including everything, on balance though as a philosophy I think SpikeInterface, when it comes to sorters, is most effective as as a wrapper that exposes all available sorter options. In some ways it's also easier to maintain because avoids having to decide what is / isn't worthwhile to expose, and avoids potential future issues users might raise wanting things included. As such I'd probably advocate towards exposing shift, scale, tmin, tmax, save_preprocessed_copy as part of this PR.

[On setting default params in SpikeInterface side _default_params vs. KS4 DEFAULT_SETTINGS. That is potentially >a good idea, but I think that having the dictionaries "spelled" out is better for readibility and design and it complies with the >pattern with other sorters. Since we discussed about having automated CI for KS4 versions, we could throw in parameter >checks there ;)

Agree, for some reason I thought this was leading to some large differences in results but its not. They are the same for all versions except two cases where the defaults changed in early KS versions. I made the _param_defaults condition on the version in these cases, but this duplicates the cls.get_sorter_version() but I can see no way around this as the class is not instantiated yet. Anyways, glad its easy to handle!

[EDIT] Turns out it wasn't as easy to handle as I thought as the kilosort import is no longer wrapped at this stage and is causing general test failures. I am also getting weird errors on kilosort4 tests versions <= 4.0.4 (which is also where the default changes are). So I'm thinking just drop support for these versions, which also removes the different-defaults problem. If default change in future we could handle with a parameters getter on the Kilosort4Sorter class.

[carlacodes]

is there any easy way to change save_preprocessed_copy to true within the existing spikeinterface framework?

[JoeZiminski]

Hi @carlacodes unfortunately not, although I think it would be quick to add. @alejoe91 would you be happy to expose save_preprocessed_copy in the kilosort4 wrapper?

[alejoe91]

@JoeZiminski sure!

[JoeZiminski]

Hey @alejoe91 I think this is good to go now, I added a reminder based on the suggested plan here. The test will error out for spikeinterface >= 0.101.1, as a reminder to update it for supporting the new versions.