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

Fix kernels and statistics (instantaneous_rate & mean_firing_rate) #313

Merged
merged 23 commits into from
May 11, 2020

Conversation

dizcza
Copy link
Member

@dizcza dizcza commented Apr 21, 2020

Breaking changes:

  • statistics:
    • when the input is a neo.SpikeTrain or a Quantity and t_start and/or t_stop are given, they (start, stop) should be of type Quantity (previously a float is accepted - error-prone);

Improvements:

  • kernels:
    • fixed [Bug] Instantaneous rate estimation fails for inconsistent kernel selection #288
    • get rid of the magic in boundary_enclosing_area_fraction function of the Kernel base class
    • fixed a slightly wrong estimate of the true boundary_enclosing_area_fraction function of AlphaKernel class --> more than x100 speedup for the AlphaKernel --> the runtime of the tests decreased from 7 sec to 70 ms;
    • fixed the wrong median_index() kernel function and added tests
    • added built-in matplotlib plots for each kernel in the docs - check-out AplhaKernel readthedocs
  • statistics:
    • vectorized binning in instantaneous_rate()
    • refactored mean_firing_rate(), see Breaking changes

Not done/Unsure:

  • previously, test_rate_estimation_consistency() had a test with trim=True. Testing with trim=True does not make sense, does it? - Becuse the x axis is shorter (trimmed). Previously, it worked because of the incorrect estimation of kernel.median_index(). If I'm right, this is a perfect example of BUG x BUG = NO BUG paradigm in programming.
  • EpanechnikovLikeKernel pdf differs from scipy implementation - we provide the valid kernel, the user decides to use it or not;
  • mean_firing_rate can easily support a list of spiketrains. Moreover, we can leverage the advantage of our parallel module here transparently for the user. I'm tempted to quickly do it in this PR, but it should be a separate topic.
  • _check_consistency_of_spiketrains() is very strict and the requirements can be relaxed
  • why tau=sqrt(3)*sigma and not just sigma in RectangularKernel? - to enclose approx. the same PDF mass as in the rest of the kernels;
  • instantaneous_rate() todo (addressed in the last commits):
    if not trim:
        # TODO: -kernel_array_size + mi + 1 ? to be consistent with 'valid'
        #  mode
        rate = rate[median_id: -kernel_array_size + median_id]

Known issues:


Update

@pep8speaks
Copy link

pep8speaks commented Apr 21, 2020

Hello @dizcza! 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-05-11 18:01:54 UTC

@coveralls
Copy link
Collaborator

coveralls commented Apr 21, 2020

Coverage Status

Coverage increased (+0.3%) to 89.208% when pulling 9bb8798 on INM-6:fix/kernels into 88682f0 on NeuralEnsemble:master.

@dizcza dizcza merged commit 12b1d93 into NeuralEnsemble:master May 11, 2020
@dizcza dizcza deleted the fix/kernels branch May 11, 2020 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants