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.

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

[Feature] Synchrofact Detection #322

Merged
merged 65 commits into from Dec 4, 2020

Conversation

Kleinjohann
Copy link
Contributor

@Kleinjohann Kleinjohann commented May 11, 2020

This is a method to detect highly synchronous spikes (at the level of sampling rate precision with an option to extend this to jittered synchrony) and annotate or optionally remove them.

The detection of complexity intervals (time intervals with at least n spikes separated by strictly fewer than spread - 1 empty bins) is particularly suited for artefact detection and analysis, but can be used to get a (time-resolved) overview of complexities at any time-scale. This is different from statistics.time_histogram and statistics.complexity_pdf since it uses the amount of empty bins separating spikes, not a simple bincount and not a moving window with a fixed width while still providing time-resolved output.

I started from an institute-internal script and adapted it to use a BinnedSpikeTrain in the histogramming procedure and I adapted a set of basic tests I wrote for said internal script.

@pep8speaks
Copy link

pep8speaks commented May 11, 2020

Hello @Kleinjohann! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-12-03 15:57:25 UTC

@Kleinjohann Kleinjohann marked this pull request as draft May 11, 2020 09:58
@coveralls
Copy link
Collaborator

coveralls commented May 11, 2020

Coverage Status

Coverage increased (+0.2%) to 90.137% when pulling 4103045 on INM-6:synchrofact_detection into 8e465fd on NeuralEnsemble:master.

@morales-gregorio
Copy link
Collaborator

Solves #326

From my side this PR is ready for review.

@Kleinjohann Kleinjohann changed the title [WIP] [Feature] Synchrofact Detection [Feature] Synchrofact Detection May 27, 2020
@Kleinjohann Kleinjohann marked this pull request as ready for review May 27, 2020 10:29
Copy link
Member

@dizcza dizcza left a comment

Choose a reason for hiding this comment

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

There are many small remarks, but two major concerns are:

  • consider switching to OOP;
  • consider out-of-place processing instead of in-place spike removal.

We'll discuss this tomorrow anyway.


Also, I don't see any references. Is this completely your own implementation and ideas?

doc/reference/spike_train_processing.rst Outdated Show resolved Hide resolved
doc/reference/spike_train_processing.rst Outdated Show resolved Hide resolved
elephant/spike_train_processing.py Outdated Show resolved Hide resolved
elephant/utils.py Outdated Show resolved Hide resolved
elephant/spike_train_processing.py Outdated Show resolved Hide resolved
elephant/spike_train_processing.py Outdated Show resolved Hide resolved
elephant/spike_train_processing.py Outdated Show resolved Hide resolved
elephant/spike_train_processing.py Outdated Show resolved Hide resolved
elephant/spike_train_processing.py Outdated Show resolved Hide resolved
elephant/spike_train_processing.py Outdated Show resolved Hide resolved
@dizcza
Copy link
Member

dizcza commented Oct 1, 2020

I find this PR ready for the final review and merge.
Since it depends on another ongoing PR (in terms of the file location of your methods) about the Spike Contrast method, let's wait for the latter to be merged first.

@dizcza
Copy link
Member

dizcza commented Oct 5, 2020

Ready for the final review.

elephant/statistics.py Outdated Show resolved Hide resolved
elephant/statistics.py Outdated Show resolved Hide resolved
elephant/statistics.py Outdated Show resolved Hide resolved
elephant/statistics.py Outdated Show resolved Hide resolved
@Kleinjohann
Copy link
Contributor Author

I have fixed everything we discussed in our meeting on this and addressed all comments in here.

I spotted some double backticks in docstrings, are those a problem?

@dizcza
Copy link
Member

dizcza commented Dec 3, 2020

Thanks a lot!

I spotted some double backticks in docstrings, are those a problem?

No, it's not a problem. Double vs single backticks is a matter of choice.

I'll merge it as soon as the tests pass.

P.S. Consider also adding into viziphant some of the nice synchroplots that you showed to us a month ago.

@dizcza dizcza merged commit 9eadcf3 into NeuralEnsemble:master Dec 4, 2020
@dizcza dizcza deleted the synchrofact_detection branch December 4, 2020 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new functionality New modules, functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants