Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add synchrony metrics to quality metrics #1205

Closed
wants to merge 10 commits into from
Closed

Add synchrony metrics to quality metrics #1205

wants to merge 10 commits into from

Conversation

Moritz-Alexander-Kern
Copy link

This PR is adding a metric for the presence of synchronous spiking events across multiple spike trains.

This is used to characterize synchronous events within the same spike train and across different spike
trains. This way synchronous events can be found both in multi-unit and single-unit spike trains.

The code was adapted from Elephant - Electrophysiology Analysis Toolkit, see docs for more information
Spike train synchrony

@alejoe91
Copy link
Member

Thanks @Moritz-Alexander-Kern! I'll provide some feedback soon

alejoe91
alejoe91 requested changes Jan 18, 2023
View reviewed changes
Copy link
Member

@alejoe91 alejoe91 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

First round of feedback! Looks good, but I'd rather have the low-level implementation of the annotate_synchrofacts here rather than depending on elephant just for that function.

Could you also add tests in the quality_metrics/tests/test_metric_functions.py?

pyproject.toml Outdated
@@ -26,6 +26,7 @@ dependencies = [
"threadpoolctl",
"tqdm",
"probeinterface>=0.2.14",
"elephant>=0.11.0"
Copy link
Member

Choose a reason for hiding this comment

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

this should be moved to the full extra, since elephant is used by submodules.

In addition, the function really only uses this function from elephant. I think it would be better to directly port this function (with proper references), so we don't have to rely on an additional dependency

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback, I see your point.

Please allow me some time to look into this.

Copy link
Member

Choose a reason for hiding this comment

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

Of course :) no rush!

spikeinterface/qualitymetrics/misc_metrics.py Outdated Show resolved Hide resolved
spikeinterface/qualitymetrics/misc_metrics.py Outdated Show resolved Hide resolved
@alejoe91 alejoe91 added the enhancement New feature or request label Jan 20, 2023
@samuelgarcia samuelgarcia added the qualitymetrics Related to qualitymetrics module label Jan 27, 2023
@samuelgarcia samuelgarcia changed the title [feat-qm] Add synchrony metrics to quality metrics Add synchrony metrics to quality metrics Jan 27, 2023
Moritz-Alexander-Kern added 3 commits February 13, 2023 11:48
Merge branch 'master' into add_synchrony_metric
ac33ce4 
# Conflicts:
#	doc/modules/qualitymetrics/synchrony_metrics.rst
#	pyproject.toml
#	spikeinterface/qualitymetrics/misc_metrics.py
#	spikeinterface/qualitymetrics/quality_metric_list.py
#	spikeinterface/qualitymetrics/tests/test_metrics_functions.py
fix tests for synchrony metric
3961be6
remove no longer needed functionality
35618de
@alejoe91
Copy link
Member

@Moritz-Alexander-Kern @musall any update on this?

@Moritz-Alexander-Kern
Copy link
Author

Update: Currently porting the code from elephant.

Following your suggestion to port the code from elephant to spikeinterface. I'm in the process of implementing this change and will update you once it is complete.

@samuelgarcia samuelgarcia changed the base branch from master to main April 5, 2023 15:59
@alejoe91
Copy link
Member

alejoe91 commented May 3, 2023

Hi @Moritz-Alexander-Kern

We recently changed the main branch to main instead of master.

We should not port the full elephant classes here, but just the minimal function required to find synchronous events (annotate_synchrofacts), which should take numpy array as inputs.

Would you have time to work on this?

@alejoe91 alejoe91 mentioned this pull request Sep 1, 2023
Merged
2 tasks
@alejoe91
Copy link
Member

alejoe91 commented Sep 1, 2023

Closing in favor of #1951

@alejoe91 alejoe91 closed this Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alejoe91 alejoe91 alejoe91 requested changes

Assignees
No one assigned
Labels
enhancement New feature or request qualitymetrics Related to qualitymetrics module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Moritz-Alexander-Kern @alejoe91 @samuelgarcia