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

[MRG] ENH: Add function to search for specific connections #367

Merged
merged 22 commits into from Jul 28, 2021

Conversation

ntolley
Copy link
Contributor

@ntolley ntolley commented Jun 17, 2021

This PR turned into a gauntlet of if/else statements so I'd definitely love some input if you guys see a place to simplify. In any case I am quite happy with the functionality and to my eyes everything seems to be working correctly. The API works as follows:

net.pick_connection(src_gids='L2_pyramidal', target_gids=[0, 1, 2])

Will return the index of any connections in net.connectivity that both contain L2 pyramidal cells as src_gids, and [0, 1, 2] in the set of target_gids.

net.pick_connection(receptor=['nmda', 'ampa'])

Will return all connections that have either NMDA or AMPA synapses.

More succinctly, supplying a list to a single parameter corresponds to a logical_or, whereas supplying multiple parameters corresponds to a logical_and across the parameters. While it's wordy to explain, I find this setup to be the most intuitive with what I expect in a search function.

Let me know what you guys think!

@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2021

Codecov Report

Merging #367 (fb99c9c) into master (78ce7cf) will decrease coverage by 0.27%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #367      +/-   ##
==========================================
- Coverage   90.54%   90.26%   -0.28%     
==========================================
  Files          16       17       +1     
  Lines        3013     3072      +59     
==========================================
+ Hits         2728     2773      +45     
- Misses        285      299      +14     
Impacted Files Coverage Δ
hnn_core/network_builder.py 92.63% <ø> (ø)
hnn_core/check.py 100.00% <100.00%> (ø)
hnn_core/network.py 92.14% <100.00%> (+0.52%) ⬆️
hnn_core/viz.py 83.29% <100.00%> (ø)
hnn_core/parallel_backends.py 82.85% <0.00%> (-4.00%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78ce7cf...fb99c9c. Read the comment docs.

@ntolley ntolley mentioned this pull request Jun 17, 2021
15 tasks
@jasmainak
Copy link
Collaborator

More succinctly, supplying a list to a single parameter corresponds to a logical_or, whereas supplying multiple parameters corresponds to a logical_and across the parameters. While it's wordy to explain, I find this setup to be the most intuitive with what I expect in a search function.

I think this makes sense to me!

hnn_core/network.py Outdated Show resolved Hide resolved
hnn_core/network.py Outdated Show resolved Hide resolved
@jasmainak
Copy link
Collaborator

I like this @ntolley !!!

hnn_core/network.py Outdated Show resolved Hide resolved
hnn_core/network.py Outdated Show resolved Hide resolved
@jasmainak
Copy link
Collaborator

@ntolley I recommend changing the title back to [WIP] when you are not actively seeking reviews or waiting for the PR to be merged. These prefixes allow us to filter the PRs that need attention: https://github.com/jonescompneurolab/hnn-core/pulls?q=is%3Apr+is%3Aopen+MRG. Probably not a biggie here but more important in larger projects that you may want to contribute to in the future :)

@ntolley ntolley changed the title [MRG] ENH: Add function to search for specific connections WIP ENH: Add function to search for specific connections Jul 6, 2021
@ntolley ntolley changed the title WIP ENH: Add function to search for specific connections [MRG] ENH: Add function to search for specific connections Jul 11, 2021
@ntolley
Copy link
Contributor Author

ntolley commented Jul 11, 2021

Ready for more reviews here. The major change is the addition of check.py which cleans up a lot of redundant code in both net.add_connection, and net.pick_connection. I'll have to do a sweep to see if there's anything else that can benefit from the existing functions, or can be simplified by moving generic input checking to this file.

hnn_core/network.py Outdated Show resolved Hide resolved
hnn_core/check.py Outdated Show resolved Hide resolved
hnn_core/check.py Outdated Show resolved Hide resolved
src_gids, target_gids = 'L2_basket', 'L2_basket'
loc, receptor = 'soma', 'gabaa'
conn_indices = net_erp.pick_connection(src_gids, target_gids, loc, receptor)
conn_idx = conn_indices[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

much better! now only thing is that you shouldn't have to do this:

Suggested change
conn_idx = conn_indices[0]

so plot_connectivity_matrix accepts a list. Can be a separate PR though. Are there other functions which benefit from pick_connection ?

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 agree that'd be a bit nicer, but what do you think the alternate behavior should be with a list? Returning multiple plots or subplots?

Copy link
Collaborator

Choose a reason for hiding this comment

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

umm ... either is fine. Maybe subplots is better so user is not overwhelmed by plot windows. Each row is one connectivity matrix if multiple subplots. And you can even implement a scrollbar if there are too many subplots

hnn_core/network.py Outdated Show resolved Hide resolved
@jasmainak
Copy link
Collaborator

Just some minor comments. Looks fine otherwise!

Copy link
Collaborator

@jasmainak jasmainak left a comment

Choose a reason for hiding this comment

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

PR is good by me

@rythorpe @cjayb do you guys want to take a look?

@rythorpe
Copy link
Contributor

Sorry for the delay, I'll try to look look through this tomorrow!

@jasmainak jasmainak merged commit 0cf4f05 into jonescompneurolab:master Jul 28, 2021
@jasmainak
Copy link
Collaborator

I went ahead and merged @rythorpe so we can keep the ball rolling. I hope it's okay. Thanks a lot @ntolley for the PR !

@rythorpe
Copy link
Contributor

Sounds good, sorry @ntolley for taking so long! This will help a lot with unifying drive and local network connectivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants