spikeinterface icon indicating copy to clipboard operation
spikeinterface copied to clipboard

Tools for Generation of Hybrid recordings

Open yger opened this issue 1 year ago • 6 comments

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

yger avatar Jan 24 '24 14:01 yger

This PR depends on #2410

yger avatar Jan 26 '24 08:01 yger

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?

yger avatar Mar 28 '24 09:03 yger

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

alejoe91 avatar Mar 28 '24 10:03 alejoe91

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

yger avatar Mar 29 '24 15:03 yger

Added additional functionalities for hybrid generations:

  • filter a Templates object based on amplitudes and depths
  • scale Templates between min/max user selected amps
  • shift Templates to move them away from their original position

Also extended the plot_unit_templates to work with a sorting_analyzer_or_templates object

alejoe91 avatar May 10 '24 16:05 alejoe91

@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!

alejoe91 avatar May 14 '24 11:05 alejoe91

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!

cwindolf avatar Jun 03 '24 15:06 cwindolf

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?

alejoe91 avatar Jun 03 '24 15:06 alejoe91

@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?

alejoe91 avatar Jun 04 '24 13:06 alejoe91

I want to read it first please.

samuelgarcia avatar Jun 04 '24 14:06 samuelgarcia

Let's go through this tomorrow

alejoe91 avatar Jun 04 '24 14:06 alejoe91

@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!

alejoe91 avatar Jun 19 '24 13:06 alejoe91

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) 😄 ?

JoeZiminski avatar Jun 27 '24 12:06 JoeZiminski

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

alejoe91 avatar Jun 27 '24 15:06 alejoe91

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 :)

zm711 avatar Jun 28 '24 13:06 zm711

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

alejoe91 avatar Jun 28 '24 14:06 alejoe91

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.

zm711 avatar Jun 28 '24 14:06 zm711

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.

@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 :)

alejoe91 avatar Jun 28 '24 14:06 alejoe91

@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.

zm711 avatar Jun 28 '24 14:06 zm711

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.

I like that!

alejoe91 avatar Jun 28 '24 14:06 alejoe91

Thank you all for the input! I think this will super important for future benchmarks.

@JoeZiminski see updated PR description :)

Merging!

alejoe91 avatar Jun 29 '24 09:06 alejoe91

Cheers, that's awesome! Look forward to trying it

JoeZiminski avatar Jun 30 '24 20:06 JoeZiminski