-
Notifications
You must be signed in to change notification settings - Fork 16
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
fix: Fix targetting class to be neuron compatible #898
base: main
Are you sure you want to change the base?
Conversation
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.
Good addition, just some optimization problems. You create intermediary lists and use N^2 lookup where you probably don't need it
if (model := by_name.get(model_name)) is not None: | ||
ps_ids = list(simdata.placement[model].load_ids()) | ||
ids_here = [ps_ids.index(ids_i) for ids_i in ids if ids_i in ps_ids] | ||
dict_target[model] = simdata.populations[model][ids_here] |
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.
What is this piece of code supposed to do? It's written very inefficiently, if I understand the input/output relation perhaps we can optimize it.
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.
yes, my understanding of ByIdTargetting
is that it should select those cell models with (global) id as in ids
.
Originally ids
was used as index in the subpopulation of each simdata
: simdata.populations[model][ids]
(kind of local id).
Here ps_ids
has the cellmodel ids of that specific simdata
, then ids_here
finds whether and where any of the ids
that we are looking for is in ps_ids
(like np.where).
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.
Could you give a numeric example with let's say 3 or 4 elements per array so I can visualize the transformation?
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.
Let's say ps_ids
is [16, 37, 78]
and the cells that we want to target are ids=[16, 78]
, ids_here
would be [0, 2]
shortcut Co-authored-by: Robin De Schepper <[email protected]>
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.
The ByIdTargetting
is inefficient:
- It uses
if ids in ps_ids
N times, with inside of itps_ids.index
~> O(2 * n^2). This will explode and never finish if someone copies a list of 1000 cell ids into a 100k cell simulation. - It's conceptually going roundabout: it relies on
ps.load_ids
which takes the chunk local ids and combines it with the chunk stats and expands them into global ids, to then look up the position of a global id in the expanded global ids, to then arrive back at the local id.
The fundamental task here is to take an array of global ids and to express it in local ids of a given PS. This is a missing piece of logic it seems (@drodarie could you look through the PS/CS and ConnectivitySetIterator
interface classes and bsb-hdf5
.. I feel like I wrote this already).
If you want to implement this efficiently, you would have to write a new piece of logic in bsb-hdf5
and/or NeuronPopulation
that uses the PlacementSet
s chunks
, and the chunk_stats
and uses simple arithmetics to calculate the local id of each global id. @drodarie could you help Danilo piece this together?
Describe the work done
CellTypeFilter
whenget_targets()
method is called.