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

Arf layout #5910

Merged
merged 10 commits into from
Aug 23, 2022
Merged

Arf layout #5910

merged 10 commits into from
Aug 23, 2022

Conversation

cvanelteren
Copy link
Contributor

@cvanelteren cvanelteren commented Aug 2, 2022

Implements arf layout (Geipel 2006) which provides improvements over the traditional spring layout.
Short description

The attractive and repulsive forces (arf) layout (Geipel
2006) improves  the spring layout in  three ways. First,
it prevents congestion of  highly connected nodes due to
strong forcing  between nodes.  Second, it  utilizes the
layout space  more effectively by preventing  large gaps
that  spring layout  tends  to create.  Lastly, the  arf
layout represents symmmetries in  the layout better than
the default spring layout.

Example:

Plotting code
import networkx as nx, numpy as np, matplotlib.pyplot as plt
from functools import partial
graphs = [nx.krackhardt_kite_graph(),
          nx.florentine_families_graph(),
          nx.grid_graph((5,5)),
          nx.erdos_renyi_graph(100, 0.02, seed = 0)
          ]
fig, ax = plt.subplots(ncols = 2, nrows = 2, share = 0)

layouts = [partial(nx.arf_layout, alpha = 10, b = 5, seed = 0),
           partial(nx.spring_layout, seed = 0)]
centers = [np.array([-1.5, 0]), np.array([1.5, 0])]
for axi, g in zip(ax, graphs):
    for layout, center in zip(layouts, centers):
        pos = layout(g)
        pos = nx.rescale_layout_dict(pos)
        pos = {x: y + center for x, y in pos.items()}
        nx.draw(g, pos = pos,
                ax = axi, node_size = 10)
    axi.annotate("Spring", xy = [0.75, 1.02], xycoords = axi.transAxes,
        va = "center", ha = "center")
    axi.annotate("Arf", xy = [0.25, 1.02], xycoords = axi.transAxes,
        va = "center", ha = "center")
    axi.axis("equal")
fig.show()

image

@cvanelteren
Copy link
Contributor Author

Small remark. Tests seem to be failing on test not relating to this PR. Local pytests pass for the changed files.

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.

I hope these comments are helpful. I went through the code this time. And I found the numpy indexing and multiplying out arrays using newaxis to be somewhat difficult to grok... But hopefully you can help me some.

The tests don't contain any test that the values produced are specific values.
Do you have any cases where you know what the values should be?

networkx/drawing/layout.py Outdated Show resolved Hide resolved
networkx/drawing/layout.py Outdated Show resolved Hide resolved
networkx/drawing/layout.py Show resolved Hide resolved
networkx/drawing/layout.py Outdated Show resolved Hide resolved
networkx/drawing/layout.py Outdated Show resolved Hide resolved
networkx/drawing/layout.py Outdated Show resolved Hide resolved
networkx/drawing/layout.py Outdated Show resolved Hide resolved
networkx/drawing/tests/test_layout.py Outdated Show resolved Hide resolved
networkx/drawing/layout.py Outdated Show resolved Hide resolved
@cvanelteren
Copy link
Contributor Author

I hope these comments are helpful. I went through the code this time. And I found the numpy indexing and multiplying out arrays using newaxis to be somewhat difficult to grok... But hopefully you can help me some.

The comments were helpful. I incorporated many changes and fixed and or added where necessary. I shared your feeling about numpy's broadcast system. It allows for neatly writing complex operations while obfuscating the operations. Using np.einsum could be an alternative but I sometimes find that equally difficult to understand properly. Anyway, the math checks out for this PR, but it could provide an interesting discussion point for future PRs (broadcasting vs einsum).

The tests don't contain any test that the values produced are specific values. Do you have any cases where you know what the values should be?

I can't recall that the paper provides bounds or proofs of convergence. I believe the author argued that there are no objective markers for what makes a good layout and as such I can't provide an absolute test case other than a known heuristic that converges. The paper uses a few measures to determine the "success" of different test graphs. Are you looking for a particular case which has a known solution? One trivial example may be an empty graph that is approximated by a circle. Something like

example
G = nx.empty_graph(10)
pos = nx.arf_layout(G, max_iter = 10000)
nx.draw(G, pos)

image

@dschult
Copy link
Member

dschult commented Aug 21, 2022

I was thinking more simple than the circle and hub layout obtained from an empty graph (which is nice for testing repulsion but not for attraction). I was thinking more like 3 nodes, 1 edge. Or even 2 nodes, 1 edge.
I'm guessing one could construct a steady state of the equations in this kind of special case by hand. Then using those positions as initial conditions, you could test that the results are indeed a solution (though not the only one).

But I'm leaning toward worrying about testing of all the spring-based layouts in another PR. This introduces new layout methods to the same level of testing that we are providing for the other layouts.

It looks like the only test that is failing now is the isort linter which wants numpy to be imported after warnings (because it is not a builtin library) and with a blank line between the builtins and non-builtins as well as after the non-builtins. This is all based on the "Details" link of the failing style test.

@cvanelteren
Copy link
Contributor Author

Should be ok now. I would also be in favor of having a separate test suite part as a PR that would test the currently present force layouts. If you want I can have a poke at it and draft up the PR for testing the force layouts along the lines we discussed here and in #5311.

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.

That would be great if you could put together a PR to take on the task of testing the force layouts.
Thanks!

@jarrodmillman jarrodmillman added this to the networkx-3.0 milestone Aug 23, 2022
@jarrodmillman jarrodmillman merged commit 88245f6 into networkx:main Aug 23, 2022
MridulS pushed a commit to MridulS/networkx that referenced this pull request Oct 12, 2022
* added arf_layout

* reference to docstring and comparison to spring layout

* rebase to origin main

* black re-format

* Left aligned docstring text

* Cleaned up computation and update variables to new docstring

* Updated naming tests. Added input check on arf_layout parameter `a`

* Fixed Linter issues for py38 target

* Fixed Linter issues for target p38

* linter issue fixed
MridulS pushed a commit to MridulS/networkx that referenced this pull request Feb 4, 2023
* added arf_layout

* reference to docstring and comparison to spring layout

* rebase to origin main

* black re-format

* Left aligned docstring text

* Cleaned up computation and update variables to new docstring

* Updated naming tests. Added input check on arf_layout parameter `a`

* Fixed Linter issues for py38 target

* Fixed Linter issues for target p38

* linter issue fixed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants