Fixing DOT format for to_agraph()

[navyagarwal]

Fixes: #6049

First, I replicated the issue discussed in #6049 and found that it still persists.

The output of the out.dot file is given below which is in the format pos="(0,0)"

As discussed in the issue, I made changes to the to_agraph() method so that the DOT format is pos="0,0". Now the code executes without any warnings as expected.

The contents of the file out.dot are also as expected.

[navyagarwal]

@dschult @MridulS Requesting review

[dschult]

This makes to_agraph create strings of two floats instead of a tuple of two floats.
But shouldn't this need to be changed in other places too? Like near line 316 and 319?

Are there other places in the nx_agraph.py module where we need to make the change from tuples to strings with 2 doubles and an exclamation point?

Thanks for tracking this down and fixing it!

[navyagarwal]

@dschult

If I make similar changes in the pygraphviz_layout method (lines 316 and 319), the following test cases of the test_agraph.py file will also have to be modified -

1. test_pygraphviz_layout_root
2. test_2d_layout
3. test_3d_layout

Plus, I have my doubts about why we need to make these changes to the pygraphviz_layout method in the first place.

In case of to_agraph, the format need to be corrected because we were converting the Networkx graph to a Pygraphviz graph which required the values of the pos attribute to be in a certain format.

Whereas for the pygraphviz_layout method, the output is a dictionary of node positions and I see no reason for these positions to be in a certain format. There is no other method in the file that is calling pygraphviz_layout and utilizing the output dictionary that would require the DOT format.

Plus, when I try out the following python script, the output in the out.dot file is very wrong if the node positions are changed to string of floats in pygraphviz_layout -

import networkx as nx

G = nx.petersen_graph()
pos = nx.nx_agraph.graphviz_layout(G)

# Add node positions to graph
for n in G.nodes:
    G.nodes[n]['pos'] = pos[n]

print(list(G.nodes.data()))

AG = nx.nx_agraph.to_agraph(G)

AG.write("dot.out")

>>> [(0, {'pos': '72.809,121.78!'}), (1, {'pos': '92.95,181.47!'}), (2, {'pos': '188.46,131.91!'}), (3, {'pos': '178.6,44.268!'}), (4, {'pos': '103.37,18.0!'}), (5, {'pos': '139.83,153.34!'}), (6, {'pos': '27.0,119.44!'}), (7, {'pos': '149.97,78.818!'}), (8, {'pos': '104.29,86.808!'}), (9, {'pos': '49.727,43.266!'})]

out.dot file -

This happens because the format is being modified in two different places.

When these changes are not made in the pygraphviz_layout method, the outputs are as expected -

>>> [(0, {'pos': (72.809, 121.78)}), (1, {'pos': (92.95, 181.47)}), (2, {'pos': (188.46, 131.91)}), (3, {'pos': (178.6, 44.268)}), (4, {'pos': (103.37, 18.0)}), (5, {'pos': (139.83, 153.34)}), (6, {'pos': (27.0, 119.44)}), (7, {'pos': (149.97, 78.818)}), (8, {'pos': (104.29, 86.808)}), (9, {'pos': (49.727, 43.266)})]

out.dot file -

Maybe I'm getting things wrong here, but per my understanding as of now, I don't think any changes other than the ones in to_agraph are required.

[dschult]

This fixes the current bug where NetworkX graphs with node attributes for position (computed by nx layout routines) get converted to pygraphviz with a tuple for the 'pos' value. They should convert the 'pos' value to the format which pygraphviz/GraphViz use which is a string of comma separated x,y coordinates followed by '!'.

I approve this PR

[rossbar]

I took a detailed look and I still think there's something fishy with this. The proposed fix does indeed get rid of the warning mentioned in #6049, but note that the node positions are not actually respected by pygraphviz - they are overwritten with the values from the A.layout function. You can verify this by repeating the example for nodes that don't have a pos attribute - you get the same result:

>>> G = nx.Graph()
>>> G.add_node(0, pos=(0, 0))
>>> G.add_node(1, pos=(1, 1))
>>> A = nx.nx_agraph.to_agraph(G)
>>> A.layout()  # No warning on this branch, warning on main
>>> print(A)  # Note that the node positions below are *not* what was specified (even accounting for scaling)
strict graph "" {
        graph [bb="0,0,126,96"];
        node [label="\N"];
        0       [height=0.5,
                pos="27,18",
                width=0.75];
        1       [height=0.5,
                pos="99,78",
                width=0.75];
}

# Same example, without node positions
>>> H = nx.Graph()
>>> H.add_nodes_from([0, 1])  # no node positions
>>> AH = nx.nx_agraph.to_agraph(H)
>>> AH.layout()
>>> print(AH)  # Note the node positions are the same as the above example
strict graph "" {
        graph [bb="0,0,126,96"];
        node [label="\N"];
        0       [height=0.5,
                pos="27,18",
                width=0.75];
        1       [height=0.5,
                pos="99,78",
                width=0.75];
}

IMO the thing that really needs to be figured out to resolve this issue is: how do you compute a layout for a graph with networkx and draw the result with pygraphviz. We have many examples of the opposite (i.e. using pygraphviz for layout and nx for drawing) in the gallery, but AFAIK no working examples of the former.

FWIW I caught this by trying to design a test for this case during review!

[navyagarwal]

@rossbar I looked into these examples, plus the one you suggested here. My understanding is that the positions output by the layout() method are consistent with the input positions in the sense that they reflect the relative positions of the nodes as specified in the input positions. However, they differ in the absolute position because of factors like size of the nodes, the number of nodes, layout algorithm used etc.

I draw this conclusion because when I plot the graphs, they follow the intended layout.

>>> G = nx.Graph()
>>> for n in range(3):
...     G.add_node(n, pos=(n, n))
>>> AG = nx.nx_agraph.to_agraph(G)
>>> print(AG)
>>> AG.layout()
>>> AG.draw("outG.png")
>>> print(AG)

strict graph "" {
	0	[pos="0,0!"];
	1	[pos="1,1!"];
	2	[pos="2,2!"];
}

strict graph "" {
	graph [bb="0,0,198,180"];
	node [label="\N"];
	0	[height=0.5,
		pos="27,18",
		width=0.75];
	1	[height=0.5,
		pos="99,90",
		width=0.75];
	2	[height=0.5,
		pos="171,162",
		width=0.75];
}

From the position values given by layout(), we can also see that '1' and '2' are at distances (72,72) and (144, 144) respectively from '0'. So, they are actually following the relative layout we want.

In the examples over here, the fact that the layout positions are following the relative layout is not apparent because there are only 2 nodes in the graph.

[rossbar]

Indeed - thanks for the follow-up @navyagarwal ! I see this now - I must've been on the wrong branch when I first tested the co-linearity example.

Either way I think this exercise highlights the importance of including a test for this behavior. The easiest way to design such a test is usually to write something that fails on main but passes on this branch. In this case, that might include:

1. Verifying that the warning is no longer raised, and
2. verifying that the (relative) position specified by the node attributes is preserved

[navyagarwal]

@rossbar

1. Verifying that the warning is no longer raised

I have added a test case to check that no warnings are raised for this branch, and this test is failing on the main branch as expected.

2. verifying that the (relative) position specified by the node attributes is preserved

I am a bit confused about this one. Do I have to check that all absolute positions generated by layout() follow the intended relative layout?

Plus, I'm not sure what's going wrong with the failed checks -

Error: Internal server error occurred while resolving "actions/checkout@v3". Internal server error occurred while resolving "actions/setup-python@v4"

[rossbar]

Do I have to check that all absolute positions generated by layout() follow the intended relative layout?

It's not required, but it would make it a better test. The main motivation for doing so is that it would check that the pygraphviz layout function is actually respecting the positions rather than overriding them, though arguably that test belongs in pygraphviz itself!

This seems good to me as-is, we can always improve it later. Thanks @navyagarwal

PS the CI failure is unrelated to your changes!