-
Notifications
You must be signed in to change notification settings - Fork 93
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/instantaneous rate #453
Fix/instantaneous rate #453
Conversation
elephant/test/test_statistics.py
Outdated
@@ -20,10 +20,10 @@ | |||
|
|||
import elephant.kernels as kernels | |||
from elephant import statistics | |||
from elephant.spike_train_generation import homogeneous_poisson_process | |||
from elephant.spike_train_generation import StationaryPoissonProcess as StatPP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed deprecation
|
||
|
||
class isi_TestCase(unittest.TestCase): | ||
class IsiTestCase(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CamelCase convention for classes
# n points have n-1 intervals; | ||
# instantaneous rate is a list of intervals; | ||
# hence, the last element is excluded | ||
rate = rate[:-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed since ouput size is now corrected
@@ -680,7 +681,7 @@ def test_spikes_on_edges(self): | |||
kernel=kernel, | |||
cutoff=cutoff, trim=True, | |||
center_kernel=center_kernel) | |||
assert_array_almost_equal(rate.magnitude, 0, decimal=3) | |||
assert_array_almost_equal(rate.magnitude, 0, decimal=2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lowering the precision is appropriate in this case ?, see comment in line 664
@@ -701,7 +702,8 @@ def test_trim_as_convolve_mode(self): | |||
for trim in (False, True): | |||
rate_centered = statistics.instantaneous_rate( | |||
spiketrain, sampling_period=sampling_period, | |||
kernel=kernel, cutoff=cutoff, trim=trim) | |||
kernel=kernel, cutoff=cutoff, trim=trim, | |||
center_kernel=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for better readability of the unittest, furthermore the default center_kernel=True
might be changed in the future
@@ -750,7 +752,7 @@ def test_instantaneous_rate_regression_245(self): | |||
# This test makes sure that the correct kernel width is chosen when | |||
# selecting 'auto' as kernel | |||
spiketrain = neo.SpikeTrain( | |||
range(1, 30) * pq.ms, t_start=0 * pq.ms, t_stop=30 * pq.ms) | |||
pq.ms * range(1, 30), t_start=0 * pq.ms, t_stop=30 * pq.ms) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quantities works with range()
, not the other way, but this could be changed back for better readability
… of sampling period), to get consistent ouput sizes, cutting of wings is done by fft convolve for center_kernel=True
Hello @Moritz-Alexander-Kern! 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 2022-03-30 09:22:37 UTC |
This PR addresses #374 .
Reproduce with:
The green bars in the above figure represent the
rate.times
. In Issue #374 it is suspected that one rate time is missing.Inspecting the rate.times with:
yields:
This output of the calculated instantaneous rate seems to be expected.
Explanation: The rate estimation is working on an interval e.g. the first interval here is [0, 0.445), the corresponding rate estimate has a
rate.times
value of0
(see figure above, orange line at time 0s). The rate estimate for interval [9.345, 9.790) is given at 9.345 (orange line).The last interval [9.790, 10.235), which would be drawn at 9.790s, is excluded since it is not a full interval for the given spiketrain with
t_stop =10 s
andsampling_period=0.445
. The rate estimate is hence considered to be "not valid" and is not returned bystatistics.instantaneous_rate
. (this is expected)Unreported Bug: calculation of range for
np.histogram
inelephant/elephant/statistics.py
Line 827 in b263ffe
To reproduce: (note that the chosen
spike_times
are just multiples of the sampling period shifted by 0.01s, compare torate.times
values from previous example)The rate estimation (orange line) in comparison to the spikes (black bars) appears to be shifted by one
sampling_period
(green bars). Note that thespike_times
were deliberately chosen so that there should be no sampling bias here. This bug is somewhat concealed since therate_times
are generated with:elephant/elephant/statistics.py
Lines 896 to 899 in b263ffe
In other words, the green bars do not represent the intervals used for the calculation of the rate estimation (implemented with
np.histogram
).This PR fixes the calculation of the range for
np.histogram
. Using the code example from above now yields:Additionally this fixes lines 874 - 890, which are no longer needed, if the following behavior is expected.
to reproduce:
In current implementation the last interval [9.0, 9.8) (
t_start=0, t_stop=9.8, sampling_period=1s
) is used to estimate the rate in other intervals, see output:with this PR, the last "not valid" interval [9.0, 9.8) is no longer considered: