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
86 changes: 70 additions & 16 deletions hnn_core/network.py
Original file line number Diff line number Diff line change
Expand Up @@ -1253,23 +1253,77 @@ 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 _get_expected_connectivities(self, src_types='all'):
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is solely for testing purposes, should this function be moved to test_network.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasmainak wanted this function as private because it was not user facing, that is why I shifted it here. But yes it is used only for testing purposes, if required I can move it to test_network.py. @jasmainak @rythorpe wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, let's move this to test_network.py

"""Return expected connectivities left after clearng connections.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Return expected connectivities left after clearng connections.
"""Return expected connectivities left after clearing 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.

Sure


Parameters
----------
src_types : list | all
Connection source types to be cleared

Returns
-------
int
Number of connections left after the deletion
operation
"""
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
if src_types == 'all':
return 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of hard coding this, it might be better to do:

Suggested change
return 0
src_types = self.gid_ranges().keys()

or something similar (please check what it should be).

Copy link
Contributor Author

@raj1701 raj1701 May 15, 2023

Choose a reason for hiding this comment

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

Just to clarify, I understand the utility of having net.clear_drives call net.clear_connectivity: the first is clears net.external_drives and then outsources clearing the drive connectivity to the second. My only issue is that we should encourage users to only use net.clear_drives when touching drive connectivity. The simplest solution would be to make clear_connectivity private, and then make a public wrapper method with the same name that users can call to clear internal network connectivity but doesn't give them access to external drive connectivity.
Alternatively, you could just separate the functionality of the two existing functions s.t. one takes care of drives + external connectivity while the other takes care of internal connectivity.
Apologies if this contradicts some of your previous conversations @raj1701 and @jasmainak - LMK if you disagree.

Can we have a private function _clear_connectivity() to clear connections. A function clear_internal_connectivity() for deleting internal connections with a check to make sure the input src_types do not have an external drive and then just call the private method to clear connections(internal)? Similarly clear_drives() can have a check to ensure src_types only has drive names.

This line of code may not be required if the above behavior is used.

deleted_connectivities = 0
for src_type in src_types:
deleted_connectivities += len(pick_connection(self,
src_gids=src_type))
return len(self.connectivity) - deleted_connectivities

def clear_connectivity(self, src_types="all"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit of a nitpick. But for stylistic consistency, I suggest using single quotes everywhere unless you need to escape certain characters. You can also check what the PEP 8 document says.

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://peps.python.org/pep-0008/#string-quotes

Ok, it makes no recommendation but I personally prefer single quotes :)

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 use single quotes

"""Remove connections with src_type in Network.connectivity.

Parameters
----------
src_types : list | 'all' | 'external' | 'internal'
Source types of connections to be cleared
'all' - Clear all connections (Default)
'external' - Clear connections from the external drives to the
local network
'internal' - Clear connections between cells of the local network

"""
if src_types == "all":
src_types = list(self.gid_ranges.keys())
elif src_types == "external":
src_types = self.drive_names
elif src_types == "internal":
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, drives, local')
# 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_drives(self, drive_names='all'):
"""Remove all 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,))
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
39 changes: 36 additions & 3 deletions hnn_core/tests/test_network.py
Original file line number Diff line number Diff line change
Expand Up @@ -781,10 +781,43 @@ 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

# Deleting all connections with src_type as 'L2_pyramidal'
expected_connectivities = (net._get_expected_connectivities(
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 = (net._get_expected_connectivities(
src_types=src_types))
net.clear_connectivity(src_types='internal')
assert len(net.connectivity) == expected_connectivities

# Testing deletion of a custom number of drives

# Deleting one external drive
all_drives = net.drive_names
drives_to_be_deleted = all_drives[0:1]
expected_connectivities = (net._get_expected_connectivities(
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