Division by zero when rescaling vertically aligned components

[wilkeber]

Hi Paul!
First, thank you for netgraph! I find it really useful to visualize parts of my work and I love the InteractiveGraphs!
Now to the issue I came across. I use netgraph to visualize computational graphs I build and reconfigure, i.e., mostly to check that whatever I configured is connected correctly. The sugiyama layout works sufficiently well for this purpose, but sometimes my graph is split into multiple components. For small sub-graphs, the sugiyama layout vertically aligns the nodes, which creates a problem when rescaling the layout to fit into the bounding box. Basically, I run into a division by zero at the following line:

netgraph/netgraph/_node_layout.py

Line 574 in f8f950c

node_positions /= np.max(node_positions, axis=0)

Here is a minimal reproducible example that triggers this case:

import networkx as nx
import netgraph as ng

G = nx.DiGraph()
G.add_node('A')
G.add_node('B')
G.add_edge('A', 'B')
edge_list = [edge for edge in G.edges]
node_layout = ng.get_sugiyama_layout(edge_list)

The result is that the node positions contain nan values:

node_layout
{'A': array([nan,  1.]), 'B': array([nan, -0.])}

I understand that you are quite busy, so I'll leave here my suggested fix. For my cases, it seems that simply not normalizing when the nodes are aligned, should do the trick:

np.divide(node_positions, np.max(node_positions, axis=0), where=np.max(node_positions, axis=0) != 0, out=node_positions)

instead of

node_positions /= np.max(node_positions, axis=0)

Best,
Pablo

[paulbrodersen]

Hi Pablo, thanks for raising this issue. Great MWE, even better solution. Do you want to create a pull request with your changes?
Alternatively, if you let me know the email address that you use on github, then I can add you as a co-author to the commit.

[wilkeber]

Hi!
Done, I just created the PR. I created this Github account just so I can inform you of this bug, so this is my first PR for a public project. It is tiny, but I hope it is along the lines of what you expected.

[paulbrodersen]

I created this Github account just so I can inform you of this bug, so this is my first PR for a public project.

Thank you very much.

It is tiny, but I hope it is along the lines of what you expected.

You are doing great!

Typically, small PRs are much more preferable to large PRs, and if you have a large PR, it is usually better to split it into several small PRs. The best outcome for a pull request is if the maintainer can simply accept it, and merge. If the PR is very large, there are bound to be some issues that need to be addressed before it can be merged. If instead you split the large PR into many, self-contained small PRs, most of the changes can readily be merged, and only the problematic bit goes into the discuss/amend/re-submit loop.

[wilkeber]

Yeah, I absolutely agree. Small PRs are so much easier to review in-between other tasks :)

[wilkeber]

Brief question; how often to you release a new netgraph version, i.e. how soon do you think this fix will be available via pip? No pressure, just trying to understand when this fix will be available to colleagues :)

[paulbrodersen]

I have parked the changes on the dev branch at the moment. I haven't tested the current state of dev branch thoroughly yet. It passes all automated tests but I struggle with automating the testing of some of the interactive functionalities, so those still need to be tested manually. I will test & merge the dev branch into master (and push to PyPI) once I am finished with some other changes that I am working on.

In the meantime, your colleagues can install the dev branch via pip in this way:

pip install https://github.com/paulbrodersen/netgraph/archive/dev.zip

[paulbrodersen]

I just merged the dev branch back into master, and updated the packages on PyPI. A simple pip install netgraph should now suffice.