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.

[alejoe91]

we use default: None elsewhere ;)

[JoeZiminski]

Thanks @alejoe91!! Though please ignore most of this PR ATM, it contains a lot of stray commits from random other PRs that are screwing everything up and I that need to drop! 😅

[alejoe91]

Ok! I'll refrain myself ;)

[JoeZiminski]

Ported from #3073

Chris

So, at the moment, if a user specifies skip_kilosort_preprocessing = True, then do_CAR is still True by default and is still applied. I guess: is this the expected behaviour?

And then what do we want to do if someone explicitly specifies skip_kilosort_preprocessing = True and do_CAR=True ?

So, at the moment, if a user specifies skip_kilosort_preprocessing = True, then do_CAR is still True by default and is still applied. I guess: is this the expected behaviour?

And then what do we want to do if someone explicitly specifies skip_kilosort_preprocessing = True and do_CAR=True ?

Alessio

So, at the moment, if a user specifies skip_kilosort_preprocessing = True, then do_CAR is still True by default and is still applied. I guess: is this the expected behaviour?

Chris

And then what do we want to do if someone explicitly specifies skip_kilosort_preprocessing = True and do_CAR=True ?

[JoeZiminski]

I think because do_CAR is the default True it is not possible to determine if the user wanted to apply CAR or just left the default on. In this case I think most users would expect skip_kilosort_preprocessing to skip all preprocessing and would be surprised if CAR was still performed by default. I would suggest to raise an error if skip_kilosort_preprocessing=False and do_CAR=True. It will be annoying for users first time because by default it will raise an error, but at least it is an easy fix and super explicit. If we get issues raised from people who want to skip KS preprocessing but keep CAR (I would imagine this is very small % of users) we could reconsider.

Further on this, I believe that the KS drift correction is performed in the code block that also performs whitening. So when whitening is turned off as part of skip_kilosort_preprocessing then drift correct is also turned off. This makes it even weirder for skip_kilosort_preprocessing to allow only CAR. Also (if this is correct) this behaviour can be added to the dosctring and I'd suggest raise an error if do_correction=True while skip_kilosort_preprocessing=True. Even though it will raise a lot of errors by default when trying to use skip_kilosort_preprocessing I feel it is quite a complex / confusing case and so best to be super explicit.

[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!

[alejoe91]

@JoeZiminski I would remove this, otherwise it will run on any open PR...no?

[JoeZiminski]

@alejoe91 oh yes good point, should this be a weekly cron job like here?

so just

on:
  workflow_dispatch:
  schedule:
    - cron: "0 12 * * 0"  # Weekly on Sunday at noon UTC

[alejoe91]

Yep!

[JoeZiminski]

Okay cheers @alejoe91, done!

[JoeZiminski]

Actually, there is a conflict since exposing save_preprocesed_copy, will fix now

[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.

[h-mayorquin]

Thanks for working on this!