-
Notifications
You must be signed in to change notification settings - Fork 222
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
base: master
Are you sure you want to change the base?
Non-contiguous subgroups #1229
Conversation
Warn to notify user if not sorted already
Needs more testing
Instead of raising a warning
@thesamovar Did you have a chance to look at this? |
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. |
Ok, sure thing. Was just making sure that you had seen it. |
# Conflicts: # brian2/groups/subgroup.py # brian2/monitors/ratemonitor.py
I haven't forgotten! 😉 But, this is a big one so I need to allocate some substantial time to it... |
Hi @thesamovar . I updated this PR with latest |
There was a problem hiding this 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.
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
Changes from commit fe0add7 got lost in merge
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 forSpatialNeuron
, 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:
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:
There's also an argument to make for consistency. Currently:
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
andPopulationRateMonitor
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
andneurons.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