[MRG+1] unify cell connectivity and drive connectivity #369

jasmainak · 2021-06-18T23:32:59Z

This is a very draft version, @cjayb do you have any objection to this kind of direction?

I think it will depend on some changes @ntolley is making in #348

rythorpe · 2021-07-07T14:11:04Z

Is this on the docket for our 0.2 release? I'm happy to work on this PR and keep the momentum of #383 going as I noticed there is a lot of unnecessary bulk involved in assigning drive connectivity.

jasmainak · 2021-07-07T14:26:29Z

I think it would be great to have! I'm always trying to motivate folks to keep the code in shape as we go rather than only add new features.

jasmainak · 2021-07-20T02:21:57Z

@rythorpe this would be a good one to get your help on if you're interested. I can take care of #315

rythorpe · 2021-07-23T20:54:01Z

Just rebased, but I'll work on this more throughout next week.

rythorpe · 2021-07-29T19:00:53Z

I'm learning that _create_drive_conns() helped enforce legacy mode behavior by accepting target_populations as an argument and creating connectivity specifications for each target cell type independent of whether or not a real connection (i.e., as stored in net.connectivity) was created. By now relying on net.connectivity to determine which drive cells target which network cells in _instantiate_drives(), we've made it much more difficult to maintain legacy mode. I think I can work around it, but be prepared for some janky hacks.

hnn_core/network.py

jasmainak · 2021-07-29T19:59:23Z

let's make it work first. I think some helper functions might be necessary but having the connectivity object unified across drives and cells will be really beneficial in terms of how we think about things!

cjayb · 2021-07-30T07:21:46Z

I think I can work around it, but be prepared for some janky hacks.

Do you need a workaround, or just a nuke? _legacy_mode was necessary to get all the gids to line up, but given all the other legacy-breaking changes in 0.2, it seems pointless to try to fix anything here. Consolidating on he new connectivity API is surely the only strategy that makes sense.

…ghts

rythorpe · 2021-08-13T01:19:10Z

I've got all the tests passing except for the following connectivity test in test_network.py. Is there systematic way to update this @ntolley without having to manually determine each correct connectivity index?

        # List of tuples with (arg, item, correct_indices)
        kwargs_good = [
            ('src_gids', 0, [30, 39]),
            ('src_gids', 'L2_pyramidal', [29]),
            ('src_gids', range(2), [30, 39]),
            ('src_gids', None, [4, 29, 30, 39]),
            ('target_gids', 35, [22, 34, 35, 37, 38, 40, 41, 42, 43, 44]),
            ('target_gids', range(2), [30, 39]),
            ('target_gids', 'L2_pyramidal',
             [22, 34, 35, 37, 38, 40, 41, 42, 43, 44]),
            ('target_gids', None,
             [22, 28, 30, 34, 35, 37, 38, 39, 40, 41, 42, 43, 44]),
            ('loc', 'soma', [30, 39]),
            ('loc', None, [30, 39]),
            ('receptor', 'gabaa', [30, 39]),
            ('receptor', None, [30, 39])]
        for arg, item, indices in kwargs_good:
            kwargs = kwargs_default.copy()
            kwargs[arg] = item
>           assert indices == pick_connection(**kwargs)
E           assert [30, 39] == [62, 71]
E             At index 0 diff: 30 != 62
E             Use -v to get the full diff

ntolley · 2021-08-13T01:38:00Z

You're telling me you don't appreciate my mountain of hard coded tests? 😄

Yeah this is a pretty gross solution, I'll push some commits to this branch tomorrow with a more sensible test. I know it's not good practice to change tests to satisfy a PR, but it's worth it in this case.

jasmainak · 2021-08-13T01:39:23Z

I’m getting deja vu 😂

On Thu 12 Aug 2021 at 21:38, Nick Tolley ***@***.***> wrote: You're telling me you don't appreciate my mountain of hard coded tests? 😄 Yeah this is a pretty gross solution, I'll push some commits to this branch tomorrow with a more sensible test. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#369 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADY6FIUFKX2BHSPKSOEX7BLT4RZQHANCNFSM466O7HXA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

-- Sent from my iPhone

rythorpe · 2021-08-13T01:43:39Z

You're telling me you don't appreciate my mountain of hard coded tests? smile

Yeah this is a pretty gross solution, I'll push some commits to this branch tomorrow with a more sensible test. I know it's not good practice to change tests to satisfy a PR, but it's worth it in this case.

I've got to admit, I'm pretty sure my heart rate spiked the moment I saw this test fail. Sounds good, thanks Nick!

hnn_core/drives.py

jasmainak · 2021-08-16T16:17:09Z

hnn_core/tests/test_network.py

+    # 15 drive connections are added as follows: evdist1: 3 ampa + 3 nmda,
+    # evprox1: 4 ampa, evprox2: 4 ampa, and 1 extra zero-weighted ampa
+    # evdist1->L5_basket connection is added to comply with legacy_mode
+    assert len(net_default.connectivity) == n_conn + 15


could we do legacy_mode = False in all/most of our tests unless we are explicitly testing the legacy mode ?

Mostly yes. Aside from the tests like this one that explicitly test against a hardcoded value, I've tried to write as many of my tests as possible to accommodate either scenario. That said, it's something we should probably dedicate a whole PR to (see #393).

jasmainak · 2021-08-16T16:21:07Z

hnn_core/network.py

+                                        location, receptor, weights, delays,
+                                        space_constant)
+            else:
+                src_gids = list(self.gid_ranges[name])


Suggested change

src_gids = list(self.gid_ranges[name])

shouldn't need this, no? You can just provide name to add_connection ?

That only works for names in Network.cell_types. We could modify this validation step in add_connection()?

I think we should ... it should allow cell_types as well as external_drives.keys()

jasmainak · 2021-08-16T16:25:47Z

I think this looks great @rythorpe . One concern I have in my mind is if the net.connectivity is still easy to inspect since we have many drives and each connectivity element represents one target population and one receptor. So we end up having n_drives x n_target_populations x n_receptor connections. I guess nicer slicing and repr of this object could be future PRs

jasmainak · 2021-08-16T16:27:43Z

Oh, also you have flake8 errors. It would be good to bring the PR to MRG state. It looks pretty clean as such

rythorpe · 2021-08-16T18:20:56Z

I think this looks great @rythorpe . One concern I have in my mind is if the net.connectivity is still easy to inspect since we have many drives and each connectivity element represents one target population and one receptor. So we end up having n_drives x n_target_populations x n_receptor connections. I guess nicer slicing and repr of this object could be future PRs

That's pretty much what we have now, although it is ominous given the sheer number of connections. What did you have in mind?

codecov-commenter · 2021-08-16T20:28:13Z

Codecov Report

Merging #369 (c8c334e) into master (367a883) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head c8c334e differs from pull request most recent head 22f5906. Consider uploading reports for the commit 22f5906 to get more accurate results

@@            Coverage Diff             @@
##           master     #369      +/-   ##
==========================================
+ Coverage   90.26%   90.30%   +0.03%     
==========================================
  Files          17       17              
  Lines        3062     3054       -8     
==========================================
- Hits         2764     2758       -6     
+ Misses        298      296       -2

Impacted Files	Coverage Δ
hnn_core/drives.py	`92.77% <100.00%> (+0.66%)`	⬆️
hnn_core/network.py	`91.70% <100.00%> (-0.30%)`	⬇️
hnn_core/network_builder.py	`94.23% <100.00%> (+0.09%)`	⬆️
hnn_core/params.py	`91.15% <100.00%> (+1.53%)`	⬆️
hnn_core/parallel_backends.py	`82.80% <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 367a883...22f5906. Read the comment docs.

jasmainak · 2021-08-16T20:31:44Z

What did you have in mind?

I guess it's fine for now, we can always do:

net.connectivity[pick_connectivity(src_gids='L2_Pyramidal')]

maybe:

net.connectivity.pick(src_gids='L2_Pyramidal')

would have been cleaner but it would need us to define a new object etc., so not sure if it's worth the effort
?

rythorpe · 2021-08-16T22:02:58Z

What did you have in mind?

I guess it's fine for now, we can always do:
net.connectivity[pick_connectivity(src_gids='L2_Pyramidal')]
maybe:
net.connectivity.pick(src_gids='L2_Pyramidal')
would have been cleaner but it would need us to define a new object etc., so not sure if it's worth the effort
?

I think this deserves it's own discussion/PR along with any renaming we do to pick_connection() as I now realize it is used in a lot of places and would blow up this PR.

jasmainak · 2021-08-17T01:55:57Z

yeah, agree that should be a different PR :)

jasmainak · 2021-08-17T01:58:53Z

doc/whats_new.rst

@@ -40,6 +40,8 @@ Changelog

 - Add :func:`~hnn_core.Network.pick_connection` to query the indices of specific connections in :attr:`~hnn_core.Network.connectivity`, by `Nick Tolley`_ in `#367 <https://github.com/jonescompneurolab/hnn-core/pull/367>`_

+- Unify the codepath for instantiating drive and local network connectivity, by `Ryan Thorpe`_, `Mainak Jas`_, and `Nick Tolley`_ in `#369 <https://github.com/jonescompneurolab/hnn-core/pull/369>`_


You should think of "what affects the user" when updating this. User does not care about code path. They care about:

change in behavior (e.g., with 0 weights in param files, you now have empty connections?). That's a bug fix.

change in net.connectivity. It has more elements now and probably not in the same order. If someone hard coded net.connectivity[32] it would now break. So that's an API change.

change in drive['conns']. It no longer exists. If someone relied on it in their code, it would break now

What do you think of 3663826 @jasmainak?

Looks good!

jasmainak · 2021-08-17T02:02:29Z

hnn_core/tests/test_network.py

@@ -389,7 +411,7 @@ def test_network():
    # Test removing connections from net.connectivity
    # Needs to be updated if number of drives change in preceeding tests
    net.clear_connectivity()
-    assert len(net.connectivity) == 18
+    assert len(net.connectivity) == 50


I am confused what's going on here. Isn't clear_connectivity going to remove all the connections? Then why do we still have 50 connections?

clear_connectivity clears local network connectivity, not the drive connectivity. Maybe we should update the name accordingly.

yeah updating the name sounds good. Either in this PR or another, as you feel like.

hnn_core/tests/test_network.py

jasmainak · 2021-08-17T02:14:43Z

It looks like I can't approve the pull request since I started it. So, I went back to "old ways" and edited the title. @ntolley or @cjayb feel free to merge once you are happy

Co-authored-by: Mainak Jas <[email protected]>

ntolley · 2021-08-18T13:14:04Z

Apologies for not keeping up too closely with the PR as it progressed, but I am definitely a fan of all the changes. Luckily for @rythorpe I really don't have nitpicks, it was so obvious that these redundant data structures needed to be merged and it's done super clean here.

Most of the issues I have are not with the PR, and have more to with general structure of net.connectivity which as mentioned before warrants some discussion on if this is the data structure we want to commit to. In any case great work!!

jasmainak mentioned this pull request Jul 6, 2021

[MRG] attach drives with a user-defined number of source (artificial) cells #383

Merged

This was referenced Jul 14, 2021

Clean up connectivity API and add tests #293

Closed

release 0.2 #365

Closed

rythorpe force-pushed the unify_connectivity branch from af96060 to 4e2d274 Compare July 23, 2021 20:50

rythorpe force-pushed the unify_connectivity branch from 94d8b5a to 91c03be Compare July 29, 2021 18:43

jasmainak commented Jul 29, 2021

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

jasmainak and others added 11 commits August 12, 2021 00:43

WIP: unify cell connectivity and drive connectivity

2790911

fix src and target gids lists that get entered into add_connection

829d144

instantiate drives according to drive drive cell from gid_ranges

3c68bcf

remove refs to drive['conn'] in network_builder

8cf01d5

update drive and network tests

48a1c07

legacy mode: set previously unspecified weight and delay parameters

87e0512

restructure weight and delay dicts in _attach_drive()

1c1b118

raise error when drives are added without specifying any synaptic wei…

50ea4bb

…ghts

update test_drives.py

fc98b8c

fix drive weight_by_type dict

ce3e83e

use gid_pairs when finding target gids in _gid_assign

e134695

rythorpe force-pushed the unify_connectivity branch from 0a4de98 to e134695 Compare August 13, 2021 01:13

rename _get_target_population_properties() to _get_target_properties()

7a2b11f

jasmainak commented Aug 16, 2021

View reviewed changes

hnn_core/drives.py Outdated Show resolved Hide resolved

jasmainak commented Aug 16, 2021

View reviewed changes

rythorpe added 4 commits August 16, 2021 14:23

clean up pick_connection test

7d83505

address comments

0810052

add test for when pick_connection returns empty set

959a181

flake8

2b12102

rythorpe added 2 commits August 16, 2021 16:32

allow passing drive name to add_connection() src_gids arg

e5e2a9e

whats_new.rst

bc48a8b

rythorpe changed the title ~~WIP: unify cell connectivity and drive connectivity~~ [MRG] unify cell connectivity and drive connectivity Aug 16, 2021

jasmainak commented Aug 17, 2021

View reviewed changes

hnn_core/tests/test_network.py Outdated Show resolved Hide resolved

jasmainak changed the title ~~[MRG] unify cell connectivity and drive connectivity~~ [MRG+1] unify cell connectivity and drive connectivity Aug 17, 2021

rythorpe and others added 2 commits August 17, 2021 12:56

whats_new comments

3663826

Update hnn_core/tests/test_network.py

22f5906

Co-authored-by: Mainak Jas <[email protected]>

rythorpe mentioned this pull request Aug 17, 2021

refine net.connectivity and pick_connection #415

Open

3 tasks

ntolley approved these changes Aug 18, 2021

View reviewed changes

ntolley merged commit 6fb132d into jonescompneurolab:master Aug 18, 2021

ntolley mentioned this pull request Aug 20, 2021

[MRG] Add probability arguments to drives API #416

Merged

jasmainak mentioned this pull request Mar 20, 2023

remove legacy drive conventions and functions #393

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+1] unify cell connectivity and drive connectivity #369

[MRG+1] unify cell connectivity and drive connectivity #369

jasmainak commented Jun 18, 2021 •

edited

Loading

rythorpe commented Jul 7, 2021

jasmainak commented Jul 7, 2021

jasmainak commented Jul 20, 2021

rythorpe commented Jul 23, 2021

rythorpe commented Jul 29, 2021

jasmainak commented Jul 29, 2021

cjayb commented Jul 30, 2021

rythorpe commented Aug 13, 2021

ntolley commented Aug 13, 2021 •

edited

Loading

jasmainak commented Aug 13, 2021 via email

rythorpe commented Aug 13, 2021

jasmainak Aug 16, 2021

rythorpe Aug 16, 2021

jasmainak Aug 16, 2021

rythorpe Aug 16, 2021

jasmainak Aug 16, 2021

jasmainak commented Aug 16, 2021 •

edited

Loading

jasmainak commented Aug 16, 2021

rythorpe commented Aug 16, 2021

codecov-commenter commented Aug 16, 2021 •

edited

Loading

jasmainak commented Aug 16, 2021

rythorpe commented Aug 16, 2021

jasmainak commented Aug 17, 2021

jasmainak Aug 17, 2021 •

edited

Loading

rythorpe Aug 17, 2021

jasmainak Aug 17, 2021

jasmainak Aug 17, 2021

rythorpe Aug 17, 2021

jasmainak Aug 17, 2021

jasmainak commented Aug 17, 2021

ntolley commented Aug 18, 2021

		@@ -40,6 +40,8 @@ Changelog

		- Add :func:`~hnn_core.Network.pick_connection` to query the indices of specific connections in :attr:`~hnn_core.Network.connectivity`, by `Nick Tolley`_ in `#367 <https://github.com/jonescompneurolab/hnn-core/pull/367>`_

		- Unify the codepath for instantiating drive and local network connectivity, by `Ryan Thorpe`_, `Mainak Jas`_, and `Nick Tolley`_ in `#369 <https://github.com/jonescompneurolab/hnn-core/pull/369>`_

[MRG+1] unify cell connectivity and drive connectivity #369

[MRG+1] unify cell connectivity and drive connectivity #369

Conversation

jasmainak commented Jun 18, 2021 • edited Loading

rythorpe commented Jul 7, 2021

jasmainak commented Jul 7, 2021

jasmainak commented Jul 20, 2021

rythorpe commented Jul 23, 2021

rythorpe commented Jul 29, 2021

jasmainak commented Jul 29, 2021

cjayb commented Jul 30, 2021

rythorpe commented Aug 13, 2021

ntolley commented Aug 13, 2021 • edited Loading

jasmainak commented Aug 13, 2021 via email

rythorpe commented Aug 13, 2021

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

jasmainak commented Aug 16, 2021 • edited Loading

jasmainak commented Aug 16, 2021

rythorpe commented Aug 16, 2021

codecov-commenter commented Aug 16, 2021 • edited Loading

Codecov Report

jasmainak commented Aug 16, 2021

rythorpe commented Aug 16, 2021

jasmainak commented Aug 17, 2021

jasmainak Aug 17, 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

jasmainak commented Aug 17, 2021

ntolley commented Aug 18, 2021

jasmainak commented Jun 18, 2021 •

edited

Loading

ntolley commented Aug 13, 2021 •

edited

Loading

jasmainak commented Aug 16, 2021 •

edited

Loading

codecov-commenter commented Aug 16, 2021 •

edited

Loading

jasmainak Aug 17, 2021 •

edited

Loading