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

Digraph Arrows to fix #2757 #2760

Merged
merged 17 commits into from Dec 17, 2017
Merged

Digraph Arrows to fix #2757 #2760

merged 17 commits into from Dec 17, 2017

Conversation

rodogi
Copy link
Contributor

@rodogi rodogi commented Nov 19, 2017

This pull request targets to replace the thicker lines used to draw arcs (directed edges) used in the nx_pylab.py file with arrow heads.

The main idea is to replace the matplotlib.collections.LineCollection with a list of matplotlib.patches.FancyArrowPatch.

The implementation is a bit slower than the original, but adds some clarity when zooming on a a network, e.g. consider the following the network with N nodes and N*2 edges:

import matplotlib.pyplot as plt
import networkx as nx


G = nx.Digraph()
G = nx.generators.directed.random_uniform_k_out_graph(N, 2)
pos = nx.layout.spring_layout(G)
nx.draw_networkx(G, pos=pos, edge_color='g', arrowstyle='-|>')
plt.show()

For N=100, this normally draws:
regular_100

With this pull request:
arrows_100

For N=1000

Zooming on the network gives:
regular_1000_zoom

With this pull request:
arrows_1000_zoom

I added the keyword arrowstyle to be able to use all the styles for matplotlib.patches.FancyArrowPatch, however, the drawing is chaotic for some of them.

@rodogi rodogi changed the title Arrows to fix #2757 Digraph Arrows to fix #2757 Nov 19, 2017
@hagberg
Copy link
Member

hagberg commented Nov 19, 2017

Thanks, that is a good start. Can you figure out how to get the arrows to scale correctly with node size? For circular nodes this shouldn't be too complicated. For other shapes this will be harder since you probably want to find the closest polygon vertex and route the arrow there.

G = nx.Graph([(1,2)])
nx.draw(G,node_size=10)
nx.draw(G,node_size=1000)

figure_1
fig2

@rodogi
Copy link
Contributor Author

rodogi commented Nov 20, 2017

This solves the problem for marker=='o':

figure_1 5
figure_2 5

For the rest of the markers it gets trickier as we need to calculate the distance from the center of the marker to the intersection point between the edge and the marker. It is doable but it will certainly take me a bit of time.

@hagberg
Copy link
Member

hagberg commented Nov 24, 2017

That looks good. I agree the general marker case is a little tricky.

One thing that I would expect users to want to do is to adjust the arrow sizes. At minimum maybe setting the mutation_scale?

@hagberg hagberg closed this Nov 24, 2017
@hagberg hagberg reopened this Nov 24, 2017
@hagberg
Copy link
Member

hagberg commented Nov 24, 2017

Also see #2163 for arrow colors fixes

@hagberg
Copy link
Member

hagberg commented Nov 24, 2017

And #1199 for alpha-values setting

@rodogi
Copy link
Contributor Author

rodogi commented Dec 5, 2017

Two keyword arguments to define the style and size of the arrow are included (arrowstyle, and arrowsize, respectively). There's the possibility to use an array of numbers for the edge colors and have the respective color for the arrow:

net1

The arrows will never overlap the marker, however for some markers (namely the square, diamond and triangles) the arrow might be clearly distanced for the marker. However, this doesn't seem to affect (too much) the quality of the drawings.

net2

For the rest of the markers, which it works well (polygons with more than 4 sides).
The alphas as a list is not yet included.

@dschult
Copy link
Member

dschult commented Dec 9, 2017

I've been wondering about what should be returned with _edges...
We could return an edge_collection when arrows are not requested
and an (edge_collection, arrow_collection) tuple if arrows are requested.

Another approach would be to make two functions -- one for edges and one for arrows.
Each returns the collection it creates. Each gets its own **kwds. Is there much
overlap between keywords for line and fancyarrow?

@rodogi
Copy link
Contributor Author

rodogi commented Dec 10, 2017

@dschult We could try to use a mpatches.FancyArrow instead of mpatches.FancyArrowPatch to draw the arrows. Patch mpatches.FancyArrow has no bug when used with mcollections.PatchCollections. This way we could maybe return edge_collection when non-directed and arrow_collection when directed?

Making a second function is a good idea too depending on the number of flexibility in terms **kwds for mpatches.FancyArrow intended. If the intention is to leave a couple of arguments to play with the arrow length, width, and shape, maybe only one function is okay?

@dschult
Copy link
Member

dschult commented Dec 10, 2017

I think for now let's try to get the features in with one function and a few arrow-specific arguments. I guess I didn't understand that for the arrows case we don't have a line collection at all... Guess I need to look at the code more. :} It seems like FancyArrowPatch has some features that help relative to FancyArrow, so I'll leave the choice of which to use up to you. And if the bug with patchcollections is likely to be fixed then let's not make the choice depend on that.```

@rodogi
Copy link
Contributor Author

rodogi commented Dec 10, 2017

Yes actually a line collection is returned when the graph is either non-directed or the arrows are disabled. For the case when the graph is directed and arrows are enabled we could use a patch collection for the arrows. The thing is that FancyArrowPatch has been reported to not work with PatchCollection since 2013 See here.

Probably the best idea is to leave a couple of arrow-specific arguments dealing with the head of the arrow and try to keep FancyArrowPatch as it assures the arrow head to be at a correct distance from the node independently from the zoom (which is not the case with FancyArrow).

The remaining question is what to return when using arrows.

@dschult
Copy link
Member

dschult commented Dec 11, 2017

I'm fine with returning None for the time being.
Any container of the patches would be fine with me also.
What do you think?

@rodogi
Copy link
Contributor Author

rodogi commented Dec 11, 2017

Maybe a list of FancyArrowPatch instances adding an example in the documentation of how to manipulate the looking of the arrows using the list?

@dschult
Copy link
Member

dschult commented Dec 13, 2017

Yes -- that is a good idea.
If the example is to change the alpha values of the arrows individually, then I think that would take care of the last unaddressed target for this implementation. Are there any others?

@hagberg
Copy link
Member

hagberg commented Dec 13, 2017

This is a really great contribution.
I suggest calling it good for this PR with maybe the addition of making sure our desire for arrow collections to enable this more efficiently is noted over at the matplotlib site.

@rodogi
Copy link
Contributor Author

rodogi commented Dec 13, 2017

I'm glad to be of some help to such an amazing package, keep the good work.

Should I include a plot example in :examples:drawing: varying for instance alpha values, or just include it in the documentation of draw_networkx_edges/draw_networkx?

@dschult
Copy link
Member

dschult commented Dec 13, 2017

I think the examples folder would be for something bigger while the doc_string works better for a small example. If you can create a 2-3 edge network with different alphas for each arrow, I'm guessing that would be short enough to put in the doc_string. I'd prefer it there. Then as a separate PR we could create an example for the examples folder that highlights the arrows features more generally.

Does this make sense? Is there a better way?

@rodogi
Copy link
Contributor Author

rodogi commented Dec 14, 2017

Yes -- it makes sense. I can eventually make a pull request to add a plot with the arrows in the example directory. I added an example to change the alpha values in the doc-string.

I also added a list in the returns section of the doc-string, it is not clear to me what is the proper syntax used when two different types are returned depending on the use of arrows or not. So I might need to change it, I currently added:

Returns
-------
matplotlib.collection.LineCollection
    `LineCollection` of the edges

list of matplotlib.patches.FancyArrowPatch
    `FancyArrowPatch` instances of the directed edges

Depending whether the drawing includes arrows or not.

Should I change it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants