Layered layout drawing algorithm

[Adrien-Luxey]

I recently proposed to add a new layered_layout algorithm to NetworkX's display toolkit, in discussion #4710.

This is still work in progress, but working to the point where I'd like NetworkX community's reviews.

I want to focus on solving glitches for some edge cases (edges drawn awkward when they could be straight, too big spacing between groups of nodes...). If someone has time, notably to fix the display of long edges (see below), I'd be glad :)

• Is anyone knowledgeable on FancyArrowPatch? Using path parameter hides the arrow for long edges, probably because FancyArrowPatch discards shrinkA and shrinkB parameters when path is given. See ny_pylab.py:794.
• import numpy as np appeared 17 times in layout.py. I made it a global import in this file. -> reverted
• Why keep the dim parameter when most layouts don't support dim != 2? A lot of layout functions do that.
• Tests still need to be written.

Hail NetworkX! :D

[dschult]

The import numpy statements need to be inside the functions so they don't get run until the function is called. Until we figure out how to do lazy importing (which we are close to figuring out) in a good way, this is our solution.

The dim parameter may not be needed in many routines, but it provides a standard API for implementing the layouts. I haven't looked at this in a long long time, but it didn't seem that out of line when I last looked at it. Certainly that change should not happen in this PR. We can do that in another PR if needed.

I'll leave comments about FancyArrowPatch and tests to others. :}
Thank for this!!

[Adrien-Luxey]

Sorry my PR was messy. I was in a hurry yesterday.

About the dim parameter: the bipartite_layout and multipartite_layout functions do not take the parameter, just like layered_layout. It is true that all other layout functions do take dim.

I'll now bootstrap the tests.

[Adrien-Luxey]

For testing, I was thinking about calling the layered_layout for different graph configurations.

tests = [
    {
        "edges": [(1, 2)],
        "align": "vertical",
        "expected_pos": {1: np.array([0., 1.]), 2: np.array([ 0., -1.])},
        "expected_edges_path": {}
    },
    {
        "edges": [(1, 2), (1, 3)],
        "align": "vertical",
        "expected_pos": {1: np.array([-0.5,  1. ]), 3: np.array([-0.5, -0.5]), 2: np.array([ 1. , -0.5])},
        "expected_edges_path": {}
    },
]

Called with:

G = DiGraph()
G.add_edges_from(tests[i]['edges'])
pos, edges_path = nx.layered_layout(G, align=tests[i]['align'])
# incorrect, just for the example: needs a function for deep comparison
assert pos == tests[i]['expected_pos'] 
assert edges_path == tests[i]['expected_edges_path'] 

I see no other function does this though. Would my test process be alright, still?

[Adrien-Luxey]

I'm happy with the code, now! Even big graphs seem free of glitches (at the cost of more time complexity).

Last things to do before we can call it a day:

• Correctly display arrows for long edges (help wanted).
• Put all of my documentation references in the docstring.

We could do more (layer width minimization, non-DAGs, MultiDiGraphs, spine edges...), but the result is already good looking as-is, and should serve most purposes. So: future works!

[Adrien-Luxey]

I added references, cleaned up the doc, built it. It looks ok. Also added a gallery example.

Now all that's missing is the arrow head for long edges (see example). I tried, but couldn't figure out FancyArrowPatch's magic.

[dschult]

I took a quick look at the code -- nothing close...
It is hard to track what is changing. Can you move your rescale_layout and rescale_layout_dict back to the position in the file that they used to be so git's diff can show what has changed more effectively? You could leave the previous order in __all__ too. Generally, anything you can do to make it easy to see what is new, what is change old code and what is unchanged old code would be helpful for review. :}
Thanks for this!

Strange that the arrowheads show up fine for some edges and not others. I expected them to appear but be mispositioned. But it looks to me like they aren't even there for some edges.

[Adrien-Luxey]

Sorry for the unnecessary modifications. I'm not experimented in FOSS contributions. I made the changes you mention for myself; e.g. I felt it was clearer to have utils/ubiquitous functions at the file's top, but didn't revert before shipping, sorry.

Regarding the drawing of long edges: the FancyArrowPatch object is created differently depending on the edge being "long" or not. Long edges have to follow the path described in edges_path. FancyArrowPatch seems to follow a very different process when called with a path and not src & dst.
It's documented in MPL: "Alternatively if path is provided, an arrow is drawn along this path and patchA, patchB, shrinkA, and shrinkB are ignored."
... But there is no code example to help understand how to achieve similar output.

Thanks for your review!

[rossbar]

There's a lot here so it will take time to get a full picture of how all the pieces fit together, but from what I can gather, it seems like the edge path specification can be decoupled from the process of determining the node positions - though like I said there's a lot here so I may just be missing that part.

I am still of the opinion that it would be much better to have any new layout conform to the same pattern as the others (i.e. only computing node positions, with a relatively consistent signature and output) and to keep the edge drawing and node position determination entirely separate.

So my question is: is it possible to formulate this layout in such a way that determining the node positions can be performed without considering edges at all? I understand that doing so would be suboptimal for performance(in #4710 you mention that this would require computing things multiple times) , visual appeal, etc. but is the coupling of edge paths and node positions intrinsic to this layout (not the drawing)? If so, then this represents a significant break not only from the API, but also the layout semantics in NX.

[Adrien-Luxey]

is the coupling of edge paths and node positions intrinsic to this layout (not the drawing)?

Yes indeed. It is an intrinsic objective of Sugiyama's method (the one I used) to minimize edge crossings. Towards this goal, in the algorithm, each non-unit edge is separated into a path spanning several virtual "dummy" edges, before node positioning is performed.

is it possible to formulate this layout in such a way that determining the node positions can be performed without considering edges at all?

We can skip edge positioning altogether and let draw_networkx_edges perform its usual straight lines. It should work well for small graphs, but (when time allows) I'll show you an example graph with and without edge positioning, you will see the added value. Still, the answer is: yes, we can make us of the layered_layout without drawing non-straight edges.

---

We could call another function returning only edge positioning. It would call the exact same code as layered_layout, which is deterministic. We would still need to call draw_networkx_edges with a parameter for edges placement (edges_path). We would still need to address the bug I presented in matplotlib issue #20157.