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

WIP: add to_backend_graph and from_backend_graph #7294

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

eriknw
Copy link
Contributor

@eriknw eriknw commented Feb 14, 2024

This is a functional WIP and I think we should discuss and think carefully about this. (yeah, I know, doing so would be easier with docs and tests).

CC @rlratzel who helped me brainstorm the design for this (and I hope will have more constructive feedback!).

I like the idea of exposing converting to/from backends (and even between backends) to users. This is related to #7259. I like having dispatchable versions of conversions so they can be dispatched to backends, which also gives them the option of adding docs and additional parameters.

Once we have this, configuration (#7225), and caching of graph conversions (#7283?), we'll be in good shape for adding more sophisticated policies around converting/dispatching, which will have better UX. If we have all that, then we'll need to think about how to best use should_run (#7257).

@eriknw eriknw marked this pull request as draft February 14, 2024 02:06
@rossbar rossbar added type: Enhancements Dispatching Related to dispatching and backend support labels Feb 14, 2024
@aaronzo
Copy link
Contributor

aaronzo commented Mar 7, 2024

How would this deal with subclasses, eg. DiGraph, MultiGraph without loss of information?

Also, as a backend developer, if I dispatch this function in my backend, could a user override this easily if a new backend popped up and they didn't want to wait for me to write a converter? Upon first reading that seems difficult to do (appreciate it's WIP though)

@eriknw
Copy link
Contributor Author

eriknw commented Mar 7, 2024

Thanks for taking a look and asking great questions!

How would this deal with subclasses, eg. DiGraph, MultiGraph without loss of information?

Do you mean user-subclasses such as class MyGraph(Graph):, or nx.DiGraph? I/we haven't given much thought to the former. The latter should be the primary use case, as I expect backend that have graph classes to either implement all four classes, or raise NotImplementedError or NetworkXNotImplemented for graph types they don't support. Maybe we can use can_run/should_run?

Also, as a backend developer, if I dispatch this function in my backend, could a user override this easily if a new backend popped up and they didn't want to wait for me to write a converter? Upon first reading that seems difficult to do (appreciate it's WIP though)

They can add backend="spam" to use the "spam" backend to convert G (whatever it is) to backend_name backend. The dispatcher may also iterate over prioritized automatic backends.

These functions are definitely nuanced and need careful thought, hence why this is WIP. We have not yet discussed them in the weekly dispatching call on Thursday (https://scientific-python.org/calendars/). I considered not having @nx._dispatchable on these functions, but I think they need to be dispatchable so that (1) backends can add extra keyword arguments, (2) the user can explicitly indicate which backend to use, and (3) to maybe iterate over automatic backends.

This means that it's possible to have an odo-style backend that has no graph classes or algorithms of its own, but can convert between backend graphs, which I think is pretty cool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dispatching Related to dispatching and backend support type: Enhancements
Development

Successfully merging this pull request may close these issues.

None yet

3 participants