Specifiers rewrite

[mstimberg]

Ok, I think this is the big chunk of what I want to do for now, it's missing quite a bit of documentation but apart from that it's mostly ready (some subgroup details are still open, see below). I made a couple of decisions that are worth discussing before merging (since it's a massive merge, we do want to get this right...).

I decided to have only exactly one Variable (previously: Specifier) for each state variable -- this means that Variable does neither store the name of the variable (since the name might be e.g. v_pre in Synapses instead of v) nor the index (since the index for v can be _element in NeuronGroup but e.g. _postsynaptic in Synapses). The advantage: In Synapses, Subgroup and other future places where we refer to the variables from another Group, we can simply refer to the Variable objects instead of copying them by hand, replacing some parts like the name. This means we don't have to change anything if a new attribute appears, etc. How are names and indices handled: names are simply the keys in the dictionary, a mapping to indices is done via a variable_indices dictionary in Group -- by default it is a defaultdict mapping everything to _element (this means that NeuronGroup does not have to care about this at all). Also it means no longer writing redundant things like specifiers['var'] = Specifier('var', ...).

Somewhat related to this: The iterate_all information is now stored in the templates (only used in Python anyway), so NeuronGroup etc. do not have to care about this.

I also implemented subgroups, although the implementation is not quite correct yet. I think the best thing for subgroups is that it is a user-interface feature and the internal code does not have to care about this. So for example, if we write Synapses(G[5:], G[5:]); S.connect(0, 0) this will be internally stored as a connection from neuron 5 to neuron 5, so the propagation code does not have to care about this. This would also avoid having to copy the spikes at every timestep, the only place where we need spike numbers that are specific to the subgroup is in SpikeMonitor -- but I think it's best to make SpikeMonitor handle this explicitly. This also means that subgroups do not have to be added to a Network. There's one weakness in the current implementation, though: If one uses subgroups in the Synapses constructor, we store source/target numbers twice to both support correct propagation and state variable assignments involving i and j (relative to the subgroup).

To summarize some of the renames: brian2.core.specifiers --> brian2.core.variables, Specifier --> Variable, USE_SPECIFIERS --> USES_VARIABLES, _neuron_idx --> _element_idx (since it's used for synapses etc. as well).

[coveralls]

Coverage remained the same when pulling 2e94e4d on specifiers_rewrite into 15f380c on master.

[mstimberg]

With the last commit, the propagation works correctly with subgroups. Recording spikes from a subgroup with a spike monitor does not, though. I guess we can handle the details of the subgroup implementation in a different issue/pull request anyway. One thing I realized: Writing Synapses(G[:5], ...) does not actually work since we only store a weak reference to the subgroup in Synapses and therefore the Subgroup object is not kept alive :-/

[coveralls]

Coverage remained the same when pulling 704eabf on specifiers_rewrite into 15f380c on master.

[thesamovar]

Awesome work. I think I'm happy with it, just one comment below really that would take a little bit of work if you think it's a good idea.

Just having one Variable and keeping the name in the dict: excellent idea!

iterate_all is gone: great!

Subgroups: shouldn't it all be reasonably simple if we use the Brian 1 way of doing it? Namely, for each Synapses object we would store a source_offset and target_offset which would be added to each i and j. As long as everything always uses that, subgroups should always work. It would also fix the bug with monitoring a subgroup.

_element_idx: what about just _idx?

[mstimberg]

_element_idx: what about just _idx?

This is currently not quite possible since _element_idx is only used in C++ code where we index single elements whereas numpy code uses _element referring to the whole index array. But on the other hand, I don't quite remember why I did it this way... Maybe I should change it back to _element_idx everywhere (even though I like _num_element better than _num_element_idx), and then _idx would indeed be a good idea.

Subgroups: shouldn't it all be reasonably simple if we use the Brian 1 way of doing it? Namely, for each Synapses object we would store a source_offset and target_offset which would be added to each i and j. As long as everything always uses that, subgroups should always work. It would also fix the bug with monitoring a subgroup.

You might be right that the other direction is easier -- I thought that some stuff would automatically work using my current technique (e.g. setting a synaptic weight depended on a neuronal parameter, e.g. S.w = '1. / freq_pre but actually this doesn't work since the template for variable setting (we use the reset template here) is not Synapses-specific and therefore does not set the pre/post-synaptic indices. But if we use a specific template for setting of synaptic state variables (and it wouldn't cost us much to do so), then we can also apply the offset to the indices, so that's not an issue. We wouldn't actually have to add anything to i and j. Hmm, maybe I should just try it out (now that I have tests in place :) ). If possible I would like to avoid the copying of spikes -- shifting them in the propagation template should do the trick I guess?

Ok, I'll give it a go, will report back soon :)

[coveralls]

Coverage remained the same when pulling 5fe62aa on specifiers_rewrite into 15f380c on master.

[coveralls]

Coverage remained the same when pulling 2c8b8e9 on specifiers_rewrite into 15f380c on master.

[coveralls]

Coverage remained the same when pulling 7c6815f on specifiers_rewrite into 15f380c on master.

[coveralls]

Coverage remained the same when pulling 383e444 on specifiers_rewrite into 15f380c on master.

[coveralls]

Coverage remained the same when pulling 5034ab4 on specifiers_rewrite into 15f380c on master.

[mstimberg]

I fixed some smaller issues (that are only captured by the examples, not by the test suite :-/ ) -- I think it is good to go now if you don't see anything that is still missing.

[thesamovar]

I think given the importance of this I'll go ahead and merge now. The branches that will be affected are Romain's spatial neurons, and my devices. For the devices branch, I think it's no big deal - I think I'll actually start a new branch anyway because I think the way we're doing it we're not actually going to use anything from the current devices branch.

[mstimberg]

The branches that will be affected are Romain's spatial neurons, and my devices.

I can do a manual merge for Romain's branch later, it's not a big deal. And as always: Until we merge master into the spatialneuron branch, it is not affected at all :)

[thesamovar]

Done!