[MRG] run_regularly operations on the CPU #97
Conversation
Both are in the "start" slot, so use their order value + name
brian2genn/templates/engine.cpp
Outdated
| {% for var in run_reg['write'] %} | ||
| {% if not run_reg['owner'].variables[var].owner.name in states_pushed %} | ||
| push{{run_reg['owner'].variables[var].owner.name}}StateToDevice(); | ||
| {% if states_pushed.append(run_reg['owner'].variables[var].owner.name) %}{% endif %} |
There was a problem hiding this comment.
Doesn't this mean that you are pushing target variables of run_regularly operations every timestep, even if they have not changed because i % {{obj['step']}} == 0 was false?
There was a problem hiding this comment.
Hah, indeed! I guess I was so focussed on not pushing the same state twice per time step that I forgot the obvious :)
brian2genn/templates/engine.cpp
Outdated
| {% for run_reg in run_regularly_operations %} | ||
| {% for var in run_reg['read'] %} | ||
| {% if not var in ['t', 'dt'] %} | ||
| {% if not run_reg['owner'].variables[var].owner.name in states_pulled %} | ||
| pull{{run_reg['owner'].variables[var].owner.name}}StateFromDevice(); | ||
| {% if states_pulled.append(run_reg['owner'].variables[var].owner.name) %}{% endif %} | ||
| {% endif %} | ||
| {% endif %} | ||
| {% endfor %} | ||
| {% endfor %} |
There was a problem hiding this comment.
doesn't this also mean that you are pulling variables every timestep, regardless whether the run_regularly operations will take place in the next iteration? I wonder whether one should not test here whether the variable will be needed?
There was a problem hiding this comment.
Right, actually it would make much more sense to put the pullStateFromDevice together with the actual execution of the operation. I think I got confused by all the state pulling that is going on here for the monitors, which is preparing things for the next time step as well.
There was a problem hiding this comment.
Looking at the code I realized that while it is not intuitive to have the state pulling at the end of the time step, it allows us to not do it twice if the state has already been pulled for a state monitor. I'll leave it this way then and just add a check that the next time step will execute it.
|
I think my last commit fixes the issues you identified, would be grateful if you could have another look. |
| {% if nm.run_regularly_object != None %} | ||
| if (i % {{nm.run_regularly_step}} == 0) | ||
| {% else %} | ||
| if (i % {{obj['step']}} == 0) |
There was a problem hiding this comment.
Just to be sure - is the intended behaviour that run_regulary timing is based on the local iteration variable i? Rather than on the global integer time step iT? I can't remember how we determine riT and whether the loop would ever be executed multiple times?
There was a problem hiding this comment.
We currently do not allow for multiple runs, so i and iT should always be identical. I think I prefer to use i here, since iT is a bit less straightforward (it gets updated as part of stepTimeGPU/stepTimeCPU, etc.).
We do have a couple of tests, e.g. a basic test for G = NeuronGroup(1, 'v : 1')
G.run_regularly('v += 1', dt=2*defaultclock.dt)
M = StateMonitor(G, 'v', record=0, when='end')
run(10 * defaultclock.dt)
assert_allclose(G.v[:], 5)
assert_allclose(np.diff(M.v[0]), np.tile([0, 1], 5)[:-1])and a similar one for G = NeuronGroup(10, 'v : 1', threshold='False')
tau = 10*ms
S = Synapses(G, G, 'w : 1', on_pre='v += w')
S.connect(j='i')
S.run_regularly('w = i')
run(defaultclock.dt)
assert_allclose(S.w[:], np.arange(10))I also specifically added a test that changes the |
This PR still needs a bit more thorough testing, but it seems to work in principle.
run_regularlyoperations are now always executed on the CPU, which comes with a number of advantages over the previous solution:TimedArrayTimed arrays #13).Synapsescan haverun_regularlyoperations as well (Fixes Support run-regularly for synapses #58)run_regularyoperation per object allowed (not sure how relevant this is in practice, though).If the user has a
run_regularlyoperation that gets executed every time step, then the new approach will probably slow the simulation down a bit. For the more standard use case of a rarely executed operation, I don't think we'll see any difference.Fixes #96.