Skip to content

Clean up code around create_sorting_analyzer and create/load_* functions in sortinganalyzer.py#4680

Open
ecobost wants to merge 11 commits into
SpikeInterface:mainfrom
ecobost:cleanup_sa_loading
Open

Clean up code around create_sorting_analyzer and create/load_* functions in sortinganalyzer.py#4680
ecobost wants to merge 11 commits into
SpikeInterface:mainfrom
ecobost:cleanup_sa_loading

Conversation

@ecobost

@ecobost ecobost commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

This PR does not functionally change much. It is mainly for code refactoring and to improve legibility.

The only two functional changes are:

  • fix small bug when calling save_to_zarr("test.zarr") would add an extra .zarr to the filename (
    zarr_path = cache_folder / f"{name}.zarr"
    )
  • when return_scaled and return_in_uV are provided, prioritize return_in_uV. Currently, it prefers return_scaled over return_in_uV

Code quality improvements:

  • type hinting
  • Collect code into logical blocks (e..g, everything that has to do wit main indices together); add comment headers (e.g., Saving main indices) and reorder this blocks (e.g., expected stuff like sorting should be saved and loaded before any of the optional things like sparsity)
  • Make create_binary_folder parallel create_zarr (you can now see how they are both doing exactly the same).
  • Make load_binary_folder parallel load_zarr.
  • Preserve the same order of operations when loading and saving. If sorting was saved first, then load first and so on.
  • Tighten code when possible

@ecobost
ecobost force-pushed the cleanup_sa_loading branch from 3349bd9 to 754c84f Compare July 10, 2026 14:06

@zm711 zm711 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few initial comments.

Comment thread src/spikeinterface/core/base.py
Comment thread src/spikeinterface/core/sortinganalyzer.py
if not is_remote:
folder = clean_zarr_folder_name(folder)
if folder.is_dir():
if folder.exists():

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why this change? is_dir() verifies existence and that it is a folder? So it's more specific.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In here particularly, we don't actually care about it being a directory because we are gonna delete it anyway (whether a fle or directory); but actually we use exists everywhere so I'll change the ones where isdir/isfile will be more appropiate

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well I think we try to use is_dir/is_file when possible, which I believe is better because more explicit, so I would revert

Suggested change
if folder.exists():
if folder.is_dir():

@ecobost ecobost Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i did revert/change exists()-> is_dir/is_filewhen needed on the next commit below. But exists() makes more sense here, if someone says save it at /xxx/yyy and overwrite=True, then whatever is in /xxx/yyy (dir or file) will be overwritten

Comment thread src/spikeinterface/core/sortinganalyzer.py Outdated
Comment thread src/spikeinterface/core/sortinganalyzer.py
@alejoe91 alejoe91 added core Changes to core module refactor Refactor of code, with no change to functionality Edinburgh hackathon 2026 PRs from Edinburgh hackathon 2026 labels Jul 14, 2026
@alejoe91 alejoe91 added this to the 0.105.0 milestone Jul 15, 2026
Comment thread src/spikeinterface/core/sortinganalyzer.py Outdated

@alejoe91 alejoe91 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @ecobost

Much cleaner :) I have a few suggestions/comments. Could you tackle them? Then this is good to go for me!

Comment thread src/spikeinterface/core/sortinganalyzer.py Outdated
Comment on lines +697 to +699
recording.dump(recording_provenance_file, relative_to=folder)
elif recording.check_serializability("pickle"):
recording.dump(folder / "recording.pickle", relative_to=folder)
recording.dump(recording_provenance_file.with_suffix(".pickle"), relative_to=folder)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
recording.dump(recording_provenance_file, relative_to=folder)
elif recording.check_serializability("pickle"):
recording.dump(folder / "recording.pickle", relative_to=folder)
recording.dump(recording_provenance_file.with_suffix(".pickle"), relative_to=folder)
recording.dump(folder / "recording.json", relative_to=folder)
elif recording.check_serializability("pickle"):
recording.dump(folder / "recording.pickle", relative_to=folder)

I think the previous version was cleaner. I don't see the point in defining the json path and change the suffix for pickle

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the idea was to signal the .json as the default output file; I could change it to
recording_provenance_file = folder / "recording" and then add the suffix at the dump line if you prefer

Comment thread src/spikeinterface/core/sortinganalyzer.py
Comment thread src/spikeinterface/core/sortinganalyzer.py Outdated
Comment thread src/spikeinterface/core/sortinganalyzer.py Outdated
if not is_remote:
folder = clean_zarr_folder_name(folder)
if folder.is_dir():
if folder.exists():

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well I think we try to use is_dir/is_file when possible, which I believe is better because more explicit, so I would revert

Suggested change
if folder.exists():
if folder.is_dir():

@ecobost

ecobost commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

@alejoe91 Couple of comments still open. Let me know when this version is ok to fix merge issues (don't wanna pollute current version)

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

Labels

core Changes to core module Edinburgh hackathon 2026 PRs from Edinburgh hackathon 2026 refactor Refactor of code, with no change to functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants