brian-team / brian2genn

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

Summed variables #87

Closed tnowotny closed 5 years ago

tnowotny commented 5 years ago

This pull request is adding the feature of summed variables to Brian2Genn. In the wake of allowing summed variables, I also had to lift the restriction that every synapse has to have a post-synaptic effect (in the narrow sense of having a "post_write_var") as the post-synaptic effect could well come through a summed variable. I suspect that the use case in teili by @schlowm0 might re-invent what GeNN post-synapses do but for the moment I think this pull request removes a roadblock for using brian2genn from teili.

tnowotny commented 5 years ago

@mstimberg - I am not sure why the tests fail on Azure. It seems it fails at the "buildmodel.sh" or final compilation stage? When I run the brian2 tests locally (on a Mac in the first instance) I get: FAILED (NOT_IMPLEMENTED=58, SKIP=16, failures=2) The two failures are: test_subgroup_summed_variable:

======================================================================
FAIL: brian2.tests.test_subgroup.test_subgroup_summed_variable
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/its/home/tn41/anaconda2/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/its/home/tn41/localdisk_projects/develop/brian2/brian2/tests/test_subgroup.py", line 489, in test_subgroup_summed_variable
    assert_array_equal(target.Iin, [10, 5, 10, 1, 1])
  File "/its/home/tn41/anaconda2/lib/python2.7/site-packages/numpy/testing/nose_tools/utils.py", line 855, in assert_array_equal
    verbose=verbose, header='Arrays are not equal')
  File "/its/home/tn41/anaconda2/lib/python2.7/site-packages/numpy/testing/nose_tools/utils.py", line 779, in assert_array_compare
    raise AssertionError(msg)
AssertionError: 
Arrays are not equal

(mismatch 60.0%)
 x: array([10., 10., 10., 10., 10.])
 y: array([10,  5, 10,  1,  1])
-------------------- >> begin captured stdout << ---------------------
running brian code generation ...
building genn executable ...
executing genn binary on CPU ...

--------------------- >> end captured stdout << ----------------------

test_pre_post_simple

======================================================================
FAIL: brian2.tests.test_synapses.test_pre_post_simple
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/its/home/tn41/anaconda2/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/its/home/tn41/localdisk_projects/develop/brian2/brian2/tests/test_synapses.py", line 894, in test_pre_post_simple
    assert_allclose(syn_mon.pre_value[0][syn_mon.t >= 1*ms], 1)
  File "/its/home/tn41/localdisk_projects/develop/brian2/brian2/tests/utils.py", line 30, in assert_allclose
    numpy_allclose(np.asarray(actual), np.asarray(desired), rtol=rtol, atol=atol, **kwds)
  File "/its/home/tn41/anaconda2/lib/python2.7/site-packages/numpy/testing/nose_tools/utils.py", line 1396, in assert_allclose
    verbose=verbose, header=header, equal_nan=equal_nan)
  File "/its/home/tn41/anaconda2/lib/python2.7/site-packages/numpy/testing/nose_tools/utils.py", line 779, in assert_array_compare
    raise AssertionError(msg)
AssertionError: 
Not equal to tolerance rtol=9.99201e-08, atol=0

(mismatch 100.0%)
 x: array([0., 0., 0., 0., 0., 0., 0., 0., 0., 0., 0., 0., 0., 0., 0., 0., 0.,
       0., 0., 0.])
 y: array(1)
-------------------- >> begin captured stdout << ---------------------
running brian code generation ...
building genn executable ...
executing genn binary on CPU ...

--------------------- >> end captured stdout << ----------------------

I am not quite sure what is going on with these. The former probably was a "not implemented" case before and now exposes that my solution to summed variables doesn't (yet) work with sub-groups? The latter seems odd as it does not seem to include summed variables? Did this come in through me lifting the constraint of "no post-synaptic effect"?

tnowotny commented 5 years ago

Ok .. it cannot work with subgroups. I looked into it and if the post-synaptic population is a subgroup, Brian 2 expects that the summed variable of post-synaptic neurons outside the subgroup is unperturbed. This is impossible to implement in the current solution because the knowledge which neurons are in the target subgroup is not stored in genn and hence all neurons in the post-synaptic neuron will be updated. Due to the single "inSyn" mechanism there is no place where to add the additional information which inSyn variables are 0 because there was 0 input at this time and those that are 0 because the inSyn belongs to a target neuron that is not part of the target subgroup. I am now throwing a NotImplementedError whenever the post-synaptic target of a summed variable is a subgroup.

tnowotny commented 5 years ago

This leaves the test test_pre_post_simple: In this test we have two problems: a) brian2genn refuses to monitor a variable that is unused. Later, this leads to test failure. The second effect seems to be a timestep and/or update ordering issue: The test is adding a spike at 2ms which leads to an increment of post_value from 0 to value 2. The test expects post_value to be 2 for all times ```t >= 2msbut brian2genn has value 2 only fort > 2*ms```, i.e. one time step later. I am not sure what to think of either problem:

  1. In principle we could let people monitor unused variables ... why not? they might just need them as a readout of some sort. I would lean towards removing this restriction (unless there is some good reason why it has to be??)
  2. I feel the brian2genn behaviour is correct and the change should only be visible at the next time step; but I realise that recording when='end' is probably meant to record once the value has been updated for the next time step. I don't know what to do about this.
tnowotny commented 5 years ago

It's not quite ready yet, actually. I am missing a step where we analyse the addVar expression for contained variables and add those variables to synapse_model.variables. I am not quite sure how to do that as from the addVar expression alone it would not be clear what type of variable may be contained - so we need a different source of this information ... the latest version of the testSummedVariables.py script exposes this problem.

tnowotny commented 5 years ago

Actually, the problem of variables missing may have been caused by the silly bug I fixed in a0744f9. Which leaves us the failing tests on Azure (currently 11 out of 17) and questions about the behaviour of test_synapses.test_pre_post_simple (as discussed above). Finally, can we discuss whether we could also accommodate subexpressions in synaptic code, which has become a problem in issue #86 ?

mstimberg commented 5 years ago

Which leaves us the failing tests on Azure (currently 11 out of 17) and questions about the behaviour of test_synapses.test_pre_post_simple (as discussed above).

I think this is the same thing, i.e. the tests are failing for test_synapses.test_pre_post_simple – I'm actually surprised they are not failing for all tests, maybe it works with single precision by accident, because we do a comparison of floating point values. I'll fix this test in Brian 2 itself, but it will not be taken into account for the brian2genn test suite until we do a new release.

Finally, can we discuss whether we could also accommodate subexpressions in synaptic code, which has become a problem in issue #86 ?

I'll try to look into this soon

tnowotny commented 5 years ago

Any luck?

tnowotny commented 5 years ago

Ok, there is no particular reason why more complex usages should be excluded. I have now added the additional processing needed (collect variables from the summed variables code line and put appropriate $(.) dressing). The gap junction example now works flawlessly. But i might have forgotten something else? But, please have a look. I think it might be good now.

tnowotny commented 5 years ago

PS: I obviously forgot to pull your changes first and had to merge them later. There was an issue with the git mergetool not working well on my Mac (even though I now think for some reason your commit changing device.py was strange as the github diff online is also behaving strangely). Anyway, I hope I did not damage any of your work on getting the testing back on track. If problems, it would be some of your edits missing in device.py.

mstimberg commented 5 years ago

(even though I now think for some reason your commit changing device.py was strange as the github diff online is also behaving strangely).

Sorry about that, apparently for some reason device.py had Windows line endings in the repository (normally it shouldn't, Windows endings should be converted into Unix ending when committing things into the repository) and it got converted to Linux endings with my commit.

On github (and I guess in gitmerge?) you can ignore whitespace changes, though: Screenshot_2019-06-13 Summed variables by tnowotny · Pull Request #87 · brian-team brian2genn

I'll look into things tomorrow, thanks for the changes!