Fix handling of rate change in poisson_generator_ps #1280
Conversation
hakonsbm
changed the title
|
|
||
| first_spikes 5 gt first_spikes 15 lt and | ||
| second_spikes 0 eq | ||
| third_spikes 50 gt third_spikes 150 lt and |
There was a problem hiding this comment.
A fine test, but I think it would be even better to test the distribution of time to first spike explicitly. For that you could use, e.g., 1000 parrots, simulate only long enough to get at least one spike with high probability and the perform a KS-test on the spike times against the expected exponential distribution. Might be easier to do that in Python, since you have the tests available there. Would be a good start on more statistical testing outside of connection testing.
precise/poisson_generator_ps.h
Outdated
| double dead_time_; //!< dead time [ms] | ||
| double rate_; //!< process rate [Hz] | ||
| double dead_time_; //!< dead time [ms] | ||
| bool rate_changed_; //!< flag for rate change |
There was a problem hiding this comment.
I am not entirely happy with rate_changed_ here in P_, since it is not a model parameter but an implementation detail. B_ is probably the best place to put it, since V_ is for variables recomputed by calibrate(). Would it be difficult to move the flag to B_?
heplesser
added
ZC: Model
|
@heplesser There is now a Python test replacing the SLI regressiontest, which performs the same change of rate, but uses a KS-test on the spike times. |
|
@hakonsbm I may have overlooked something but can't you just set |
| """Statistical test of poisson_generator_ps rate change""" | ||
|
|
||
| # Lower limit of acceptable p_value | ||
| p_value_lim = 0.3 |
There was a problem hiding this comment.
p = 0.3 seems an unusual limit.
| min_times = [np.min(times[np.where(senders == s)]) | ||
| for s in np.unique(senders)] | ||
| d, p_val = scipy.stats.kstest( | ||
| min_times, 'expon', args=(start_t + 0.1, 10.)) |
There was a problem hiding this comment.
I am not really happy with the "magic numbers" 0.1 and 10. here. It would be more robust and transparent to pass the expected rate as argument. For even more robustness, I'd suggest to set the resolution explicitly and have it in a variable; maybe use 0.25 or so to avoid the numerically unpleasant 0.1 ;).
|
@heplesser I have updated the test, please have another look. |
| nest.Simulate(100) | ||
| nest.SetStatus(sd, {'n_events': 0, | ||
| 'start': float(sim_time) + resolution, | ||
| 'stop': 2.*sim_time}) |
| nest.SetStatus(pg, {'rate': rate}) | ||
| nest.SetStatus(sd, {'n_events': 0, | ||
| 'start': 2. * sim_time, | ||
| 'stop': 3.*sim_time}) |
There was a problem hiding this comment.
Spaces around * as on line above.
|
|
||
| def kstest_first_spiketimes(sd, start_t): | ||
| def kstest_first_spiketimes(sd, start_t, rate): |
There was a problem hiding this comment.
Code in ks_first_spiketimes() depends in resolution and n_parrots defined in the surrounding scope. That is neither transparent nor safe. I would suggest to pass them as arguments along with rate. I'd also suggest to make the function a private method in TestPgRateChange instead of defining it as an internal function here.
|
@heplesser The test is updated with your proposed changes now. |
Adds a
P::rate_changed_flag topoisson_generator_pswhich lets theevent_hook()handle the interval from the rate change to the first subsequent spike. This makes sure that the generator model correctly handles rate changes. A regeressiontest is also added.Fixes #1278
Fixes #1279