brian-team / brian2genn

Brian 2 frontend to the GeNN simulator
http://brian2genn.readthedocs.io/
GNU General Public License v2.0
47 stars 16 forks source link

Updating shared variables in run_regularly actions leads to compile error #113

Closed kernfel closed 4 years ago

kernfel commented 4 years ago

I'm trying to use a run_regularly command to update an input stimulus. Part of the stimulus definition includes a shared variable, which is to be updated by the run_regularly code. This works fine in runtime Brian2, but GeNN compilation fails with reference to what seems to be an improperly declared vector.

Minimal working example:

from brian2 import *
import brian2genn
set_device('genn')

model = Equations('''
individual_var: 1
shared_var: 1 (shared)
''')

G = NeuronGroup(100, model)
G.run_regularly('''
shared_var = 1
individual_var = 1
''', 50*ms)

N = Network(G)
N.run(100*ms)

Compile output:

make -C magicnetwork_model_CODE
make[1]: Entering directory '/home/felix/projects/scratch/GeNNworkspace/magicnetwork_model_CODE'
make[1]: Nothing to be done for 'all'.
make[1]: Leaving directory '/home/felix/projects/scratch/GeNNworkspace/magicnetwork_model_CODE'
/usr/lib/nvidia-cuda-toolkit/bin/g++ -std=c++11 -Wno-write-strings -I. -Ibrianlib/randomkit -w -O3 -ffast-math -fno-finite-math-only -march=native  main.cpp  objects.cpp  code_objects/neurongroup_run_regularly_codeobject.cpp  brianlib/randomkit/randomkit.cc -o main -Lmagicnetwork_model_CODE -lrunner -Wl,-rpath magicnetwork_model_CODE 
In file included from /usr/include/c++/7/vector:60:0,
                 from main.h:11,
                 from main.cpp:8:
/usr/include/c++/7/bits/stl_algobase.h: In instantiation of ‘_OI std::__copy_move_a(_II, _II, _OI) [with bool _IsMove = false; _II = double*; _OI = double]’:
/usr/include/c++/7/bits/stl_algobase.h:422:45:   required from ‘_OI std::__copy_move_a2(_II, _II, _OI) [with bool _IsMove = false; _II = double*; _OI = double]’
/usr/include/c++/7/bits/stl_algobase.h:455:8:   required from ‘_OI std::copy(_II, _II, _OI) [with _II = double*; _OI = double]’
/usr/include/c++/7/bits/stl_algo.h:782:23:   required from ‘_OutputIterator std::__copy_n(_RandomAccessIterator, _Size, _OutputIterator, std::random_access_iterator_tag) [with _RandomAccessIterator = double*; _Size = int; _OutputIterator = double]’
/usr/include/c++/7/bits/stl_algo.h:806:27:   required from ‘_OIter std::copy_n(_IIter, _Size, _OIter) [with _IIter = double*; _Size = int; _OIter = double]’
engine.cpp:51:86:   required from here
/usr/include/c++/7/bits/stl_algobase.h:378:57: error: no type named ‘value_type’ in ‘struct std::iterator_traits<double>’
       typedef typename iterator_traits<_OI>::value_type _ValueTypeO;
                                                         ^~~~~~~~~~~
/usr/include/c++/7/bits/stl_algobase.h:383:9: error: no type named ‘value_type’ in ‘struct std::iterator_traits<double>’
       const bool __simple = (__is_trivial(_ValueTypeI)
                             ~~~~~~~~~~~~~~~~~~~~~~~~~~
                       && __is_pointer<_II>::__value
                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                       && __is_pointer<_OI>::__value
                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         && __are_same<_ValueTypeI, _ValueTypeO>::__value);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
make: *** [Makefile:10: main] Error 1

The offending line in engine.cpp is std::copy_n(brian::_array_neurongroup_shared_var, 1, shared_varneurongroup);. The error resolves if the shared variable is removed or made individual.

tnowotny commented 4 years ago

Yep, it's a bug. For shared variables, the variable translates into a "extra global parameter" (EGP) which is not an array (unlike in brian where it is an array of length 1). So, during the copy commands here: https://github.com/brian-team/brian2genn/blob/1cfe3f22055a323658e93558e2c7e004e750fb8c/brian2genn/templates/engine.cpp#L143 and here: https://github.com/brian-team/brian2genn/blob/1cfe3f22055a323658e93558e2c7e004e750fb8c/brian2genn/templates/engine.cpp#L166 an potentially also here: https://github.com/brian-team/brian2genn/blob/1cfe3f22055a323658e93558e2c7e004e750fb8c/brian2genn/templates/engine.cpp#L110 we need to have an "&" in front of {{var}}{{obj.monitored}} or {{var}}{{obj['owner'].name}} if the variable {{var}} is a shared variable. Unfortunately, I am not quite sure whether and how the information about the nature of {{var}} is available within the engine.cpp template (not even sure where the variables entry of the run_regularly_operations dictionaries is set as it doesn't appear here: https://github.com/brian-team/brian2genn/blob/1cfe3f22055a323658e93558e2c7e004e750fb8c/brian2genn/device.py#L1716-L1729 @mstimberg , I am afraid I can diagnose the problem but am missing this key step towards a solution. Can you help?

mstimberg commented 4 years ago

Unfortunately I don't have the time to look into this in detail right now. But in general: {{var}} is the name of the variable, and you should be able to get the corresponding Variable object via {{obj['owner'].variables[var]}}. For a shared variable defined in the equations, this should be an ArrayVariable object which has e.g. the size attribute. Actually, this is already used here: https://github.com/brian-team/brian2genn/blob/1cfe3f22055a323658e93558e2c7e004e750fb8c/brian2genn/templates/engine.cpp#L166 If size == 1, you can consider it shared/scalar. You can also directly use its .scalar attribute.

kernfel commented 4 years ago

size==1 seems prone to issues in (admittedly contrived) cases where population size is 1 in the first place, but .scalar seems to work. I don't see a way to replicate this with monitored values though (line 110 pointed out by Thomas above) since we don't have access to the monitored source object (only its name) from within the template. I guess shared variables are unlikely to be monitored, though?

There's an alternative solution that would deal with all cases, namely using C++ templates to wrap the copy_n calls. Can I assume that the arrays are always some kind of scalar*?

tnowotny commented 4 years ago

Hi @kernfel, I think I put a workable fix into the fix_shared_variables branch including state monitors. Can you check it works for you?

tnowotny commented 4 years ago

(the source object is available I think also in the state monitor case) I am not sure whether the state monitor works as you would expect though, but that may be a separate issue.

mstimberg commented 4 years ago

Two quick comments: size==1 would be fine from Brian's side (we basically treat shared variables in the same way as a non-shared variable for a group of size 1), but that might indeed not be the case for GeNN.

Regarding monitors: in Brian at least, you can record shared variables, but it is indeed rather rare. It will also be recorded for each neuron, but I think we raise a warning in this case, since it is obviously not very interesting to record the same value several times.

tnowotny commented 4 years ago

yes, indeed, for brian2genn testing on size == 1 would be wrong as a neuron populations of size one with a non-shared variable would have an array of length 1, while a shared variable would be a scalar.

tnowotny commented 4 years ago

Ok ... after a little glitch earlier, everything seems to be working now except that we are throwing a spurious warning about shared var not being monitored in this little example (while it actually is monitored perfectly fine...) - I will leave that for another day.

from brian2 import *
import brian2genn
set_device('genn')

model = Equations('''
individual_var: 1
shared_var: 1 (shared)
''')

G = NeuronGroup(100, model)
G.run_regularly('''
shared_var = shared_var+1
individual_var = 1
''', 5*ms)

M = StateMonitor(G, True, True)

N = Network(G,M)
N.run(100*ms)
print(M.shared_var)
print(M.individual_var)
print(M.t)
kernfel commented 4 years ago

The only warning I get with this example is WARNING Variable shared_var is a shared variable but it will be recorded once for every target. [brian2.monitors.statemonitor], which does in fact seem to happen (M.shared_var.shape is (100,1000)) and is independent of Brian2GeNN. In any case, I'm happy with the fix, thank you!

mstimberg commented 4 years ago

Closed via #114