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

[MRG] Add function to visualize connectivity weights #339

Merged
merged 23 commits into from Jun 5, 2021

Conversation

ntolley
Copy link
Contributor

@ntolley ntolley commented May 26, 2021

This has been on the to-do for awhile. I've added a function to visualize the connectivity weights so that the gaussian decay with distance is visible. Since gids are arranged in a grid, it produces some interesting repeating patterns:

image
image

I still need to add some quick smoke tests, but I am satisfied with the current implementation of the function and definitely ready for some reviews.

@codecov-commenter
Copy link

codecov-commenter commented May 26, 2021

Codecov Report

Merging #339 (9d1d93c) into master (0f96ef6) will increase coverage by 1.25%.
The diff coverage is 98.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #339      +/-   ##
==========================================
+ Coverage   89.80%   91.06%   +1.25%     
==========================================
  Files          13       13              
  Lines        2462     2540      +78     
==========================================
+ Hits         2211     2313     +102     
+ Misses        251      227      -24     
Impacted Files Coverage Δ
hnn_core/viz.py 90.80% <98.79%> (+9.53%) ⬆️
hnn_core/cell.py 97.25% <100.00%> (+0.06%) ⬆️
hnn_core/network.py 93.14% <100.00%> (+0.18%) ⬆️
hnn_core/parallel_backends.py 83.57% <0.00%> (+0.86%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f96ef6...9d1d93c. Read the comment docs.

hnn_core/viz.py Outdated Show resolved Hide resolved
hnn_core/viz.py Outdated
net : Instance of Network object
The Network object
conn_idx : int
Index of _Connectivity object to be visualized
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we avoid talking about _Connectivity since it's a private object? You can just say net.connectivity. It's just a dict for most users

hnn_core/viz.py Outdated
Comment on lines 618 to 624
# Identical calculation used in Cell.par_connect_from_src()
dx = src_pos[0] - target_pos[0]
dy = src_pos[1] - target_pos[1]
d = np.sqrt(dx**2 + dy**2)

weight = nc_dict['A_weight'] * \
np.exp(-(d**2) / (nc_dict['lamtha']**2))
Copy link
Collaborator

Choose a reason for hiding this comment

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

use a private function that is used by both?

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe @rythorpe has more thoughts here. But it's a bit odd that the weight computation is done at the cell level. I think it's more a network property. One potential direction is to have _parconnect_from_src the following signature:

_parconnect_from_src(gid_presyn, postsyn, weight, delay, threshold)

that is pre-compute nc.weight and nc.delay in NetworkBuilder

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe @rythorpe has more thoughts here. But it's a bit odd that the weight computation is done at the cell level. I think it's more a network property. One potential direction is to have _parconnect_from_src the following signature:

_parconnect_from_src(gid_presyn, postsyn, weight, delay, threshold)

that is pre-compute nc.weight and nc.delay in NetworkBuilder

Did this get resolved yet? I'm just getting caught up and am unclear whether this issue is still a WIP. cc @jasmainak @ntolley

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this was a more general comment I made. Probably only tangentially related to the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a good point though and I agree with you. We should be mindful of this moving forward!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed this comment, this seems like a reasonable short term solution. But I think Ryan's point on separating cell/network functions requires some more thought.

hnn_core/viz.py Outdated
# Identical calculation used in Cell.par_connect_from_src()
dx = src_pos[0] - target_pos[0]
dy = src_pos[1] - target_pos[1]
d = np.sqrt(dx**2 + dy**2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

try to avoid single letter variable names, hard to grep and replace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, this is copied directly from cell.py so it can be changed with a common function. Any preference for where that function would live? It could just be moved out of the class and remain in cell.py.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah I think that sounds reasonable to me

hnn_core/viz.py Outdated
@@ -569,13 +569,87 @@ def plot_cell_morphology(cell, ax, show=True):
return ax


def plot_connectivity_weights(net, conn_idx, ax=None, show=True):
Copy link
Collaborator

@jasmainak jasmainak May 26, 2021

Choose a reason for hiding this comment

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

should we still have the plot_connectivity_matrix function? Shouldn't it be absorbed into this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was realizing it is pretty superfluous while writing this. I'll add a flag that skips the weight computation and just plots black/white squares.

Then this can be renamed plot_connectivity_matrix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you might want to ignore this comment now since I understand the situation better now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this function is still relevant after getting the cell-specific plotting function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly I am still leaning towards my previous comment in this thread. While the weight visualization here is obviously not as intuitive as cell-specific plotting, it seems weird not to include the functionality.

I'm planning to remove the "black and white" matrix function, and rename this function to plot_connectivity_matrix. Then I can set a show_weights=False flag for the default behavior.

@jasmainak
Copy link
Collaborator

jasmainak commented May 26, 2021

I don't understand the grid structure @ntolley . Could you explain more? Should the x/y axis be physical distance of the cells so people can relate it to the lamtha parameter?

@jasmainak
Copy link
Collaborator

Looks very promising by the way. Apologies for so many comments, it just reflects my excitement :)

@ntolley
Copy link
Contributor Author

ntolley commented May 26, 2021

Should the x/y axis be physical distance of the cells so people can relate it to the lamtha parameter?

This not possible if you want to visualize all connection weights. The gids are arranged like so:

1  2  3  4
5  6  7  8
9  10 11 12
13 14 15 16

If you consider matrix entries at (1,1), (1,2), (1,3), (1,4) sure that direction involves increasing distances and you will see a gaussian decay. But at the next entry (1,5) the distance is equivalent to (1,2).

What you're probably envisioning is defining a single gid as the reference, and then plotting the weights with respect to that. In that case the axes could correspond to physical distances, and the plot would appear as a bivariate gaussian distribution.

@ntolley
Copy link
Contributor Author

ntolley commented May 26, 2021

@jasmainak what do you think about an additional

plot_gid_connectivity(net, conn_idx, gid)

?

@jasmainak
Copy link
Collaborator

indeed, I get it now. Yeah, I guess we'll need two different functions. Perhaps call it plot_cell_connectivity(net, conn_idx, src_gid)

it's a bit odd to use indices (as opposed to the name of the cell population) for this, but let's make this work first and then we can think of nicer solutions

@ntolley
Copy link
Contributor Author

ntolley commented May 26, 2021

Indeces are definitely awkward at the moment, but I was thinking this will be a lot cleaner after implementing that "pick" method we had discussed. Something like:

con_indeces = net.pick_connectivity(src_type='L2_pyramidal', target_type='L2_basket', ...)
for con_idx in con_indeces:
    plot_connectivity_matrix(net, con_idx)

This way we don't have to overwhelm the viz function with a bunch of methods filtering for desired gids.

@jasmainak
Copy link
Collaborator

indeed that seems like the way to go!

@ntolley
Copy link
Contributor Author

ntolley commented May 27, 2021

Most recent commits move the gaussian connection calculations into separate functions. Will add the function to visualize weights with respect to a specific gid soon.

@ntolley
Copy link
Contributor Author

ntolley commented Jun 2, 2021

For visualizing single cell connection weights, I decided to go with a scatter plot rather than a matrix. Since not all of our cells lies in a perfect grid (basket cells), reading X/Y position off a matrix would be misleading:
image
image

@jasmainak
Copy link
Collaborator

Fantastic, love it! Didn't realize our cells were not in a perfect grid ...

@jasmainak
Copy link
Collaborator

diff looks clean as such. Any test to update?

hnn_core/viz.py Outdated
Comment on lines 687 to 698
def plot_cell_connectivity(net, conn_idx, src_gid, ax=None, show=True):
"""Plot synaptic weight of connections from a src_gid.

Parameters
----------
net : Instance of Network object
The Network object
conn_idx : int
Index of connection to be visualized
from `net.connectivity`
src_gid : int
Each cell in a network is uniquely identified by it's "global ID": GID.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm in doubt what this function does. Could you be more explicit about the first two arguments? I don't understand which int's to enter. Loving the output though!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely will add a better docstring. conn_idx is awkward to work with at the moment as it is really just the index of the connection you'd like to visualize in net.connectivity[conn_idx]. Unfortunately the alternative of passing the _Connectivity object directly isn't much better.

I think this awkwardness will be resolved when I implement a utility function discussed with @jasmainak. It will look like

net.pick_connectivity(src_type='L5_pyramidal', receptor='ampa',...)

and return the indeces of all connections that match the provided arguments.

Comment on lines +688 to +689
from .network import Network
from .cell import _get_gaussian_connection
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't nest this if possible ... not sure if you have a circular import issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just checked, unfortunately the nesting is required here as well.

@ntolley
Copy link
Contributor Author

ntolley commented Jun 4, 2021

Thanks for the reviews everyone! I have added a few features to make this a bit more robust. First, target cells that aren't connected to the src_gid, but still belong to the target_type are visualized as black dots:
image
This can arise during probabilistic connections, as well as drives where unique=True.

I have also omitted plotting the src_gid position when the connection corresponds to a drive since its position is rather arbitrary. I believe I have addressed all of the comments but please feel free to add any more suggestions! Otherwise I'm happy with the state of the PR once the CI's go green.

@@ -24,6 +24,8 @@ Changelog

- Add probability argument to :func:`~hnn_core.Network.add_connection`. Connectivity patterns can also be visualized with :func:`~hnn_core.viz.plot_connectivity_matrix`, by `Nick Tolley`_ in `#318 <https://github.com/jonescompneurolab/hnn-core/pull/318>`_

- Add function to visualize connections originating from individual cells :func:`~hnn_core.viz.plot_cell_connectivity`, by `Nick Tolley`_ in `#339 <https://github.com/jonescompneurolab/hnn-core/pull/339>`_
Copy link
Collaborator

Choose a reason for hiding this comment

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

add to api.rst ?

hnn_core/viz.py Outdated
# Gather positions of all gids in target_type.
x_pos = [target_type_pos[idx][0] for idx in range(len(target_type_pos))]
y_pos = [target_type_pos[idx][1] for idx in range(len(target_type_pos))]
ax.scatter(x_pos, y_pos, color='k', zorder=-1, s=4)
Copy link
Collaborator

Choose a reason for hiding this comment

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

just a thought, should we make this a little greyish to that people are not confused that the size stands for something ... it was not apparent to me because the red dot was different size, then there were the black dots and then there was the connectivitiy dots all different sizes. Maybe you can also think of changing the symbol of the src_gid dot from circle to square or something. Sorry, I know this is a bit bikeshedding at this point ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestions. I think to be more explicit, I'll do a red square for src_gid, and black x's for the non-connected cells.

@jasmainak jasmainak merged commit c65bc1e into jonescompneurolab:master Jun 5, 2021
@jasmainak
Copy link
Collaborator

Merged, thanks @ntolley ! Connectivity API gets better and better. What's next?!

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

5 participants