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

added traversal algorithm to nx_parallel #91

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

Conversation

RohitP2005
Copy link

I added traversal algorithm with dfs and bfs which is _dispatchable . Kindly suggest further corrections

@RohitP2005
Copy link
Author

@stefanv can u help with what to be done next

@dschult dschult added the type: Enhancement New feature or request label Dec 20, 2024
Copy link
Member

@dschult dschult left a comment

Choose a reason for hiding this comment

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

This is a lot of new functions!
But I can't tell if it is doing anything that networkx is not already doing.
For example the breadth_first_search.py functions look copied straight fro networkx. That code is not parallel. I think the idea is to split up the "embarrasing parallelizable" loop and do the iterations of that loop on different processors.

Maybe just focus on one function and get that working... Then you can do the others.
To get an idea of how this works look at the other functions already in nx-parallel. For example, betweenness_centrality.

@RohitP2005
Copy link
Author

This is a lot of new functions! But I can't tell if it is doing anything that networkx is not already doing. For example the breadth_first_search.py functions look copied straight fro networkx. That code is not parallel. I think the idea is to split up the "embarrasing parallelizable" loop and do the iterations of that loop on different processors.

Maybe just focus on one function and get that working... Then you can do the others. To get an idea of how this works look at the other functions already in nx-parallel. For example, betweenness_centrality.

Sorry about that , i will look into it asap , thanks for the help

@RohitP2005
Copy link
Author

@dschult
Thanks so much for your detailed feedback! You're absolutely right—it’s better to focus on implementing and testing one function thoroughly before moving on to others.

I’ve removed the DFS algorithm for now and added a BFS algorithm that is parallelized for improved performance.

Here’s an example of how to use the new parallel_bfs function:

# Create a sample graph
G = nx.erdos_renyi_graph(100, 0.05)

# Perform parallel BFS
bfs_result = parallel_bfs(G, source=0)
print("BFS Traversal Order:", bfs_result)

Copy link
Member

@Schefflera-Arboricola Schefflera-Arboricola left a comment

Choose a reason for hiding this comment

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

Thanks @RohitP2005 for the PR! Could you please follow this checklist for adding a new algorithm to the project - https://github.com/networkx/nx-parallel/blob/main/CONTRIBUTING.md#general-guidelines-on-adding-a-new-algorithm and you can also look at the existing algorithms in the repository to get a better idea! If any of these points in the checklist are not clear then please let us know. Thanks!

@RohitP2005
Copy link
Author

@Schefflera-Arboricola Yeah sure , i will look into it.
I will make sure the new algorithm added meets your expectations

Copy link
Member

@Schefflera-Arboricola Schefflera-Arboricola left a comment

Choose a reason for hiding this comment

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

Thanks for all the work @RohitP2005 !

I haven't looked at the core implementation in this PR too closely.. but since BFS is not an embarrassingly parallel algorithm, I don't think its parallel implementation could look this simple 😅. If you are new to parallel graph algorithms then I'd suggest you consider adding an embarrassingly parallel algorithm instead (see - #82). BFS for starters might be not be great.

Thanks, and sorry for the delayed reviews :)


@nxp._configure_if_nx_active()
@py_random_state(3)
def parallel_bfs(G, source=None, get_chunks="chunks", n_jobs=None):

Choose a reason for hiding this comment

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

n_jobs is included in the configurations. so no need to include it as a parameter here. (see - https://github.com/networkx/nx-parallel/blob/main/Config.md)


@nxp._configure_if_nx_active()
@py_random_state(3)
def parallel_bfs(G, source=None, get_chunks="chunks", n_jobs=None):

Choose a reason for hiding this comment

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

the names of the functions in the main networkx project and in this nx-parallel project are usually kept same. (see - https://github.com/networkx/nx-parallel/blob/main/CONTRIBUTING.md#general-guidelines-on-adding-a-new-algorithm)

@@ -0,0 +1,2 @@
from .depth_first_search import *

Choose a reason for hiding this comment

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

Suggested change
from .depth_first_search import *

Comment on lines +16 to +19
G : graph
A NetworkX graph.
source : node, optional
Starting node for the BFS traversal. If None, BFS is performed for all nodes.

Choose a reason for hiding this comment

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

you don't need to document the parameters that are already documented in the main networkx repo :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: Enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants