-
Notifications
You must be signed in to change notification settings - Fork 233
Allow run_sorter to accept dicts
#4005
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@chrishalcrow maybe worth saving the |
|
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" 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 ?
Not sure I like this ugly hack in the format spikeinterface_info.json. I would be happy to have a call. |
| si_file = f | ||
| return BaseExtractor.load(si_file, base_folder=folder) | ||
|
|
||
| elif object_type.startswith("Group"): |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
…pikeinterface into sort-dict-minimal
|
bien joué |
Allows
run_sorterto accept dictionaries of recordings, and return a dict of sorters.Longer-term aim, make this workflow just work:
Think this PR is minimally invasive. The old function
run_sortercan now accept a dict. If it gets a dict, it callsrun_sorterfor each recording and saves in a folderAnd
spikeinterface_info.jsonlooks 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.