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

Revisit ParallelGraph implementation and maybe add graph object as the attibute #4

Closed
MridulS opened this issue Jul 13, 2023 · 3 comments

Comments

@MridulS
Copy link
Member

MridulS commented Jul 13, 2023

instead of subclassing Graph.

@dschult
Copy link
Member

dschult commented Sep 11, 2023

There are a few options for this:

  • make a shell class that stores the original graph as an attribute and has an attribute __networkx_plugins__.
  • add the __networkx_parallel__ attribute to the original graph to signal that we want to use the "parallel" backend.

These mean different handling by the functions in nx-parallel. The first requires the function would extract the original graph via the attribute. The second would allow treating the input as the original graph (which it is).

The second seems nicer for nx-parallel developers because you can just treat the input object as a graph. But some users might get confused by having one object. We could tell them to set or remove the __networkx_plugins__ attribute to indicate whether to call the backend or not. Or we could provide a function PG = nx_parallel.parallel_graph(G) that adds the attribute.

I'm going to play a little with these two formulations -- and maybe something else will come up. Any thoughts about advantages or disadvantages?

G = nx.path_graph(4000)
nx_result = nx.betweenness_centrality(G)
G.__networkx_plugins__ = "parallel"    # or G = nx_parallel.parallelize(G)
parallel_result = nx.betweenness_centrality(G)

@dschult
Copy link
Member

dschult commented Sep 12, 2023

Looks like the second approach doesn't play nicely with calling networkx functions directly (we already knew that from Kasvish's experience, but I had "recovered my innocense" about that issue -- so re-learned it today.

If we call the parallel version of closeness_vitality, it calls nx.wiener_index(G, ...). So it gets passed G which is now a ParallelGraph, so the backend looks for whether it is implemented in the "parallel" backend. It isn't so an error is raised. But! we can call nx.wiener_index.__wrapped__(G, ...) which doesn't look for the backend. It is the original networkx python function without the _dispatch decorator. But! nx.wiener_index calls nx.is_connected(G) so, since G is a ParallelGraph we get an error unless nx-parallel implements is_connected.

It seems that either gluing a mixin class to the original input graph or manually setting __networkx_plugins__ lead to trouble. Converting the instance G to a version that is requesting the use of "nx-parallel" functions is not an effective approach because it doesn't easily let you call some functions using the ParallelGraph instance while also using the Graph instance in other places. The flexible approach is to actually store two objects. The original graph and a shell object which indicates it is a nx-parallel graph object while still allowing the original graph to be easily accessed.

That's what this PR originally suggested and I'm re-learning why that is a good approach.

@MridulS
Copy link
Member Author

MridulS commented Oct 5, 2023

Done in #9

@MridulS MridulS closed this as completed Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants