Skip to content

Add read_kilosort4_motion function#4518

Open
alejoe91 wants to merge 18 commits intoSpikeInterface:mainfrom
alejoe91:read_ks_motion
Open

Add read_kilosort4_motion function#4518
alejoe91 wants to merge 18 commits intoSpikeInterface:mainfrom
alejoe91:read_ks_motion

Conversation

@alejoe91
Copy link
Copy Markdown
Member

and fix #4470

@alejoe91 alejoe91 added the sorters Related to sorters module label Apr 16, 2026
Copy link
Copy Markdown
Contributor

@galenlynch galenlynch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! This introduces two unrelated things: (1) a new read_kilosort4_motion helper that reconstructs an SI Motion object from KS4's ops.npy (yblk, dshift, batch_size, fs); (2) closes the kilosort file logger to fix #4470 (Windows file lock). However, it seems to have some bugs, and doesn't yet include tests for the added code.

Bugs

  1. np not imported at module level. The new function uses np.load, np.linspace, np.arange, but import numpy as np in this file only exists locally inside _setup_recording_arguments (line 173). Calling read_kilosort4_motion as-is will raise NameError. Needs a module-level import numpy as np.

  2. np.linspace(t_start + t_bin/2, t_end - t_bin/2) is missing num= — defaults to 50 samples regardless of displacement.shape[0]. The else branch gets the count right via np.arange(displacement.shape[0]); the if recording is not None branch silently builds a 50-long array and the interpolator will be garbage whenever the session doesn't happen to have 50 batches. Should be np.linspace(..., num=displacement.shape[0]).

  3. displacement = dshift + yblk is wrong. In KS4, dshift = imin * ops['binning_depth'] is the estimated shift per batch per block (shape (n_batches, n_blocks)), and yblk is the y-center of each block; both are assigned into ops at datashift.py:214-215. SI's Motion.displacement is the shift, not shifted position — see the interpolator in get_displacement_at_time_and_depth. Adding yblk injects a per-column offset equal to the block depth — the resulting "displacement" would be hundreds of microns instead of the few-µm drift. Should be displacement = dshift; spatial_bins_um = yblk is already correct.

Could you add a unit test with a synthetic ops.npy? That would catch (1) and (2) immediately

Other notes

  • Bare except Exception for the yblk/dshift check is lax — KeyError would be more informative.
  • Logger cleanup looks fine; matches kilosort's own close_logger pattern.
  • Minor: stray double blank line after from packaging import version.

@alejoe91
Copy link
Copy Markdown
Member Author

Thanks @galenlynch! All fixed in the latest commit. I also added a comment to the Motion function highlighting that displacements shold be relative to spatial bins. I was tricked myself :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sorters Related to sorters module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

kilosort4.log not being closed

2 participants