Skip to content

Conversation

@chrishalcrow
Copy link
Member

@chrishalcrow chrishalcrow commented Jun 23, 2025

Allows run_sorter to accept dictionaries of recordings, and return a dict of sorters.

Longer-term aim, make this workflow just work:

pp_recs = si.common_reference(rec.split_by(‘group’))
sortings = si.run_sorter(pp_rec)
analyzer = si.create_sorting_analyzer(sortings, pp_recordings)

Think this PR is minimally invasive. The old function run_sorter can now accept a dict. If it gets a dict, it calls run_sorter for each recording and saves in a folder

{folder}/
    spikeinterface_info.json
    splitting_key_0/
      {whatever run_sorter spits outs}
    splitting_key_1
        {whatever run_sorter spits outs}
    …

And spikeinterface_info.json looks like e.g.:

{
    "version": "0.102.4",
    "dev_mode": true,
    "object": "Group[SorterOutput]",
    "dict_keys": [
        "a",
        5
    ],
}

Tried a bigger refactor here: https://github.com/chrishalcrow/spikeinterface/tree/sort-by-group which avoids the recurrent logic here, but it was a mess. This seems a lot nicer.

@chrishalcrow chrishalcrow changed the title Allow for sort_by_dict Allow run_sorter to accept dicts Jun 23, 2025
@chrishalcrow chrishalcrow added the enhancement New feature or request label Jun 23, 2025
@alejoe91
Copy link
Member

@chrishalcrow maybe worth saving the dict_keys dtype in the spikeinterface_info since this gets lost when dumping to json

@samuelgarcia
Copy link
Member

Thank you Chris, this is cool.

We also need to implement the spikeinterface.load that handle to open this new folder type.

We should discuss a bit more the "object": "dict of BaseSorting"
I add in mind something like "Group[Sorting]"

Also not sure to clearly understand why we need this "split_by_property" in the dict because the information is dump in all sub-folder in recording.json no ?

@chrishalcrow maybe worth saving the dict_keys dtype in the spikeinterface_info since this gets lost when dumping to json

Not sure I like this ugly hack in the format spikeinterface_info.json.
List of keys can be int or str no ? json handle it.
The list is in the dict to open back the sub folder in the good order and will converted to str for folder.
So I do not see why.

I would be happy to have a call.

si_file = f
return BaseExtractor.load(si_file, base_folder=folder)

elif object_type.startswith("Group"):
Copy link
Member Author

Choose a reason for hiding this comment

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

Feedback on these lines of code very welcome! Wasn't sure best way to proceed.

if recording is not None:
sorting.register_recording(recording)

if recording.get_annotation("split_by_property") is not None:
Copy link
Member Author

Choose a reason for hiding this comment

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

Hello, I now think this is bad idea. The user might split_by to do some preprocessing, then aggregate the recording back together, then sort. So we don't know that the split_by_property was used to do the split sorting.

Maybe better to not mess with BaseSorter, and deal with this at the Analyzer creation? Since that's where we merge Recording and Sorting information?

Copy link
Member

Choose a reason for hiding this comment

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

yep, you are rigth, lets discuss a bit more on this

Copy link
Member Author

Choose a reason for hiding this comment

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

Super. I don't think we need to set the unit properties on load. On load, you get a dict whose keys tell you the split. I think we should only set unit properties to these keys at the unit_aggregation stage, since that is the point that you would lose the dictionary structure.

@samuelgarcia
Copy link
Member

bien joué

@alejoe91 alejoe91 added this to the 0.103.0 milestone Jul 3, 2025
@alejoe91 alejoe91 merged commit abd45a7 into SpikeInterface:main Jul 4, 2025
20 of 21 checks passed
@chrishalcrow chrishalcrow deleted the sort-dict-minimal branch July 4, 2025 14:51
@alejoe91 alejoe91 added the sorters Related to sorters module label Jul 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request sorters Related to sorters module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants