[MRG] Add warnings for invalid connections #419

ntolley · 2021-08-26T16:29:54Z

This PR adds checks inside net.add_connection to ensure the valid loc, receptor, target_type combinations are specified. Depending on the definition of the cell, certain combinations will break during simulate_dipole. For example, pyramidal cells do not have ampa synapses so if the user accidentally defines one with net.add_connection, then the simulation will break.

While writing this I am realizing there are some issues with how things are organized internally. At the moment we support proximal, distal, and soma as valid location arguments. However, soma is handled a bit strangely since it is not defined in the sect_loc attribute but still passes as a valid location because it has defined synapses:

hnn-core/hnn_core/cells_default.py

Line 289 in 4fde7c6

sect_loc = {'proximal': ['apical_oblique', 'basal_2', 'basal_3'],

This brings up an important question, should we allow targeting of locations with more precision? For example loc='apical_1'. It would be fairly easy to allow, and may make adding to network_models.py easier with that sort of flexibility.

ntolley · 2021-08-26T16:39:21Z

Also @rythorpe adding these warnings makes the legacy mode drives API to fail. Specifically, L5_basket cells don't have any synapses defined for loc='distal. Previously this was just ignored silently, such that when adding a distal drive these cells get zero weight in net._attachdrive()

hnn-core/hnn_core/cells_default.py

Line 190 in 4fde7c6

sect_loc = dict(proximal=['soma'], distal=[])

hnn-core/hnn_core/network.py

Line 741 in 4fde7c6

weights_by_type.update({target_type: {'ampa': 0.}})

Any thoughts on a workaround?

ntolley · 2021-08-26T16:42:15Z

hnn_core/network.py

@@ -11,6 +11,7 @@
 from copy import deepcopy

 import numpy as np
+from numpy.testing._private.utils import raises


VSCode is getting too big for its britches, this was added automatically 😬

I'll remove it later.

github autopilot? :)

rythorpe · 2021-08-26T17:10:10Z

hnn-core/hnn_core/network.py

Line 741 in 4fde7c6

weights_by_type.update({target_type: {'ampa': 0.}})

Any thoughts on a workaround?

Is the problem that legacy mode doesn't currently add an NMDA weight for a non-specified target cell type? If so, you could just copy and paste network.py L741 except with 'nmda' instead.

jasmainak · 2021-08-27T04:42:28Z

hnn_core/network.py

+            valid_loc.extend(item)
+
+        valid_loc = list(set(valid_loc))
+        # Separation ecessary to identify valid receptors


Suggested change

# Separation ecessary to identify valid receptors

# Separation necessary to identify valid receptors

jasmainak · 2021-08-27T04:45:03Z

hnn_core/network.py

+        valid_loc = list(set(valid_loc))
+        # Separation ecessary to identify valid receptors
+        if loc in self.cell_types[target_type].sect_loc and \
+                self.cell_types[target_type].sect_loc[loc]:


can you be more explicit with the second condition? what exactly is it checking?

jasmainak · 2021-08-27T04:45:46Z

hnn_core/network.py

        conn['loc'] = loc

-        valid_receptor = ['ampa', 'nmda', 'gabaa', 'gabab']
-        _check_option('receptor', receptor, valid_receptor)
+        valid_receptors = set(np.concatenate([


Suggested change

valid_receptors = set(np.concatenate([

valid_receptors = set(sum([

does this work?

jasmainak · 2021-08-27T04:49:23Z

should some of the checks go to check.py or does it remain in the same file?

jasmainak · 2021-08-27T04:53:23Z

This brings up an important question, should we allow targeting of locations with more precision?

Is there a real use case? Otherwise, I'd suggest just documenting it better. E.g., by saying: "loc must be either in cell.sect_loc or 'soma'" Let's not open a can of worms if not necessary

jasmainak · 2021-12-09T20:29:05Z

@ntolley this PR seems pretty easy to bring to MRG. Could be a nice one to tackle to get back into the groove?

ntolley · 2022-07-18T11:08:40Z

@ntolley this PR seems pretty easy to bring to MRG. Could be a nice one to tackle to get back into the groove?

While it may have taken me some time to "get back into the grove" 😄 , it may be for the better that I came back to this with fresh eyes.

@jasmainak @rythorpe I've implemented a fairly clean system to allow connections to be added through net.add_connection that allows loc to be the normal targets like 'proximal' or 'distal', but now specific sections like 'apical_tuft'. It also automatically checks if the synapses exist on that section (the point of this PR).

Since everything goes through net.add_connection, it will also be possible to add drives that target specific sections now too (and maybe the tonic bias?). I think this is a really good step towards making the code more generalizable, as now the "grouped sections" like proximal and distal aren't hard-coded and instead derived from the Cell object.

Let me know what you think so far. If you like the idea of targeting specific sections with drives, I'll go ahead and remove the hard coded checks for proximal and distal.

jasmainak · 2022-07-18T14:14:55Z

Can you have "proximal" or "distal" defined across cell types instead of just a group of sections in one cell? Will your code be able to handle that?

@chenghuzi this a PR you might want to review :)

ntolley · 2022-07-18T14:21:32Z

Can you elaborate on what you are thinking? With the way the API for drives is designed, as well as the structure of the Cell class, I'm not sure it's clear to me how that would work.

When you define a proximal/distal drive, the weights and delays for each cell need to be specified, so there's no way to avoid thinking in terms of the specific cells that compose the proximal/distal drive.

jasmainak · 2022-07-18T14:26:09Z

Isn't it so that currently the distal drive doesn't target L5 basket cells? I suppose the logic for that would have to be outside add_connection then ...

ntolley · 2022-07-18T14:40:45Z

Ah I understand now. So that logic is handled here:

hnn-core/hnn_core/drives.py

Line 41 in b66742d

raise ValueError('When adding a distal drive, synaptic weight cannot '

and here:

hnn-core/hnn_core/cells_default.py

Line 339 in b66742d

sect_loc = dict(proximal=['soma'], distal=[])

I think I see why specifying proximal/distal at the cell level could be useful to break away these hard coded dependencies. Perhaps this is getting at @rythorpe's idea of a "constraints" file.

Perhaps the larger issue is simply moving the hard-coded specifications from drives.py into a more generalized format.

jasmainak · 2022-07-18T15:00:19Z

Maybe one could iterate over the cell types in net.cell_types looking for "proximal"/"distal" sections ? But if you intend to have connection weights that are cell-type-specific, it's probably better to have the logic outside add_connection anyway

codecov-commenter · 2022-07-18T20:14:35Z

Codecov Report

Merging #419 (aea5553) into master (f5d1691) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #419      +/-   ##
==========================================
+ Coverage   89.93%   90.03%   +0.10%     
==========================================
  Files          20       20              
  Lines        3883     3893      +10     
==========================================
+ Hits         3492     3505      +13     
+ Misses        391      388       -3

Impacted Files	Coverage Δ
hnn_core/network.py	`92.16% <100.00%> (+0.19%)`	⬆️
hnn_core/network_builder.py	`94.37% <100.00%> (ø)`
hnn_core/network_models.py	`100.00% <100.00%> (ø)`
hnn_core/parallel_backends.py	`83.14% <0.00%> (+0.82%)`	⬆️

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 f5d1691...aea5553. Read the comment docs.

rythorpe · 2022-07-19T19:57:29Z

I like this a lot actually @ntolley. I'm definitely in favor of moving away from hardcoded definitions for "proximal" and "distal". These properties are and should be network-specific. Perhaps we could start (probably in a separate PR) by adding a new attribute to Network called Network.connectivity_constraints of type dict that takes the form

Network.connectivity_constraints = {
    'exogenous_drive_targets': {
        'proximal': ['L5_pyr_soma', 'L2_pyr_soma', 'L5_bask_soma', 'L5_bask_soma'],
        'distal': [...]
        },
    'local_net_conn_restrictions': {
        'L5_bask': ['L2_bask', 'L2_pyr'],
        'L2_bask': [...],
        ...
    }

By default, Network.connectivity_constraints would be left empty (e.g., {'exogenous_drive_targets': None, 'local_net_conn_restrictions': None}. The different network models specified in network_models.py would then set this attribute when instantiating a specific network configuration.

rythorpe · 2022-07-19T20:50:28Z

hnn_core/network.py

+            Target location of synapses. Must be an element of
+            `Cell.sect_loc` such as 'proximal' or 'distal', which defines a
+            group of sections, or an existing section such as 'soma' or
+            'apical_tuft' (defined in `Cell.sections` for all targeted cells)


+1 for this. Eventually, it'd be nice to allow users to specify which cell sections apply to a given sect_loc. I can imagine a few different network changes I'd like to implement for my own project where the location (i.e., 'proximal' or 'distal') isn't necessarily intuitive.

jasmainak · 2022-07-20T14:29:41Z

hnn_core/network_models.py

@@ -11,7 +11,8 @@
 from .externals.mne import _validate_type


-def jones_2009_model(params=None, add_drives_from_params=False):
+def jones_2009_model(params=None, add_drives_from_params=False,
+                     legacy_mode=True):


why did you add this here? We need to set ourselves free of the legacy ...

I promise it's actually towards that goal! legacy_mode=True by default in the Network. But the old interface for jones_2009_model() didn't have the option to turn it on or off.

The reason I added it is because legacy_mode needs to be False to add a drive that targets a specific section. This is because legacy_mode=True creates a connection for all cells, even if they aren't specified in the weights dict (they just have a weight of zero). Therefore if you try to target 'apical_trunk' for example, it'll throw an error because it tries to create a connection to the basket cells when they don't have a section named 'apical_trunk'.

Would a good fix be to just make that clearer in the docstring?

If you plan to deprecate it in the future, the default should be False and it should raise a deprecation warning if someone sets it to True.

It sounds like the examples are secretly using legacy_mode=True ? I think the "right" thing to do is to start a separate PR that fixes this problem, then rebase this PR after that is merged.

If we set default to False here, then we'll need to explicitly set legacy_mode=True for all of our examples + tests that use this network model in order to maintain consistency. Either that or we just rip the bandaid off and change the outcome of our examples + tests now.

Btw, I noticed that the new GUI currently loads the "ghost" drives ... probably because of the implicit legacy_mode=True

Ugh. Legacy mode is going to be the death of us.

@jasmainak that's exactly what's happening. Considering this change doesn't require changes to the current code, can we deal with legacy_mode in a separate PR?

…ections

ntolley · 2022-07-25T13:35:37Z

Just rebased this PR. Everything is good on my end if you guys are happy with the changes!

jasmainak · 2022-07-31T22:59:14Z

Thanks @ntolley ! 🥳 🥳 🥳

ntolley commented Aug 26, 2021

View reviewed changes

jasmainak reviewed Aug 27, 2021

View reviewed changes

ntolley force-pushed the connectivity_warnings branch from 41f2e7b to 82cf21c Compare February 12, 2022 19:37

ntolley force-pushed the connectivity_warnings branch from 82cf21c to 9db8532 Compare July 18, 2022 10:54

ntolley changed the title ~~WIP: Add warnings for invalid connections~~ [MRG] Add warnings for invalid connections Jul 18, 2022

ntolley changed the title ~~[MRG] Add warnings for invalid connections~~ WIP: Add warnings for invalid connections Jul 18, 2022

ntolley mentioned this pull request Jul 18, 2022

follow-up GUI improvements #497

Closed

8 tasks

ntolley force-pushed the connectivity_warnings branch from 9db8532 to f34157b Compare July 18, 2022 20:13

ntolley changed the title ~~WIP: Add warnings for invalid connections~~ [MRG] Add warnings for invalid connections Jul 18, 2022

ntolley force-pushed the connectivity_warnings branch from b675784 to 1248e83 Compare July 18, 2022 20:19

rythorpe reviewed Jul 19, 2022

View reviewed changes

rythorpe approved these changes Jul 19, 2022

View reviewed changes

jasmainak reviewed Jul 20, 2022

View reviewed changes

ntolley added 2 commits July 25, 2022 15:30

Start system for warning invalid connections

56090fc

Revert "Start system for warning invalid connections"

8180d0c

ntolley added 6 commits July 25, 2022 15:30

Enable targetting specific connections, add warnings for invalid conn…

b9ab7cb

…ections

Remove hard coded section locations in network_builder

fe9dbd4

Allow drives to target individual sections

724c4b1

Update whats_new.rst

476d097

Garbage collector ;)

f7e744a

Update doc to mention legacy_mode

e1007ce

ntolley force-pushed the connectivity_warnings branch from 0a34276 to e1007ce Compare July 25, 2022 13:30

Fix tests

aea5553

jasmainak merged commit d1f57b2 into jonescompneurolab:master Jul 31, 2022

ntolley mentioned this pull request Jan 25, 2023

Clean up connectivity API and add tests #293

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Add warnings for invalid connections #419

[MRG] Add warnings for invalid connections #419

ntolley commented Aug 26, 2021

ntolley commented Aug 26, 2021

ntolley Aug 26, 2021

jasmainak Aug 27, 2021

rythorpe commented Aug 26, 2021

jasmainak Aug 27, 2021

jasmainak Aug 27, 2021

jasmainak Aug 27, 2021

jasmainak commented Aug 27, 2021

jasmainak commented Aug 27, 2021

jasmainak commented Dec 9, 2021

ntolley commented Jul 18, 2022 •

edited

jasmainak commented Jul 18, 2022 •

edited

ntolley commented Jul 18, 2022 •

edited

jasmainak commented Jul 18, 2022

ntolley commented Jul 18, 2022

jasmainak commented Jul 18, 2022 •

edited

codecov-commenter commented Jul 18, 2022 •

edited

rythorpe commented Jul 19, 2022

rythorpe Jul 19, 2022

jasmainak Jul 20, 2022

ntolley Jul 20, 2022 •

edited

ntolley Jul 20, 2022

jasmainak Jul 20, 2022

jasmainak Jul 20, 2022 •

edited

rythorpe Jul 20, 2022

jasmainak Jul 21, 2022

rythorpe Jul 22, 2022

ntolley Jul 25, 2022

ntolley commented Jul 25, 2022

jasmainak commented Jul 31, 2022

	# Separation ecessary to identify valid receptors
	# Separation necessary to identify valid receptors

	valid_receptors = set(np.concatenate([
	valid_receptors = set(sum([

[MRG] Add warnings for invalid connections #419

[MRG] Add warnings for invalid connections #419

Conversation

ntolley commented Aug 26, 2021

ntolley commented Aug 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rythorpe commented Aug 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmainak commented Aug 27, 2021

jasmainak commented Aug 27, 2021

jasmainak commented Dec 9, 2021

ntolley commented Jul 18, 2022 • edited

jasmainak commented Jul 18, 2022 • edited

ntolley commented Jul 18, 2022 • edited

jasmainak commented Jul 18, 2022

ntolley commented Jul 18, 2022

jasmainak commented Jul 18, 2022 • edited

codecov-commenter commented Jul 18, 2022 • edited

Codecov Report

rythorpe commented Jul 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntolley Jul 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmainak Jul 20, 2022 • edited

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 commented Jul 25, 2022

jasmainak commented Jul 31, 2022

ntolley commented Jul 18, 2022 •

edited

jasmainak commented Jul 18, 2022 •

edited

ntolley commented Jul 18, 2022 •

edited

jasmainak commented Jul 18, 2022 •

edited

codecov-commenter commented Jul 18, 2022 •

edited

ntolley Jul 20, 2022 •

edited

jasmainak Jul 20, 2022 •

edited