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
[MRG] Add functions to modify and visualize connectivity #318
[MRG] Add functions to modify and visualize connectivity #318
Conversation
Codecov Report
@@ Coverage Diff @@
## master #318 +/- ##
==========================================
- Coverage 90.50% 89.90% -0.61%
==========================================
Files 13 13
Lines 2434 2497 +63
==========================================
+ Hits 2203 2245 +42
- Misses 231 252 +21
Continue to review full report at Codecov.
|
examples/plot_connectivity.py
Outdated
for conn in net.connectivity: | ||
if conn['src_type'] == 'L2_basket': | ||
conn.update_probability(0.1) |
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.
Can we use the net.add_connection(..., probability)
method so we can still play around with _Connectivity
class. I suspect we'll need to make changes when we unify it with the drive connectivity
I like that connectivity is becoming a proper object slowly! But it's not yet clear to me if the object we expose to the user stores the "list of connectivities" or just a single connectivity instance? It seems to me it might be more useful to do: net.connectivity.plot() # this will show all the connectivities as subplots?
net.connectivity.filter(drop_probability, src_types, target_types) etc. In order to not let this decision slow down the development, I suggest starting from functions |
@ntolley are you familiar with the Iris dataset plot? I am imagining something like this might be useful to show connections between different cell classes? Instead of scatterplot, you'll have matshow/imshow |
Great suggestion! I was actually just about to gripe that there is too much to plot for the default network, but filtering to a subset of connections might work here. This sort of plot would require fixing the In general though I would like to emphasize the list of connections (or dict of connections that allows efficient indexing) rather than a generic My rationale is that the "canonical" neocortical circuit is characterized by a unique set of connectivity rules. I think the API should reflect this conserved circuit pattern, and allow users to interact with it. For me the motivation is to probe what makes these connection rules so special, not so much looking at how a singular cell class impacts cortical function. |
okay I see, makes sense. How have neuro papers made plots like this? Do you have any examples? Might be good to conform to the expectations of most users ... |
Interestingly neither the Blue Brain Project or the Brain Modeling Toolkit plot connectivity matrices as we've discussed. Instead they have matrices of connection probabilities, where the rows/columns are the different cell classes. While these are some the most closely related projects, their emphasis on 1:1 morphologic reconstructions makes the visualizations not very relevant. Papers constructing functional networks with single unit recordings will likely be the best place mimic visualizations. Mainly because the method forces grouping cells into broad categories (and there's only 4 in HNN). This Scherberger Paper does a nice plot showing how single units from different areas are connected: Also Olaf Sporns has done a lot of work applying graph theory to neural networks, and his papers may have more visualization examples. However, his focus is on inter-areal connectivitiy rather than single neurons. |
I like the subplots A and B in the figure you share above. Agree that we don't want to go the Blue Brain way ... But are we going to colorcode and show lamtha? I still have difficulty visualizing how the weights are chosen and how these weights drive the currents. Agree that some domain knowledge is missing on my part. I am thinking that there needs to be a diagram like one of these at some point: where the thickness of the line indicates the weight/probability and color indicates if it's inhibitory/excitatory? And the populations are arranged spatially as they would be in a cortical column. That way you can literally take the diagram and explain to someone what the mechanism is ... For now let's go with A (and optionally B) though |
examples/plot_connectivity.py
Outdated
|
||
net_remove = net.copy() | ||
dpl_remove = simulate_dipole(net_remove, n_trials=1) | ||
net_remove.cell_response.plot_spikes_raster() | ||
net.connectivity[10].plot() |
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.
can you change the sphinx gallery thumbnail to point to this plot?
examples/plot_connectivity.py
Outdated
for conn in net.connectivity: | ||
if conn['src_type'] == 'L2_basket': | ||
conn.drop(0.1) |
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.
connectivity
is a private class. You shouldn't expose it (yet) ... could you perhaps expose probability
using add_connection
? This feels like you're trying to modify the connection but we're still trying to settle on API for adding a connection
hnn_core/network.py
Outdated
The probability attribute will store the most recent value passed to | ||
this function. As such, this number does not accurately describe the | ||
connections probability of the original set after successive calls. |
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.
keep the complexity bounded, otherwise software will become unmaintainable. You should try to address 80 percent usecases. Look up feature creep. I'd suggest throwing an error if someone tries to drop connections more than once
hnn_core/network.py
Outdated
@@ -1170,6 +1196,74 @@ def __repr__(self): | |||
|
|||
return entr | |||
|
|||
def drop(self, probability): |
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.
honestly I would keep this for future PR. I think you can add probability
to add_connection
with 1 or 2 lines and expose it in an example. This falls under the usecase of modifying existing connections which we haven't started looking into yet.
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.
But we'll probably want to modify the "default" connections probabilistically, too?
examples/plot_connectivity.py
Outdated
# spiking. L2 basket and pyramidal cells rhythymically fire in the gamma | ||
# range (30-80 Hz). As a final step, we can see how this change in spiking | ||
# activity impacts the aggregate current dipole. | ||
# Adding a single inhibitory connection didn't completely restored the normal |
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.
# Adding a single inhibitory connection didn't completely restored the normal | |
# Adding a single inhibitory connection did not completely restore the normal |
hnn_core/network.py
Outdated
|
||
Parameters | ||
---------- | ||
conn : Instance of _Connectivity object |
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.
This is now a class method, document ax
and show
instead
How about adding an argument to |
related idea is to move from hnn_core import default_network
network = default_network(params) and then: def default_network(params):
net = Network()
net.add_connection(...)
return net That way the |
@ntolley you need a rebase here |
Can do! I'm a bit underwater with research projects until mid next week but I can focus on wrapping up this PR afterwards. |
991dd91
to
60b45a6
Compare
The default network is now created with I think all that is left to do is to fix the connectivity example. Since we are not supporting modifications of existing connectivity, I think this will have to be turned into the example suggested by @cjayb where we create a network (not necessarily the default!) with successive calls to In that case it will likely be useful to move away from instantiating the entire default network, but just add a few sparse connections and show off the plotting functions, as well as how they behave in simulation. |
Can you make the CIs happy so I can see how the example looks like? Also @ntolley would you have some time to brainstorm the connection API over zoom sometime? I would like to make sure that we're not overseeing any critical usecases |
Just wanted to mention that as such the PR looks good. But overall, we need to think a bit about the connectivity API and what needs to be done to make sure users have the best experience |
Just finished updating the example to be more appropriate for the current API. I've opted to create hand-made networks and compare their activity patterns. Also I have a super free schedule for zoom Wed-Fri next week! |
In terms of critical use cases, two things come to mind. The first is implementing the src-target definition conventions in the drives API, allowing for drives to only a subset of cells. In my own research/reading. In my own research/reading this abililty seems fairly important for understanding circuit-EEG activity (how does the dipole look when subpopulations are driven from indepedent sources?). The second thing we should consider is adding connection probabilities that decay with distance. While a fixed connection probability is fine for now, distance dependent probability is much more biologically realistic topology. My understanding is that the motivation for Gaussian decaying synaptic strengths was to generally how net synaptic inputs from one location fall off with distance. In reality though the strength of a synaptic connection isn't really influenced by distance (as far as I know...). |
Cool, could you send me a zoom/calendar invite on one of those days? I'm pretty free. Everything works except Wednesday 9 am |
e1276c8
to
1ea1fa9
Compare
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.
Looks great! I have a few minor comments/questions, sorry about being last-minute.
examples/plot_connectivity.py
Outdated
src_gid = net.gid_ranges['L2_basket'][0] | ||
target_gids = 'L2_pyramidal' | ||
|
||
net_all = Network(params, add_drives_from_params=True) |
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.
Maybe it's because I'm an "old" user, but it's quite confusing to me that a "Network
" can be initialised without connections. That's not really a network, but rather a bag of cells, right?
And why is this called net_all
? Isn't it more like net_none
? I really am confused...
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.
net_all
refers to the default all-to-all connectivity pattern which is later compared with a net_sparse
pattern.
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.
I see why the terminology is a bit confusing, but I am not sure how to get around it without reintroducing the add_connection
steps back to the __init__
.
I think one way this could be improved is a more robust way to remove connections. Something like
net = default_network(params)
net.clear_connectivity(remove_drives=False)
This way the user never really has to deal with the Network
constructor.
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.
Or how about
net = default_network(params, connect=True, attach_drives=True)
I don't know if it should be a priority to avoid Network
? There's just something odd about the example right now in the way that default_network
and Network
are used.
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.
@jasmainak did you have a vision in mind for how default_network()
would be used? Going down this path, it seems like we're just recreating the original Network.__init__
in some other location.
In my mind instantiating an empty network should require some intention by the user as very few people have the background to create a realistic network de novo.
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.
Also I think this brings up a more important question of how different "supported" templates will be imported. For example if we create a well tuned sparse network, will there be a sparse_network(params)
function? Or calcium_network(params)
...
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.
see my other comment. I think Network
should be generalized a bit and default_network
is where the heavy lifting of the params
extraction should be done. I want the network class to go the same way as we did with the Cell
class. I can't make up my mind whether we should have different param
file specifications for every new network or if we can leverage existing specifications. I think the latter. However, once loaded into HNN-core, it should be represented with our Network
class which is fairly generalizable.
@ntolley don't tag anyone in commits -- they'll get notified every time you rebase and push that commit |
😨 sorry chris! We'll try and get this merged soon haha @jasmainak I am adding methods to clear connectivity and drives separately to avoid using |
@ntolley are you aware of this paper: https://www.biorxiv.org/content/biorxiv/early/2021/04/18/2021.04.16.440210.full.pdf ? @stephanie-r-jones mentioned to me that the connectivity is changed in there. Could that be a good example for us? |
@jasmainak regarding our previous discussion I was wrong. We can totally add that new connection with the current API. Here's the relevant line from the supplemental:
I just checked and this is easily done in the model. |
Nice! Is there something from the paper that we can easily demo/illustrate due to the new connectivity? I'm fine leaving this for another PR though if you think it's outside the scope. |
Let's leave it for another PR, I'll make it my next one to focus on. I agree that it will be great to show off as many examples as possible with published (almost) results. It's possible the influence of beta events on ERP's is partially present with just this new connection, but I'll need to play with the code some more. There were a lot of modified parameters in this paper so it may require a separate params file. Of course this can be more easily done with #321 merged! |
Notes | ||
---- | ||
`net = default_network(params)` is the reccomended path for creating a | ||
network. Instantiating the network as `net = Network(params)` will |
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.
we should migrate the stuff that is specific to params
specification to default_network
slowly leaving a generalized Network
class in place. That is to say, it should not take a params
dictionary as input and should not have add_drives_from_params
. Happy to delegate this to another PR though
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.
I agree this is a great direction to go. Let's iterate on generalising Network
and removing add_drives_from_params
together with the legacy
-flag.
@@ -201,8 +344,6 @@ def __init__(self, params, add_drives_from_params=False, | |||
self.n_cells = sum(len(self.pos_dict[src]) for src in | |||
self.cell_types) | |||
|
|||
self._set_default_connections() | |||
|
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.
in a future PR, add_drives_from_params
should be moved to default_network
function
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.
looks good other than nitpicks. Can you address them and I'll merge first thing tomorrow morning.
Is there anything to update in whats_new
@ntolley ?
Co-authored-by: Mainak Jas <jasmainak@users.noreply.github.com>
I believe I've addressed the remaining comments! Let me know if anything else catches your eye, otherwise feel free to merge. |
Thanks @ntolley ! |
At long last we're here! This PR is meant to flush out the API for modifying and visualizing the connectivity now that it has been decoupled from
network_builder.py
(#276), and the data structure is lightweight and interpretable (#313).Thanks to @jasmainak's suggestion to store separate connection types as objects, there is now a very clean path towards modifying existing connections. To demonstrate, this PR enables modification of existing connections with
net.connectivity[idx].update_probability(0.5)
. I have additionally added aprobability
argument tonet.add_connection
.Here are my currently planned functions to add:
net.connectivity[idx].plot
which will produce a "checkerboard" to visualize connectionsnet.connectivity[idx].update_srcs
andnet.connectivity[idx].update_targets
to filter connections to a subset of srcs and targets. This will be really useful for modifying drives to the network that target different sub populations of cells.Let me know if you guys have any other suggestions!
P.S. I think this mode of modifying connections will be quite amenable to the GUI since it modifies everything in place.