Skip to content

Move the Base.save to Sorting.save() and Recording.save() - #4671

Open
samuelgarcia wants to merge 17 commits into
SpikeInterface:mainfrom
samuelgarcia:refactor_base_save_dump
Open

Move the Base.save to Sorting.save() and Recording.save()#4671
samuelgarcia wants to merge 17 commits into
SpikeInterface:mainfrom
samuelgarcia:refactor_base_save_dump

Conversation

@samuelgarcia

@samuelgarcia samuelgarcia commented Jul 9, 2026

Copy link
Copy Markdown
Member

Move saving logic to extractor classes.

For historical reason, lazyness and original bad design, the BaseExtractor.save() was a terrible mess for:

  • save/load properties
  • annotation
  • dump provevance
  • create the "si_folder.json"
  • ...
    It was using some kind of "hook" to sub classes and the logic was almost impossible to follow.

I tried to move the logic save()/load() to classe (BinaryFolderRecording, ZarrRecordingExtractor, SharedMemoryRecording, ZarrSortingExtractor, NpzFolderSorting, NumpyFolderSorting).
The code looks quite strange at soe places but at least it is easier follow.

@alejoe91 : the time vector handling is not done correctly.
But your PR #4689 should be rebase on this one I think. Lets do it togoether.

move saving logic to extractor classes
@samuelgarcia samuelgarcia added the Edinburgh hackathon 2026 PRs from Edinburgh hackathon 2026 label Jul 9, 2026
@alejoe91 alejoe91 changed the title mode the Base.save to Sorting.save() and Recording.save() Move the Base.save to Sorting.save() and Recording.save() Jul 15, 2026
@alejoe91 alejoe91 added this to the 0.106.0 milestone Jul 17, 2026
@samuelgarcia
samuelgarcia marked this pull request as ready for review September 8, 2026 11:00
@alejoe91

alejoe91 commented Sep 8, 2026

Copy link
Copy Markdown
Member

@samuelgarcia fixed some bugs in preprocessing, but there are more. Do you want me to fix it?

@alejoe91

alejoe91 commented Sep 9, 2026

Copy link
Copy Markdown
Member

@samuelgarcia ported changes from #4689 (timestamps handling + removing metadata_folder) and cleaned up code and docs.

One main question: in #4689 , I added an extra _in_reduce mechanism to the to_dict/_extra_metadata_to_dict stack. This was only set to True by the __reduce__, and propagated times kwargs only in this case.

Here I propose a modified version: if the user "messes" with timestamps, this is propagated to dict no matter what. Note that:

  • this is needed when saving in parallel (timestamps need to be saved!)
  • the final si_folder.json will NOT have this information, since it sets include_extra_metadate=False (added also include_annotations/include_properties to False)

Let me know what you think!

@alejoe91 alejoe91 added core Changes to core module refactor Refactor of code, with no change to functionality labels Sep 9, 2026
@alejoe91 alejoe91 modified the milestones: 0.106.0, 0.105.0 Sep 9, 2026
gain_to_uV: float | np.ndarray | None = None,
offset_to_uV: float | np.ndarray | None = None,
is_filtered: bool | None = None,
file_timestamps_paths: str | Path | list[str | Path] | None = None,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think we should limit the complexity of BinaryRecordingExtractr to load only one or sevreal binary files.
And keep the complexity only in BinaryFolderRecording other we need to maintained a 2 classes design.

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.

2 participants