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

Non-contiguous subgroups #1229

Open
wants to merge 65 commits into
base: master
Choose a base branch
from
Open

Non-contiguous subgroups #1229

wants to merge 65 commits into from

Conversation

mstimberg
Copy link
Member

Brian has always allowed subgroups, but these needed to use slicing syntax. While working on the SpatialNeuronGroup (see e.g. #1008) I found that quite limiting, e.g. it would not be possible to have a subgroup representing the somata of several neurons in the group. To a lesser extent, the same problem already exists for SpatialNeuron, e.g. you could not have a subgroup for all distal dendrite compartments.
In the current version, you can work around this limitation by using indices directly. For example, this would work:

distal = neuron.indices['distance>100*um']
neuron.g_na[distal] = ...
neuron.g_kd[distal] = ...
neuron.g_Ca[distal] = ...

This is ok, but it has the disadvantage of being quite redundant, and in general we try to avoid that the user needs to know about indices in the first place. With this PR, you can write instead:

distal = neuron['distance > 100*um']
distal.g_na = ...
distal.g_kd = ...
distal.g_Ca = ...

There's also an argument to make for consistency. Currently:

>>> neurons = NeuronGroup(10, 'v : volt')
>>> neurons.v[:5] = -70*mV
>>> neurons[:5].v = -70*mV  # same effect
>>> neurons.v['i>=5'] = -60*mV
>>> neurons['i>=5'].v = -60*mV
Traceback (most recent call last):
...
TypeError: Subgroups can only be constructed using slicing syntax, a single index, or an array of contiguous indices.

With the new support, the last line would work as well.

Finally, another use case is to record a non-contiguous subset of neurons with a SpikeMonitor. For example to record a random subset of neurons in a non-random network (i.e. where you cannot simply take any subset).

Now, the main arguments against supporting this feature are performance and code complexity. It is true that for SpikeMonitor and PopulationRateMonitor the code is now a bit more complex to also support non-contiguous subgroups, but performance-wise it should still be ok by requiring that subgroup indices are ordered. Of course, if you want to record the first and the last neuron of a large group, this will be quite inefficient.

For now, you cannot create Synapses based on a non-contiguous subgroup, because this complicates the indexing quite a bit. It's perfectly doable, but I wanted to get the basics working first.

Oh, while thinking about the consistency of the different syntaxes of indexing things, I also made two diagrams (the same thing with and without units). I guess it can be useful to explain the difference between neurons[:5].v and neurons.v[:5], but not sure whether it is too detailed for the documentation. Might also go into a blog post one day.

Here they are for reference: https://gist.github.com/mstimberg/25b5058bc85a1f1fbaff958741629ee8

@mstimberg
Copy link
Member Author

@thesamovar Did you have a chance to look at this?

@thesamovar
Copy link
Member

No, will be too busy for a little while I'm afraid with neuromatch and teaching, and this feels like a substantial feature that will require some discussion.

@mstimberg
Copy link
Member Author

Ok, sure thing. Was just making sure that you had seen it.

@thesamovar
Copy link
Member

I haven't forgotten! 😉 But, this is a big one so I need to allocate some substantial time to it...

@mstimberg
Copy link
Member Author

Hi @thesamovar . I updated this PR with latest master, would be great if you could have a look at it. See my initial post for more details about the reasoning. The main functionality is there, the reason why this is still marked as a draft is because it is lacking more tests and documentation before it can be merged.

Copy link
Member

@thesamovar thesamovar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I read the reasoning and a quick look at the code. I think realistically I'm not going to have time to go through the code in detail any time soon, so this might have to do.

I'm slightly reluctant because of potential inefficiencies. Could we potentially raise a warning if users try to record from such a thing that it might be inefficient? Not sure if that's a good idea or not.

Also, is there anything that users would reasonably expect to be able to do with this that wouldn't work either silently (big problem of course) or with an error (ok if it's clear, bad if it's unclear)?

Overall I'm going with approve because I think this is a feature that would be convenient in many circumstances.

mstimberg and others added 30 commits November 22, 2022 12:06
Needs refactoring and documentation
Also fixing a few issues related for standalone
No need to store the individual spikes, we only care about the total number
…roup

# Conflicts:
#	brian2/devices/cpp_standalone/device.py
Only C++ standalone supported until now
Implementation for Cython and numpy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants