spikeinterface
spikeinterface copied to clipboard
Tools for Generation of Hybrid recordings
For the hybrid project, make a function that can get Templates from any recording. To be discussed, as some features to play/interact with Templates are currently only written for WaveformExtactor and might need to be duplicated
This PR depends on #2410
With this PR, we can get templates quickly from a recording, and also we have a high-level function to create quickly an hybrid recording, given templates and motion. Check if this is fine for you @samuelgarcia @alejoe91 . Note that this will need to be adapted with real templates from @h-mayorquin, because I will need to estimate their putative positions in order to move them around. Is there any plan to make the localization methods also work with templates directly?
Thanks @yger
Looking great! For the localization code, it's currently living in the unit_localizations.py, which I don't think is appropriate.
I would suggest to make a localization_tools.py in the postprocessing folder and migrate all the localization functions
In doing so, we could also add support for a Templates object
I've moved all utility functions into localization_tools.py in postprocessing, as suggested by @alejoe91 . Will start to expand the compute positions functions to also work with a Template objects, as this will be requested for hybrid
Added additional functionalities for hybrid generations:
- filter a
Templatesobject based on amplitudes and depths - scale
Templatesbetween min/max user selected amps - shift
Templatesto move them away from their original position
Also extended the plot_unit_templates to work with a sorting_analyzer_or_templates object
@samuelgarcia @yger since I was working on it, I also added a Widen/Narrow option and scalebars for plot_unit_waveforms/templates. Here's how it looks:
Screencast from 14-05-2024 12:39:14.webm
This is ready to merge on my side!
Hey! Me and @chris-langfield are interested in using this code in our benchmarks. I was wondering how ready / stable it is? Also will the Motion object change the API here?
Lastly, if I bring my own (n_drift_bins, n_samples, n_channels) array of templates, can I manually plug that in here? (I couldn't tell if this code assumes that it computes the drifting templates via the DriftingTemplates.precompute_displacements()). Would you be open to adding a DriftingTemplates.from_precomputed(templates_array, drift_bin_um=5, **other_kwargs) constructor to support that kind of workflow? I'd be super happy to contribute code along those lines.
Also, if there are other to-do items that I could help out with here, let me know!
Hi @cwindolf
That is great! :) Sure we'd be happy to add the from_precomputed option. This would also make it compatible with MEArec drifting generation.
We are currently splitting this PR into several smaller PRs, so that this one will only have the main generator functions.
Can you make a separate PR for the from_precomputed?
@yger @samuelgarcia @h-mayorquin
I would suggest that we merge this one. In a follow-up PR I'll add a documentation/How-to page to show how to construct drift-aware hybrid recordings (probably starting from Nick's data).
What do you guys think?
I want to read it first please.
Let's go through this tomorrow
@samuelgarcia @yger @h-mayorquin @cwindolf
Everything is merged and all suggestions have been incorporated.
Also extended tests and moved more "tools" functions to the new localization_tools.py.
This is ready to merged on my side!
I am very fascinated by this PR and the somewhat mysterious 'Hybrid' recordings, sounds cool! Could you extend the PR message with an explanation to the naive (me) 😄 ?
Ready for final review here. See How To docs here: https://spikeinterface--2436.org.readthedocs.build/en/2436/how_to/benchmark_with_hybrid_recordings.html
I was tagged, but I can't find where. Happy to comment on something if my opinion was desired. Just got an alert this morning, but now looking I can't find it :)
I was tagged, but I can't find where. Happy to comment on something if my opinion was desired. Just got an alert this morning, but now looking I can't find it :)
It was about naming. Now one of the parameter is templates_in_uV, but I think it should be are_templates_in_uV? Not sure though since it's not a function
Cool, I think it is confusing right now because we have two arguments templates and templates_in_uV which seems to imply that we need to give both (templates and the same templates scaled). The docstring tells us one is a bool, but I do think something like scale_templates or return_scaled (if we need to be consistent) would be clear. are_templates_in_uV could also work with the assumption that we are forcing the user to answer the question, but I think the scale term would be more consistent to the library.
Cool, I think it is confusing right now because we have two arguments
templatesandtemplates_in_uVwhich seems to imply that we need to give both (templates and the same templates scaled). The docstring tells us one is a bool, but I do think something likescale_templatesorreturn_scaled(if we need to be consistent) would be clear.are_templates_in_uVcould also work with the assumption that we are forcing the user to answer the question, but I think thescaleterm would be more consistent to the library.
@zm711 we need to know if the provided templates are already scaled or not, so we don't want a verb IMO, so I'm leaning towards are_templates_in_uV, because it's indeed a question we need the user to provide info for :)
@zm711 we need to know if the provided templates are already scaled or not, so we don't want a verb IMO, so I'm leaning towards are_templates_in_uV, because it's indeed a question we need the user to provide info for :)
Then yes I think that are_templates_in_uV is better. I guess my point would be something like are_templates_scaled because our library has an equivalence between uV and scaled.
Then yes I think that
are_templates_in_uVis better. I guess my point would be something likeare_templates_scaledbecause our library has an equivalence betweenuVandscaled.
I like that!
Thank you all for the input! I think this will super important for future benchmarks.
@JoeZiminski see updated PR description :)
Merging!
Cheers, that's awesome! Look forward to trying it