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

step_current_generator must require sufficient spacing of amplitude_times #740

Closed
heplesser opened this issue May 31, 2017 · 2 comments
Closed

Comments

@heplesser
Copy link
Contributor

@heplesser heplesser commented May 31, 2017

Currently, amplitude_times in step_current_generator can be set to arbitrary doubles; NEST only checks that they are non-descending. If times are less than resolution apart, the step_current_generator will not advance correctly through the amplitude array, as it can move at most one array position per time step.

Given that the output current of the generator can only change at time steps, allowing the user to set amplitude_times values that do not coincide with time steps, is misleading.

Therefore, I propose that the generator in the future should only accept amplitude_times values that are strictly increasing multiples of the resolution and that are cast to time steps using Time::ms_stamp().

The following example runs once at a high resolution, resolving all amplitude times and once at a low resolution:

/run_sim [ /doubletype ]  % resolution, neg power of two
{
  /res Set
  ResetKernel
  0 << /tics_per_ms 1024. /resolution res >> SetStatus
  
  /scg /step_current_generator 
    << /amplitude_times  [ 1. 1.0625 1.125 1.1875 1.25 2. 2.5 ]
       /amplitude_values [ 10. -20.  40.   -80.   160. -320. 640. ] 
    >> Create def
  /nrn /iaf_psc_alpha << /V_th 1e10 /C_m 1.0 /tau_m 1.0
                         /E_L 0.0 /V_m 0.0 /V_reset 0.0 >> Create def
  
  /vm  /voltmeter Create def
  vm << /start 1.75 /stop 4.0 /interval 0.25 >> SetStatus
  
  scg nrn Connect
  vm  nrn Connect

  5 Simulate
  vm [ /events /V_m ] get
}
def

(High resolution: ) =only 2. -5 pow run_sim ==
(Low resolution : ) =only 2. -2 pow run_sim ==

yielding

High resolution: <. 0.000000e+00 -3.137381e+00 3.294848e+01 6.105218e+01 8.293936e+01 -6.190513e+00 -7.560493e+01 8.268632e+01 2.059637e+02 .>
Low resolution : <. 0.000000e+00 2.211992e+00 -2.701283e+00 -6.527746e+00 -9.507798e+00 -1.182866e+01 -1.363616e+01 1.309476e+02 2.435496e+02 .>
@heplesser
Copy link
Contributor Author

@heplesser heplesser commented Jun 1, 2017

@zbarni Could you take a look at this one? The solution is the same as required in #671.

@appukuttan-shailesh
Copy link
Contributor

@appukuttan-shailesh appukuttan-shailesh commented Jun 2, 2017

@heplesser, @zbarni : Just a note to consider the inclusion of a
spike_generator::Paramters_::assert_valid_spike_time_and_insert_() type check for the step_current_generator.

@heplesser heplesser added this to the NEST 2.12.1 milestone Jun 29, 2017
@jougs jougs closed this in 2433064 Aug 30, 2017
jougs added a commit that referenced this issue Aug 30, 2017
Fix #740 by representing times in steps in step_current_generator.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.