-
Notifications
You must be signed in to change notification settings - Fork 304
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
Fix BFS Docstring #2318
Fix BFS Docstring #2318
Conversation
pull latest updates
Add MG Hits and MG Neighborhood_sampling to benchmarks (rapidsai#2254)
…rithms Fix add shuffle to capi algorithms
start : Integer or list, optional (default=None) | ||
The id of the graph vertex from which the traversal begins, or | ||
if a list, the vertex from which the traversal begins in each | ||
component of the graph. Passing in multiple start vertices |
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 the current MG bfs call supports only 1 seed vertex per component, would it be better to instead say that the list option is unsupported as of this moment and add a FIXME in the code instead? Stating that certain parameters results in undefined behavior would be better suited for GitHub issues, rather than the user-friendly docs
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.
Same for the SG bfs call. Overall the PR looks good, so I can approve it later today
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.
list is supported so long as no two vertices in the list are in the same component. I can remove the mention of undefined behavior and just state that.
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 just changed it.
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.
Looks good!
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.
Looks good to me
@gpucibot merge |
Corrects the BFS docstring by clarifying that multiple start vertices are accepted as long as they are from different components, and that passing start vertices from the same component will result in undefined behavior.