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
63 changes: 47 additions & 16 deletions hnn_core/network.py
Original file line number Diff line number Diff line change
Expand Up @@ -1253,23 +1253,54 @@ 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="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' | 'drives' | 'local'
Source types of connections to be cleared
'all' - Clear all connections (Default)
'drives' - Clear connections originating from external drives
'local' - Clear connections within cells
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
'drives' - Clear connections originating from external drives
'local' - Clear connections within cells
'drives' - Clear connections from the external drives to the local network
'local' - Clear connections between cells of the local network

How about 'external' or 'exogenous' instead of 'drives', and 'internal' or 'endogenous' instead of 'local'? I've gotten the impression our documentation is already a little hazy on how we delineate between these two distinct categories of 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 will rename instances of drives as exogenous and instances of local as endogenous

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like 'external' and 'internal' ... very explicit, clear and I prefer it over exogenous/endogenous which is a bit too academic.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fair!

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 rename accordingly


"""
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":
src_types = list(self.gid_ranges.keys())
elif src_types == "drives":
src_types = self.drive_names
elif src_types == "local":
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')
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_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
63 changes: 60 additions & 3 deletions hnn_core/tests/test_network.py
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,30 @@ def test_network_drives_legacy():
n_bursty_sources)


def get_expected_connectivities(net, 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.

just make this a private function ... it's not user facing

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

"""Return expected connectivities left after clearng connections.

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

Returns
-------
int
Number of connections left after the deletion
operation
"""
if src_types == 'all':
return 0
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 +805,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 = (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='local')
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 = (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 local and external connections are deleted
assert len(net.connectivity) == 0

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