-
Notifications
You must be signed in to change notification settings - Fork 56
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
[MRG] ENH: Add function to search for specific connections #367
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
04921cb
to
b1082c8
Compare
I think this makes sense to me! |
I like this @ntolley !!! |
@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 :) |
b1082c8
to
06dcb7f
Compare
Ready for more reviews here. The major change is the addition of |
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] |
There was a problem hiding this comment.
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:
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
?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Just some minor comments. Looks fine otherwise! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d072456
to
e6e29d1
Compare
Sorry for the delay, I'll try to look look through this tomorrow! |
Co-authored-by: Mainak Jas <[email protected]>
Co-authored-by: Mainak Jas <[email protected]>
e6e29d1
to
fb99c9c
Compare
Sounds good, sorry @ntolley for taking so long! This will help a lot with unifying drive and local network connectivity. |
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:
Will return the index of any connections in
net.connectivity
that both contain L2 pyramidal cells assrc_gids
, and[0, 1, 2]
in the set oftarget_gids
.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 alogical_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!