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

Handle weights as distance= in testing dispatch #6671

Merged
merged 3 commits into from
May 13, 2023

Conversation

eriknw
Copy link
Contributor

@eriknw eriknw commented Apr 29, 2023

This handles ego_graph, which I just implemented in graphblas-algorithms here: python-graphblas/graphblas-algorithms#61

This dispatch function is only used when testing, and it automatically converts graph inputs to the backend specified in an environment variable (it also converts Graph outputs back to networkx graphs). To convert correctly, it needs to know which weight to use for each algorithm. To date, this has worked fairly well by using a small set of rules. As done in this PR, these rules will likely need to be updated as more and more algorithms support dispatch. Some thoughts:

  • Perhaps we don't need to do this; instead, can we rely on every backend to properly handle networkx Graphs as inputs?
  • Or, instead, perhaps we add e.g. weight_argument="distance" to the @dispatch decorator to indicate which argument should be used as the weight argument. Sooner or later, we probably ought to make @dispatch return an instance of a class instead of relying on closures.

CC @jim22k @MridulS

@jim22k
Copy link
Contributor

jim22k commented May 1, 2023

@eriknw This was always a hack, and you're finding the rough edges.

I was really hoping to not force every backend algorithm to check if the input is nx.Graph and convert. However, if we assume that backends have a core implementation and a wrapper implementation that matches the NetworkX API, then the conversion check and handling would live in the wrapper code, keeping the core implementation clean. It would give us the ultimate flexibility to correctly handle whatever algorithm-specific arguments affect the conversion.

Now that NetworkX handles multiple graphs (G, H) in the same call, this only gets more complicated.

I vote to throw away the hacks and pass nx.Graph directly into the backend implementation layer. For a deprecation policy, we can check if the backend has a convert_from_nx method and only call it if it exists. That way backends can "upgrade" by not implementing convert_from_nx and instead handling nx.Graph directly in the wrapper code.

Finally, if we're making this change for convert_from_nx, should we go ahead and eliminate convert_to_nx and make the wrapper layer do the conversion to a NetworkX object if we passed in a NetworkX object? We talked about that during the initial discussions. I was against it then, but it's looking like that might be the better option now. Keeping the conversions fully contained within a single algorithms means that full context is available if a particular algorithm needs one-off logic.

@eriknw
Copy link
Contributor Author

eriknw commented May 2, 2023

convert_to_nx is still very, very handy to help backends pass networkx tests. I don't want to scrap that.

I also don't want to effectively require backends to return a networkx return type (such as networkx.Graph) if the inputs were networkx types.

What do you think about adding e.g. edge_attribute="weight" to @dispatch?

I'm also envisioning a future where we have a top-level configuration for e.g. auto-converting and dispatching, and also per-algorithm configuration that you can manipulate via e.g. pagerank.config[...]. So, I think there may be value in having more information for each algorithm and more conversions (perhaps someday we'd want allow conversions directly between plugins).

These are longer conversations to be had. For now, I think we can continue as done in this PR. Indicating the weight with distance= argument is also used by closeness_centrality and harmonic_centrality.

Copy link
Contributor

@jim22k jim22k left a comment

Choose a reason for hiding this comment

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

I still think we should revisit this so it doesn't become hacks built upon hacks as we build out the dispatching coverage. But I'm okay with this change for now to not lose momentum.

@MridulS MridulS merged commit 7976383 into networkx:main May 13, 2023
@jarrodmillman jarrodmillman added this to the 3.2 milestone Jun 4, 2023
Alex-Markham pushed a commit to Alex-Markham/networkx that referenced this pull request Oct 13, 2023
* Handle weights as `distance=` in testing dispatch

* fix `test_intersection`

This change was suggested here:
python-graphblas/graphblas-algorithms#62 (comment)
dschult pushed a commit to BrunoBaldissera/networkx that referenced this pull request Oct 23, 2023
* Handle weights as `distance=` in testing dispatch

* fix `test_intersection`

This change was suggested here:
python-graphblas/graphblas-algorithms#62 (comment)
cvanelteren pushed a commit to cvanelteren/networkx that referenced this pull request Apr 22, 2024
* Handle weights as `distance=` in testing dispatch

* fix `test_intersection`

This change was suggested here:
python-graphblas/graphblas-algorithms#62 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants