[MRG] ENH: interactive connectivity #376

jasmainak · 2021-06-24T01:07:47Z

Not sure if this is useful, could get some feedback before working more on it ...

closes #375

jasmainak · 2021-06-24T03:18:20Z

@ntolley if you want to fetch this PR locally, try to do:

$ git fetch upstream pull/376/head:interactive_connectivity
$ git checkout interactive_connectivity

ntolley · 2021-06-26T18:08:33Z

After messing with this I'm realizing that the method to get the src_gid is still flawed because it relies on the src_range attribute. While the attribute is accurate for the src_type, it does not reflect the exact gids in the specific connection, and instead includes all gids of that type.

The pick_connectivity PR will further help resolve this as it replaces src_range with src_gids, which specific exactly which gids are included without looking at the 'gid_pairs' attribute.

In any case I'm a fan! Is is it possible to label the subplots separately so it's clear which one represents source vs target gids?

jasmainak · 2021-06-29T21:55:18Z

it does not reflect the exact gids in the specific connection, and instead includes all gids of that type.

@ntolley do you have a short script to share that demoes the issue? would be helpful to use it for debugging here

ntolley · 2021-06-30T01:16:23Z

Minimal script to reproduce the issue:

import hnn_core
import os.path as op
from hnn_core import default_network, read_params
from hnn_core.viz import plot_cell_connectivity
hnn_core_root = op.dirname(hnn_core.__file__)

params_fname = op.join(hnn_core_root, 'param', 'default.json')
params = read_params(params_fname)
net = default_network(params, add_drives_from_params=True)

conn_idx = 13
gid_idx = 11
src_gid = net.connectivity[conn_idx]['src_range'][gid_idx]
fig, ax = plot_cell_connectivity(net, conn_idx, src_gid)

ntolley · 2021-06-30T01:18:43Z

This is happening because the src_range of a drive like 'evprox1' includes 1 gid for every cell. So for example, if the connection is only targeting L5 basket cells, and you query a gid that is only connected to L2 pyramidal cells, you'll get a key error.

jasmainak · 2021-07-02T14:18:04Z

@ntolley pushed some fixes. This is my script to debug:

import hnn_core
import os.path as op
from hnn_core import default_network, read_params
from hnn_core.viz import plot_cell_connectivity
hnn_core_root = op.dirname(hnn_core.__file__)

params_fname = op.join(hnn_core_root, 'param', 'default.json')
params = read_params(params_fname)
net = default_network(params, add_drives_from_params=True)

del net.connectivity[-1]
conn_idx = 29
net.add_connection(net.gid_ranges['L2_pyramidal'][::2],
                   'L5_basket', 'soma',
                   'ampa', 0.00025, 1.0, lamtha=3.0,
                   probability=0.8)
fig, ax = plot_cell_connectivity(net, conn_idx)

what do you think? One issue that still remains is that the drives have all their positions at origin ... so it's not possible to visualize them in this way. Currently, we only show two axes if it's not a drive. Any further thoughts?

codecov-commenter · 2021-07-02T16:47:55Z

Codecov Report

Merging #376 (2480ef2) into master (d6b08ff) will increase coverage by 0.03%.
The diff coverage is 91.80%.

❗ Current head 2480ef2 differs from pull request most recent head b1fe671. Consider uploading reports for the commit b1fe671 to get more accurate results

@@            Coverage Diff             @@
##           master     #376      +/-   ##
==========================================
+ Coverage   89.97%   90.01%   +0.03%     
==========================================
  Files          16       16              
  Lines        2934     2965      +31     
==========================================
+ Hits         2640     2669      +29     
- Misses        294      296       +2

Impacted Files	Coverage Δ
hnn_core/viz.py	`83.51% <91.80%> (+0.02%)`	⬆️
hnn_core/parallel_backends.py	`83.71% <0.00%> (+0.85%)`	⬆️

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 d6b08ff...b1fe671. Read the comment docs.

cjayb

This is very cool, especially the _fake_click! +1 to merge

ntolley · 2021-07-06T00:50:59Z

@jasmainak would you prefer some level of interactivity with the drives? I suppose one option would be a slider placed horizontally the bottom that is set to the src_gids of the connections. That way we are not misleading about the location of the drives.

This may be an over-complicated solution to a pretty clean PR. I am ok with leaving it as is, because there's less of a benefit of exploring the drives.

(On the flip side, going the slider route would allow for flipping through different connections in the same figure.)

ntolley · 2021-07-06T00:57:13Z

hnn_core/viz.py

+    return im
+
+
+def plot_cell_connectivity(net, conn_idx, src_gid=None, axes=None,


Update docstring to indicate interactive plot? Also it'd be good to add a notes section describing how to use it.

ntolley · 2021-07-06T00:58:53Z

hnn_core/viz.py

+    # Extract indeces to get position in network
+    # Index in gid range aligns with net.pos_dict
+    target_src_pair = conn['gid_pairs'][src_gid]
+    target_indeces = np.where(np.in1d(target_range, target_src_pair))[0]


Great solution, much cleaner

it's your code, I didn't do anything ... just moved it around :)

ntolley · 2021-07-06T01:03:32Z

hnn_core/viz.py

-    src_gid : int
-        Each cell in a network is uniquely identified by it's "global ID": GID.
-    ax : instance of Axes3D
+    src_gid : int | None


This is a good description, but after writing so many docstrings it's making me think it will be worthwhile creating some consistency for the arguments that appear super often. Sort of in line with the hnn-core glossary we had talked about, these parameters could start with the boilerplate description, and then more function specific text.

I will defer this to a separate PR: #387 :)

ntolley · 2021-07-06T01:04:50Z

hnn_core/viz.py

-    src_idx = np.where(src_range == src_gid)[0][0]
-    src_pos = src_type_pos[src_idx]
+    if src_gid not in valid_src_gids:
+        raise ValueError(f'src_gid not a valid cell ID for this connection '


Suggested change

raise ValueError(f'src_gid not a valid cell ID for this connection '

raise ValueError(f'src_gid {src_gid} not a valid cell ID for this connection '

ntolley · 2021-07-06T01:06:41Z

hnn_core/tests/test_viz.py

@@ -47,9 +55,21 @@ def test_network_visualization():
    with pytest.raises(TypeError, match='src_gid must be an instance of'):
        plot_cell_connectivity(net, conn_idx, src_gid='blah')

-    with pytest.raises(ValueError, match='src_gid not in the'):
+    with pytest.raises(ValueError, match='src_gid not a valid cell ID'):


Suggested change

with pytest.raises(ValueError, match='src_gid not a valid cell ID'):

with pytest.raises(ValueError, match='src_gid -1 not a valid cell ID'):

ntolley · 2021-07-06T01:07:21Z

hnn_core/tests/test_viz.py

        plot_cell_connectivity(net, conn_idx, src_gid=-1)

+    # smoke test interactive clicking
+    del net.connectivity[-1]


why is the deletion necessary?

not strictly necessary but I thought this would be good practice, otherwise the same "kind of connection" would occur in two different net.connectivity elements and the net effect would be a higher weight than we intended?

ntolley · 2021-07-06T01:16:52Z

hnn_core/tests/test_viz.py

+    ax_src = fig.axes[0]
+    pos = net.pos_dict['L2_pyramidal'][2]
+    _fake_click(fig, ax_src, [pos[0], pos[1]])
+


Can you add a test to make sure the figure is actually updated? A really simple one would be to grab the title with fig.texts and assert that it is different after running _fake_click.

you're making me work harder for my merges ;-) It's good! Take a look at the last commit

ntolley · 2021-07-06T23:49:31Z

hnn_core/tests/test_viz.py

+
+    pos = net.pos_dict['L2_pyramidal'][2]
+    _fake_click(fig, ax_src, [pos[0], pos[1]])
+    pos_in_plot = ax_target.collections[2].get_offsets().data[0]


Very fancy! Great unit test

ntolley

This looks great @jasmainak! Happy to merge if you're all wrapped up here.

jasmainak · 2021-07-07T00:39:57Z

Good to go for me!

jasmainak changed the title ~~ENH: interactive connectivity~~ [WIP] ENH: interactive connectivity Jun 27, 2021

jasmainak changed the title ~~[WIP] ENH: interactive connectivity~~ [MRG] ENH: interactive connectivity Jul 2, 2021

jasmainak added 5 commits July 2, 2021 22:37

ENH: interactive connectivity

490ac1f

FIX: not all gids in src_ranges may be valid

39ad1bc

Flake8

d3c7c92

TST with fake clicks

9380d5c

DOC: update whats new

da2b9e7

jasmainak force-pushed the interactive_connectivity branch from add412e to da2b9e7 Compare July 3, 2021 02:38

cjayb approved these changes Jul 5, 2021

View reviewed changes

ntolley reviewed Jul 6, 2021

View reviewed changes

jasmainak mentioned this pull request Jul 6, 2021

consistency between arguments in docstrings #387

Open

ntolley reviewed Jul 6, 2021

View reviewed changes

jasmainak force-pushed the interactive_connectivity branch 3 times, most recently from 77c0445 to d8d7a47 Compare July 6, 2021 21:47

FIX: address nick comments

b1fe671

jasmainak force-pushed the interactive_connectivity branch from d8d7a47 to b1fe671 Compare July 6, 2021 21:51

ntolley reviewed Jul 6, 2021

View reviewed changes

ntolley approved these changes Jul 6, 2021

View reviewed changes

ntolley merged commit a618596 into jonescompneurolab:master Jul 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] ENH: interactive connectivity #376

[MRG] ENH: interactive connectivity #376

jasmainak commented Jun 24, 2021 •

edited

Loading

jasmainak commented Jun 24, 2021

ntolley commented Jun 26, 2021 •

edited

Loading

jasmainak commented Jun 29, 2021

ntolley commented Jun 30, 2021 •

edited

Loading

ntolley commented Jun 30, 2021

jasmainak commented Jul 2, 2021

codecov-commenter commented Jul 2, 2021 •

edited

Loading

cjayb left a comment

ntolley commented Jul 6, 2021 •

edited

Loading

ntolley Jul 6, 2021

ntolley Jul 6, 2021

jasmainak Jul 6, 2021

ntolley Jul 6, 2021

jasmainak Jul 6, 2021

ntolley Jul 6, 2021

ntolley Jul 6, 2021

ntolley Jul 6, 2021

jasmainak Jul 6, 2021

ntolley Jul 6, 2021

jasmainak Jul 6, 2021

ntolley Jul 6, 2021

ntolley left a comment

jasmainak commented Jul 7, 2021

		return im


		def plot_cell_connectivity(net, conn_idx, src_gid=None, axes=None,

	raise ValueError(f'src_gid not a valid cell ID for this connection '
	raise ValueError(f'src_gid {src_gid} not a valid cell ID for this connection '

	with pytest.raises(ValueError, match='src_gid not a valid cell ID'):
	with pytest.raises(ValueError, match='src_gid -1 not a valid cell ID'):

[MRG] ENH: interactive connectivity #376

[MRG] ENH: interactive connectivity #376

Conversation

jasmainak commented Jun 24, 2021 • edited Loading

jasmainak commented Jun 24, 2021

ntolley commented Jun 26, 2021 • edited Loading

jasmainak commented Jun 29, 2021

ntolley commented Jun 30, 2021 • edited Loading

ntolley commented Jun 30, 2021

jasmainak commented Jul 2, 2021

codecov-commenter commented Jul 2, 2021 • edited Loading

Codecov Report

cjayb left a comment

Choose a reason for hiding this comment

ntolley commented Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntolley left a comment

Choose a reason for hiding this comment

jasmainak commented Jul 7, 2021

jasmainak commented Jun 24, 2021 •

edited

Loading

ntolley commented Jun 26, 2021 •

edited

Loading

ntolley commented Jun 30, 2021 •

edited

Loading

codecov-commenter commented Jul 2, 2021 •

edited

Loading

ntolley commented Jul 6, 2021 •

edited

Loading