BFS layout implementation

[matthieugouel]

Hi 👋 !

First, thank you for you work, networkx is simply an amazing package!

I wanted draw a DiGraph by using a BFS algorithm. I did not find a way to do it with the actual layouts of networkx (sorry if it's already possible!) so I quickly implemented one myself.

This is the kind of output it produces:

I would understand if you disagree to merge this PR because of the specificity of the layout but I wanted to share it in case it helps someone.

Best,
Matthieu.

[rossbar]

This seems like a nice idea to me; I see the use-case for visualizing the order in which nodes are visited during BFS, and I think the layered approach makes sense.

The one thing that jumps out to me is that there is already a multipartite_layout function that has this concept of positioning nodes in layers, so I think it'd be good to explore if that code could be resued. I haven't thought about this too hard, but I'm thinking something along the lines of (roughly):

def bfs_layout(G, start, **kwargs):
    G, center = _process_params(G, center, 2)
    H = G.copy()  # So original graph remains unmodified
    
    # Compute layers with BFS
    visited = set()
    queue = deque([(start, 0)])
    while queue:
        current_vertex, current_depth = queue.pop()
        if current_vertex in visited:
            continue
        for nbr in H.neighbors(current_vertex):
            queue.appendleft((nbr, current_depth + 1))
        visited.add(current_vertex)
        H.nodes[current_vertex]["layer"] = current_depth

    # Compute node positions with multipartite_layout
    return nx.multipartite_layout(H, subset_key="layer", **kwargs)

What do you think?

[matthieugouel]

Hi @rossbar !

Thanks for your response. You're right, I didn't know about multipartite layout at the time. What is nice with your implementation is that we can make use of the multipartite features such as align parameter.

[matthieugouel]

Hi,

Is there a blocker that prevents this PR to be merged? Can I do something to help?

Best,
Matthieu.

[dschult]

I resolved the conflicts with this PR and the main branch. Just wanted to warn you that this changed your branch on your fork. So you'll need to pull it down to local before doing further work.

I don't know of any blockers, but clearly we need more reviewer resources/time to address PRs like this and others.

One thing I notice is that this has no tests. Can you add some tests in the drawing/tests/test_layout.py module?

[rossbar]

I took the liberty of pushing up a few test cases for expected behavior with "well-behaved" input graphs. In order to really push this over the hump, I think it'd be good to define what should the behavior be when the graph is disconnected. Currently the function raises an exception from multipartite_layout due to not every node in H having a subset_key. Should we raise an exception for disconnected graphs? Attempt to layout each component?

My feeling is that raising makes the most sense - it's difficult to know what the desired behavior would be when it comes to positioning unreachable chunks of graphs based on their reachability from a starting node! WDYT?

Other than that and some additional non-blocking comments/suggestions, this is nearly ready to go IMO!

[dschult]

This looks good to me -- I like @rossbar's suggestions and tests and I added a few comments below. The main one being that I think we should allow the pos return dict to be smaller than all the nodes in G. The resulting dict can be combined with other pos dicts for the other components, or the drawing function can use G.subgraph(pos) as the graph and pos=pos as the position.

[dschult]

After looking at this again (and more closely) I agree with @rossbar that we should raise an exception when the BFS does not cover all nodes -- that is, when G is a disconnected graph.

I also concur that it would be good to use nx.multipartite_layout and we should also use nx.bfs_layers to get the layers.

Perhaps the error message when disconnected can include something about how they can use just a subgraph of the original graph to get a dict for some of the nodes.
One bit of code could bel:

bfs_layout(G.subgraph(nx.node_connected_component(G, start)))

but that might be too long for an error message -- see below...

[dschult]

I'm going to try committing the suggestions to clean up review

[dschult]

Sorry this is too ugly. fixing style via webpage is not the way to do it.

[dschult]

Thanks @matthieugouel for this submission. I think all discussions have now been addressed. Reviews should follow fairly quickly.

[rossbar]

Thanks @matthieugouel and @dschult for the revisions - handling dicts as subset_key in multipartite_layout is a particularly nice improvement!

I took the liberty of pushing up one final test case for the non-connected graph case, but this LGTM!