-
Notifications
You must be signed in to change notification settings - Fork 10
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
Create aliases for {a,e}fferent_{edges,nodes}
#264
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #264 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 33 33
Lines 2743 2751 +8
=========================================
+ Hits 2743 2751 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
edges_by_source = efferent_edges | ||
edges_by_target = afferent_edges | ||
target_nodes_by_source = efferent_nodes | ||
source_nodes_by_target = afferent_nodes |
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 find the naming a bit clunky. outgoing_edges
and incoming_edges
would be IMO more intuitive for eff and aff edges.
target_nodes_by_source
also sounds very confusing!
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 find the naming a bit clunky.
outgoing_edges
andincoming_edges
would be IMO more intuitive for eff and aff edges.
Sure, whatever makes sense to everybody is fine by me.
target_nodes_by_source
also sounds very confusing!
What would you suggest? I can always elaborate it with get_target_nodes_of_source_nodes
or whatever. I just want something that people will immediately understand so they don't accidentally choose the incorrect function for their purposes.
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.
What about just target_nodes
and source_nodes
?
They would match the names of the underlying libsonata edges population target_nodes and source_nodes
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.
In libsonata, they expect edge ids as input (as opposed to node ids), right? Wouldn't that be even more confusing?
Oftentimes, the naming of
afferent_edges
/efferent_edges
/afferent_nodes
/efferent_nodes
cause confusion among our users. Also, quite frankly, I always have to look them up too as they don't feel intuitive. It's not clear what they mean and when is it needed to query by source nodes and when by target nodes.To remove the ambiguity without introducing breaking changes, I created aliases for them (naming is open for discussion):