-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
MAINT: Renaming functions with a different name while dispatching #56
Conversation
"""The parallel implementation first divides the a list of all permutation (in case | ||
of directed graphs) and combinations (in case of undirected graphs) of `nbunch` | ||
into chunks and then creates a generator to lazily compute the local node | ||
connectivities for each chunk, and then employs joblib's `Parallel` function to | ||
execute these computations in parallel across all available CPU cores. At the end, | ||
the results are aggregated into a single dictionary and returned. | ||
|
||
Note, this function uses the name `approximate_all_pairs_node_connectivity` while |
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.
also, added this to make the user aware that this function uses a different name while dispatching.
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 thought that users would never call this function directly -- that is, they would call the networkx function which in turn would call the appropriate function for them. Do I understand the public facing api correctly?
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 don't think we are that strict about how someone "should" use a backend and a backend is also a package. So, I added this for someone who might want to call nx_parallel's implementation directly for their graph(like if the cost of dispatching through networkx to nx-parallel is too much for their case). But, as mentioned in the note added in README(in this PR) it's not the recommended way. If you think it's too much info to put in the main docs, then let me know, I can remove it and just keep(or maybe change a little) the first note in README.
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.
OK... and I have a question: in the README file just after the paragraph describing this, the examples says:
nx.approximate_all_pairs_node_connectivity(H) # runs the parallel implementation in `approximation/connectivity`
But is this right? nx.approximate_all_pairs_node_connectivity(H)
will raise due to the function not being found. In nx
we call it as nx.approximation.all_pairs_node_connectivity(H)
I was thinking that the dispatcher calls the backend using the function nx_parallel.approximate_all_pairs_node_connectivity(H)
. but the user would still call it after importing networkx as nx.approximation.all_pairs_node_connectivity(H)
.
If I am right, we could allow both in the nx-parallel package (if you want to allow that) by including both routes to the name in the __init__.py
files. But for now just having the longer name is Ok too. So I'm Ok with what is in this PR... But could you check the README.md example for how to call those functions?
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.
Thank you very much for pointing this out!
fixed in #58
@@ -5,18 +5,25 @@ | |||
import nx_parallel as nxp | |||
|
|||
__all__ = [ | |||
"all_pairs_node_connectivity", | |||
"approximate_all_pairs_node_connectivity", |
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.
If you want to allow both ways to call the functions, we could do something like
"approximate_all_pairs_node_connectivity", | |
"approximate_all_pairs_node_connectivity", | |
"all_pairs_node_connectivity", |
and then below (after defining the longer named function) use
all_pairs_node_connectivity = approximate_all_pairs_node_connectivity
But I'm fine with this PR as is. Which do you prefer?
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.
while generating the get_info
function we fetch all the functions in the nx_parallel package and then extract their docstrings... so having 2 all_pairs_node_connectivity
functions in the package(one of them being in connectivity/connectivity
(with docstring) and the other one being in approximation/connectivity
(without docstring)) would probably result in an incorrect mapping between functions and the additional_docs
. So, for now, I'd prefer the current implemented way, although this way does allow the user to use nx-parallel as an independent package, so I'd like to try and find if there's a better way to generate get_info
. But, I think that's for another PR.
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.
Sounds good!
I approve this PR
Reverting the change made in PR #32
The changes made in the above PR would let the user do something like
nxp.tournament.is_strongly_connected(H)
for functions with a differentname
in_dispachable
decorator(ref PR’s first comment for more) but it breaks the way we currently create theget_info
function and how we automatically generate the algorithms list in the README.md. Let me know what you think.