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

[ENH] Fixed bug in clear_connectivity and improved tests for deleting drives #613

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
1 change: 1 addition & 0 deletions examples/howto/plot_connectivity.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
def get_network(probability=1.0):
net = jones_2009_model(add_drives_from_params=True)
net.clear_connectivity()
net.clear_drives()
jasmainak marked this conversation as resolved.
Show resolved Hide resolved

# Pyramidal cell connections
location, receptor = 'distal', 'ampa'
Expand Down
79 changes: 63 additions & 16 deletions hnn_core/network.py
Original file line number Diff line number Diff line change
Expand Up @@ -1253,23 +1253,70 @@ def add_connection(self, src_gids, target_gids, loc, receptor,

self.connectivity.append(deepcopy(conn))

def clear_connectivity(self):
"""Remove all connections defined in Network.connectivity
def _clear_connectivity(self, src_types):
"""Remove connections with src_type in Network.connectivity.

Parameters
----------
src_types : list
Source types of connections to be cleared

"""
connectivity = list()
for conn in self.connectivity:
if conn['src_type'] in self.external_drives.keys():
connectivity.append(conn)
self.connectivity = connectivity

def clear_drives(self):
"""Remove all drives defined in Network.connectivity"""
connectivity = list()
for conn in self.connectivity:
if conn['src_type'] not in self.external_drives.keys():
connectivity.append(conn)
self.external_drives = dict()
self.connectivity = connectivity
_validate_type(src_types, list, 'src_types', 'list, drives, local')
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be updated? I think it should say something like this for the last arg: 'list of drive names or local network connections'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can remove this check as types are already validated in the public functions - clear_connectivity() and clear_drives(). Should I remove this check or update it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, go ahead and remove it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure will do

# Finding connection indices to be deleted
conn_idxs = list()
for src_type in src_types:
conn_idxs.extend(pick_connection(self, src_gids=src_type))

# Deleting the indices
for conn_idx in sorted(conn_idxs, reverse=True):
del self.connectivity[conn_idx]

def clear_connectivity(self, src_types='all'):
"""Clear connections between cells of the local network
Comment on lines +1275 to +1276
Copy link
Collaborator

Choose a reason for hiding this comment

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

@rythorpe is this what you intended? A "dangling" cell population will be problematic even with local network connectivity ... I'm wondering if we should make clear_connectivity private altogether until it has clarified in our minds how to do it right

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep! I don't see how this could be problematic. If you think we should prevent users from selectively removing local network connections, perhaps we can just get rid of the src_types arg and make this function clear all local network connections (which I guess is more consistent with the name of this method anyway).

Copy link
Collaborator

Choose a reason for hiding this comment

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

but clear_drives needs the src_types argument ...

Copy link
Contributor

Choose a reason for hiding this comment

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

but clear_drives needs the src_types argument ...

why?


Parameters
----------
src_types : list | 'all'
Source types of connections to be cleared
'all' - Clear all connections between cells of the local network

"""
if src_types == "all":
src_types = list((src_type for src_type in self.gid_ranges.keys()
if src_type not in self.drive_names))
_validate_type(src_types, list, 'src_types', 'list')
drive_names = self.drive_names
for src_type in src_types:
if src_type in drive_names:
raise ValueError('src_types contains %s which is an external '
'drive.' % (src_type,))
self._clear_connectivity(src_types)

def clear_drives(self, drive_names='all'):
"""Remove drives defined in Network.connectivity.

Parameters
----------
drive_names : list | 'all'
The drive_names to remove
"""
if drive_names == 'all':
drive_names = self.drive_names
_validate_type(drive_names, list, 'drive_names', 'list')
all_drive_names = self.drive_names
for drive_name in drive_names:
if drive_name not in all_drive_names:
raise ValueError('drive_names contains %s which is not an '
'external drive.' % (drive_name,))
for drive_name in drive_names:
del self.external_drives[drive_name]
self.clear_connectivity(src_types=drive_names)

@property
def drive_names(self):
"""Returns a list containing names of all external drives."""
return list(self.external_drives.keys())

def add_electrode_array(self, name, electrode_pos, *, conductivity=0.3,
method='psa', min_distance=0.5):
Expand Down
73 changes: 70 additions & 3 deletions hnn_core/tests/test_network.py
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,28 @@ def test_network_drives_legacy():
n_bursty_sources)


def get_expected_connectivities(net, src_types):
"""Return expected connectivities left after clearing connections.

Parameters
----------
net : The network instance
src_types : list
Connection source types to be cleared

Returns
-------
int
Number of connections left after the deletion
operation
"""
deleted_connectivities = 0
for src_type in src_types:
deleted_connectivities += len(pick_connection(net,
src_gids=src_type))
return len(net.connectivity) - deleted_connectivities


def test_network_connectivity():
"""Test manipulation of local network connectivity."""
params = read_params(params_fname)
Expand Down Expand Up @@ -781,10 +803,55 @@ def test_network_connectivity():
pick_connection(**kwargs)

# 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) == 4 # 2 drives x 2 target cell types
# Test invalid argument type
with pytest.raises(TypeError, match="src_types must be an instance of"):
net.clear_connectivity(src_types=10)

# Test Clearing connections of local src_types

# Using clear_connections to delete a drive
with pytest.raises(ValueError,
match="src_types contains evdist1 which is an "
"external drive."):
net.clear_connectivity(src_types=['evdist1'])

# Deleting all connections with src_type as 'L2_pyramidal'
expected_connectivities = (get_expected_connectivities(
net, src_types=['L2_pyramidal']))
net.clear_connectivity(src_types=['L2_pyramidal'])
assert len(net.connectivity) == expected_connectivities

# Deleting all local connections
src_types = list()
external_drives = net.drive_names
for conn in net.connectivity:
if (conn['src_type'] not in src_types and
conn['src_type'] not in external_drives):
src_types.append(conn['src_type'])
expected_connectivities = (get_expected_connectivities(
net, src_types=src_types))
net.clear_connectivity(src_types='all')
assert len(net.connectivity) == expected_connectivities

# Testing deletion of a custom number of drives

# Using clear_drives to delete a local connection
with pytest.raises(ValueError,
match="drive_names contains L2_pyramidal which "
"is not an external drive."):
net.clear_drives(drive_names=['L2_pyramidal'])

# Deleting one external drive
all_drives = net.drive_names
drives_to_be_deleted = all_drives[0:1]
expected_connectivities = (get_expected_connectivities(
net, src_types=drives_to_be_deleted))
net.clear_drives(drive_names=drives_to_be_deleted)
assert len(net.connectivity) == expected_connectivities

# Deleting all external drives
net.clear_drives()
# All internal and external connections are deleted
assert len(net.connectivity) == 0

with pytest.warns(UserWarning, match='No connections'):
Expand Down