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

raj1701 · 2023-03-08T17:42:43Z

Closes #549
Fixed all bugs in clear_drives and clear_connectivity functions
Wrote tests for the following

clear_connectivity with no arguments passed. Deletes all connections apart from external drive connections
clear_drives with drive_names as argument passed. Deletes connections of the specified drives
clear_drives with no argument. Deletes connections of all external drives

jasmainak · 2023-03-08T19:04:51Z

hnn_core/network.py

-        """
-        connectivity = list()
+    def clear_connectivity(self, src_types=None):
+        """Remove connections with src_type in Network.connectivity."""


can you document src_types ?

also is clear_connectivity tested?

Sure will add the documentation for src_types.
Yes it is tested in all three tests. First test checks the base conditions where no arguments are passed. Here it successfully deletes all cell connections apart from those originating from external drives
The other two tests it tests for deletion of connections from specific source types.

jasmainak · 2023-03-08T19:06:06Z

hnn_core/network.py

+            del self.external_drives[drive_name]
+        self.clear_connectivity(src_types=drive_names)
+
+    def get_external_drive_names(self):


How about?

Suggested change

def get_external_drive_names(self):

@property

def drive_names(self):

It feels more Pythonic.

Sure will make this change

codecov-commenter · 2023-03-08T19:56:46Z

Codecov Report

Merging #613 (8512d57) into master (78d26fa) will decrease coverage by 0.28%.
The diff coverage is 86.98%.

❗ Current head 8512d57 differs from pull request most recent head c7c4a6c. Consider uploading reports for the commit c7c4a6c to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #613      +/-   ##
==========================================
- Coverage   92.15%   91.87%   -0.28%     
==========================================
  Files          22       22              
  Lines        4219     4333     +114     
==========================================
+ Hits         3888     3981      +93     
- Misses        331      352      +21

Impacted Files	Coverage Δ
hnn_core/drives.py	`98.54% <ø> (ø)`
hnn_core/gui/_viz_manager.py	`86.93% <73.07%> (-2.08%)`	⬇️
hnn_core/network.py	`93.19% <91.17%> (-0.16%)`	⬇️
hnn_core/gui/gui.py	`94.87% <93.75%> (-1.57%)`	⬇️
hnn_core/__init__.py	`100.00% <100.00%> (ø)`
hnn_core/dipole.py	`92.59% <100.00%> (+0.13%)`	⬆️
hnn_core/network_models.py	`100.00% <100.00%> (ø)`
hnn_core/optimization.py	`94.85% <100.00%> (+2.10%)`	⬆️
hnn_core/params.py	`91.89% <100.00%> (+0.25%)`	⬆️
hnn_core/viz.py	`89.23% <100.00%> (+0.04%)`	⬆️

... and 1 file with indirect coverage changes

raj1701 · 2023-03-08T19:57:36Z

@jasmainak Is the short description for src_types enough or should I elaborate it more?

jasmainak · 2023-03-08T21:52:58Z

hnn_core/network.py

+            The drive_names to remove
+        """
+        if drive_names == 'all':
+            drive_names = list(self.external_drives.keys())


Suggested change

drive_names = list(self.external_drives.keys())

drive_names = self.drive_names

Sure will do

jasmainak · 2023-03-08T21:53:55Z

examples/howto/plot_connectivity.py

@@ -71,7 +71,7 @@
 # directly.
 def get_network(probability=1.0):
    net = jones_2009_model(add_drives_from_params=True)
-    net.clear_connectivity()
+    net.connectivity = list()


undo this since you have clear_connectivity function

Sure will do

Here net.connectivity needs to be completely cleared.
So both clear_connectivity() and clear_drives() will need to be called one by one.

clear_connectivity() will leave connections of external drives open. For deleting them clear_drives will need to be called.

But if there are existing external drive connections only clear_connectivity needs to be called....

Which direction is better?

jasmainak · 2023-03-08T21:55:06Z

hnn_core/tests/test_network.py

    net.clear_drives()
-    assert len(net.connectivity) == 0
+    assert len(net.connectivity) == 0  # All connections are deleted


clear_drives should still leave the connections within the cortical column ... were there no connections within the cell populations?

Earlier clear_connectivity() was called which deleted all connections other than the ones involving external drives. That was pushed by the previous pull request i think, the one I got merge conflict with so I kept that change

hnn_core/network.py

rythorpe · 2023-03-10T18:46:36Z

I'm guessing you merged master with your clear_drive branch? We want to maintain a clean commit history, so rebasing instead of merging is the way to go here. You could probably do an interactive rebase to clean up your history. Any recommendations @jasmainak?

raj1701 · 2023-03-10T19:11:30Z

Yes I merged master in clear_drive because of which I got external commits
For correcting :
I reset the HEAD to the last correct commit. Made the changes and commited again. Now this branch has 4 commits and only the changes made by me. Is this fine @rythorpe or should I do something else?

jasmainak · 2023-03-10T20:23:14Z

you almost never want to do a merge or pull (fetch + merge) when working on public repos. Just fetch the latest master and rebase:

$ git fetch upstream master:master
$ git rebase master

raj1701 · 2023-03-10T20:47:27Z

Yes will remember this in future. Thanks!!

raj1701 · 2023-03-16T12:21:11Z

Hey @rythorpe @jasmainak what changes need to be made in this?

jasmainak · 2023-03-19T22:31:22Z

hnn_core/network.py

+        src_types : list | None
+            Source types of connections to be removed
+            None - Remove all connections other than those which have an
+            external drive as a source type


@raj1701 I think you have been struggling with the logic because you are trying to preserve the old behavior and logic without fully understanding the purpose of that behavior. You need to clarify the behavior before writing the logic. To me, what makes the most sense is:

net.clear_connectivity() # clear all connections net.clear_connectivity(src_types='drives') # clear connections from drives to cells net.clear_connectivity(src_types='local') # clear connections within cells net.clear_connectivity(src_types=['L2_Pyramidal', 'L5_basket']) # clear connection starting with cell populations L2_pyramidal and L5_basket

To do this, you should use pick_connection and avoid rewriting the logic of finding the appropriate connections. Once you have the indices of the connections, you can write a simple for loop to delete the items:

for conn_idx in conn_idxs: del net.connectivity[conn_idx]

Yes @jasmainak I was trying to preserve the old logic. If no argument is passed to clear_connectivity then the default value of src_types should be called "All" instead of "None". Will refactor the code accordingly and make the required changes.

raj1701 · 2023-03-21T18:55:30Z

Hey @jasmainak I redesigned the code. It passed all the tests in my local system but here the tests are failing for a file I didn't even change...

rythorpe · 2023-03-22T20:17:06Z

Hey @jasmainak I redesigned the code. It passed all the tests in my local system but here the tests are failing for a file I didn't even change...

Looks like an MPI error... I'm going to rerun the tests because sometimes running MPIBackend on the CI servers is finicky.

jasmainak · 2023-03-23T07:23:01Z

hnn_core/network.py

+            src_types = list()
+            for conn in self.connectivity:
+                if conn['src_type'] not in src_types:
+                    src_types.append(conn['src_type'])


this seems pretty convoluted ... could you use self.gid_ranges.keys() ?

jasmainak · 2023-03-23T07:23:59Z

hnn_core/network.py

+            src_types = list()
+            external_drives = self.drive_names
+            for conn in self.connectivity:
+                if (conn['src_type'] not in src_types and
+                   conn['src_type'] not in external_drives):
+                    src_types.append(conn['src_type'])


same here ... I think you just need to do a list comprehension to leave out the external drives from all the cell populations

jasmainak · 2023-03-23T07:28:10Z

hnn_core/tests/test_network.py

+
+    Parameters
+    ----------
+    net - The network instance


we follow the numpy docstring conventions: https://numpydoc.readthedocs.io/en/latest/format.html#parameters

Hey @jasmainak I have simplified the loops and followed the numpy docstring conventions

jasmainak · 2023-03-24T00:35:47Z

hnn_core/network.py

+            src_types = list()
+            for src_type in self.gid_ranges.keys():
+                src_types.append(src_type)


Suggested change

src_types = list()

for src_type in self.gid_ranges.keys():

src_types.append(src_type)

src_types = list(self.gid_ranges.keys())

the constructor of list can take an iterable

jasmainak · 2023-03-24T00:36:50Z

hnn_core/network.py

+            src_types = list()
+            all_src_types = self.gid_ranges.keys()
+            local_src_types = all_src_types - self.drive_names
+            for src_type in local_src_types:
+                src_types.append(src_type)


Suggested change

src_types = list()

all_src_types = self.gid_ranges.keys()

local_src_types = all_src_types - self.drive_names

for src_type in local_src_types:

src_types.append(src_type)

src_types = [src_type for src_type in list(self.gid_ranges.keys()) if src_type not in self.drive_names]

jasmainak · 2023-03-24T00:38:00Z

hnn_core/network.py

+
+        Parameters
+        ----------
+        src_types : list | all | drives | local


raj1701 · 2023-03-24T08:25:03Z

@jasmainak I made these changes. Sorry for the extra commit. I pushed by mistake before changing one thing.

raj1701 · 2023-04-27T09:22:55Z

Hey @jasmainak can you suggest any changes required for this PR?

rythorpe · 2023-04-27T21:03:31Z

hnn_core/network.py

+            'drives' - Clear connections originating from external drives
+            'local' - Clear connections within cells


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.

Sure will rename instances of drives as exogenous and instances of local as endogenous

@jasmainak @ntolley wdyt?

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

That's fair!

Sure will rename accordingly

jasmainak

Looks like I never posted the comments from my review ... here goes

examples/howto/plot_connectivity.py

jasmainak · 2023-05-04T22:52:34Z

hnn_core/network.py

+            'drives' - Clear connections originating from external drives
+            'local' - Clear connections within cells


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

jasmainak · 2023-05-04T22:54:48Z

hnn_core/tests/test_network.py

@@ -510,6 +510,30 @@ def test_network_drives_legacy():
                                                 n_bursty_sources)


+def get_expected_connectivities(net, src_types='all'):


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

Sure will do

rythorpe · 2023-05-09T15:32:12Z

hnn_core/network.py

@@ -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'):


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

@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?

yes, let's move this to test_network.py

rythorpe · 2023-05-09T15:32:23Z

hnn_core/network.py

-    def clear_connectivity(self):
-        """Remove all connections defined in Network.connectivity
+    def _get_expected_connectivities(self, src_types='all'):
+        """Return expected connectivities left after clearng connections.


Suggested change

"""Return expected connectivities left after clearng connections.

"""Return expected connectivities left after clearing connections.

rythorpe

It looks like there are two ways to modify drive connectivity: net.clear_connectivity(src_types='internal') and net.clear_drives(drive_names='all'). The first method, however, doesn't clear net.external_drives, which could be misleading to users. I'm not sure why we need two different ways to clear drive connections, so my recommendation would be to consolidate.

Zen of Python: There should be one-- and preferably only one --obvious way to do it.

rythorpe · 2023-05-09T18:19:29Z

Just to clarify, I understand the utility of having net.clear_drives call net.clear_connectivity: the first 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.

raj1701 · 2023-05-09T19:29:09Z

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.

jasmainak · 2023-05-15T01:44:36Z

hnn_core/network.py

-        self.external_drives = dict()
-        self.connectivity = connectivity
+        if src_types == 'all':
+            return 0


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).

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.

jasmainak · 2023-05-15T01:45:53Z

hnn_core/network.py

+                                          src_gids=src_type))
+        return len(self.connectivity) - deleted_connectivities
+
+    def clear_connectivity(self, src_types="all"):


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.

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

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

Sure will use single quotes

jasmainak · 2023-05-16T22:16:04Z

hnn_core/network.py

+    def clear_connectivity(self, src_types='all'):
+        """Clear connections between cells of the local network


@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

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).

but clear_drives needs the src_types argument ...

but clear_drives needs the src_types argument ...

why?

rythorpe · 2023-05-16T22:38:07Z

hnn_core/network.py

-                connectivity.append(conn)
-        self.external_drives = dict()
-        self.connectivity = connectivity
+        _validate_type(src_types, list, 'src_types', 'list, drives, local')


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'.

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?

Good point, go ahead and remove it!

Sure will do

Fixed bud and improved tests for deleting drives

270eca7

raj1701 changed the title ~~Fixed bud and improved tests for deleting drives~~ [ENH] Fixed bug in clear_connectivity and improved tests for deleting drives Mar 8, 2023

jasmainak reviewed Mar 8, 2023

View reviewed changes

rythorpe mentioned this pull request Mar 8, 2023

[ENH] Updated clear_drives function and added tests #612

Closed

Added parameter description and refactored code

2e64179

jasmainak reviewed Mar 8, 2023

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

Minor changes

894d386

Added source_types as None description

28ebd50

raj1701 force-pushed the clear_drive branch from 5bc3792 to 28ebd50 Compare March 10, 2023 19:08

jasmainak reviewed Mar 19, 2023

View reviewed changes

Revamped logic

a12afca

jasmainak reviewed Mar 23, 2023

View reviewed changes

Simplified loops

1bf4d51

jasmainak reviewed Mar 24, 2023

View reviewed changes

raj1701 added 2 commits March 24, 2023 08:21

simplified code

9b53e13

Refactoring

446d6f9

rythorpe reviewed Apr 27, 2023

View reviewed changes

jasmainak reviewed May 4, 2023

View reviewed changes

Made function private and improved documentation

c7c4a6c

rythorpe reviewed May 9, 2023

View reviewed changes

jasmainak reviewed May 15, 2023

View reviewed changes

Separated clear_drives and clear_connectivity functionality

e7d289f

jasmainak reviewed May 16, 2023

View reviewed changes

rythorpe reviewed May 16, 2023

View reviewed changes

	def get_external_drive_names(self):
	@property
	def drive_names(self):

	drive_names = list(self.external_drives.keys())
	drive_names = self.drive_names

	src_types : list \| all \| drives \| local
	src_types : list \| 'all' \| 'drives' \| 'local'

		'drives' - Clear connections originating from external drives
		'local' - Clear connections within cells

		@@ -510,6 +510,30 @@ def test_network_drives_legacy():
		n_bursty_sources)


		def get_expected_connectivities(net, src_types='all'):

	"""Return expected connectivities left after clearng connections.
	"""Return expected connectivities left after clearing connections.

		def clear_connectivity(self, src_types='all'):
		"""Clear connections between cells of the local network

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

Are you sure you want to change the base?

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

Conversation

raj1701 commented Mar 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raj1701 Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 8, 2023 • edited Loading

Codecov Report

raj1701 commented Mar 8, 2023 • 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

raj1701 Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

jasmainak Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rythorpe commented Mar 10, 2023

raj1701 commented Mar 10, 2023 • edited Loading

jasmainak commented Mar 10, 2023

raj1701 commented Mar 10, 2023

raj1701 commented Mar 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raj1701 commented Mar 21, 2023

rythorpe commented Mar 22, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raj1701 commented Mar 24, 2023 • edited Loading

raj1701 commented Apr 27, 2023

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 left a 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

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

rythorpe left a comment

Choose a reason for hiding this comment

rythorpe commented May 9, 2023 • edited Loading

raj1701 commented May 9, 2023 • edited Loading

Choose a reason for hiding this comment

raj1701 May 15, 2023 • 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

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

raj1701 Mar 8, 2023 •

edited

Loading

codecov-commenter commented Mar 8, 2023 •

edited

Loading

raj1701 commented Mar 8, 2023 •

edited

Loading

raj1701 Mar 9, 2023 •

edited

Loading

jasmainak Mar 8, 2023 •

edited

Loading

raj1701 commented Mar 10, 2023 •

edited

Loading

raj1701 commented Mar 24, 2023 •

edited

Loading

rythorpe commented May 9, 2023 •

edited

Loading

raj1701 commented May 9, 2023 •

edited

Loading

raj1701 May 15, 2023 •

edited

Loading