-
Notifications
You must be signed in to change notification settings - Fork 14
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
nx-cugraph: dispatch graph method to gpu or cpu #17
nx-cugraph: dispatch graph method to gpu or cpu #17
Conversation
This is to avoid unnecessary conversions to networkx data structures.
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.
LGTM, I just had some minor comments about the new utility function which need not hold up approval.
@@ -167,3 +167,31 @@ def _default_should_run(*args, **kwargs): | |||
|
|||
def _restore_networkx_dispatched(name): | |||
return getattr(BackendInterface, name) | |||
|
|||
|
|||
def _gpu_cpu_api(nx_class, module_name): |
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.
Just a suggestion, feel free to ignore - I think I'd find these names a bit more self-documenting:
def _gpu_cpu_api(nx_class, module_name): | |
def create_method_dispatcher_factory(nx_class, module_name): |
which then gets called like:
create_method_dispatcher =
create_method_dispatcher_factory(...)
which then gets used like:
a = create_method_dispatcher("a")
...but I can also see how these feel like ctors and could be named with nouns.
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 cycled through a few names for these myself. The current @_gpu_cpu_api
matches the style we already have with @networkx_api
. Naming decorators can be descriptive, so I think they both read perfectly fine. If we were assigning the values to variables and not using decorators, then a factory-like name like your suggestion would probably be more appropriate.
/merge |
This is to avoid unnecessary conversions to networkx data structures.
Same as: rapidsai/cugraph#4749