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

[raj1701]

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

[codecov-commenter]

Codecov Report

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

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

:mega: 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%)	:arrow_down:
hnn_core/network.py	93.19% <91.17%> (-0.16%)	:arrow_down:
hnn_core/gui/gui.py	94.87% <93.75%> (-1.57%)	:arrow_down:
hnn_core/__init__.py	100.00% <100.00%> (ø)
hnn_core/dipole.py	92.59% <100.00%> (+0.13%)	:arrow_up:
hnn_core/network_models.py	100.00% <100.00%> (ø)
hnn_core/optimization.py	94.85% <100.00%> (+2.10%)	:arrow_up:
hnn_core/params.py	91.89% <100.00%> (+0.25%)	:arrow_up:
hnn_core/viz.py	89.23% <100.00%> (+0.04%)	:arrow_up:

... and 1 file with indirect coverage changes

[raj1701]

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

[rythorpe]

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]

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]

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]

Yes will remember this in future. Thanks!!

[raj1701]

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

[raj1701]

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]

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.

[raj1701]

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

[raj1701]

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

[rythorpe]

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]

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.