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

Add edge labels #65

Merged
merged 6 commits into from
Jun 19, 2019
Merged

Add edge labels #65

merged 6 commits into from
Jun 19, 2019

Conversation

JackDevine
Copy link
Member

The edgelabels kwarg accepts either a dictionary, a vector or a matrix describing the edge labels.

If you use a vector or matrix, then either missing, false, nothing, NaNor "" values will not be displayed.

The labels are 3/4 of the way along the given edge in the direction of the edge. We could also have the ability to say how far along the edge the labels are.

using LightGraphs
using GraphRecipes
using Plots
g = WheelGraph(5)
edgelabel = Any["$(i)_$(j)" for i in 1:size(adjacency_matrix(g))[1], j in 1:size(adjacency_matrix(g))[2]]
edgelabel[3] = ""
plot(g, names=[1,2,3,4,5], edgelabel=edgelabel)

edge_labels

Copy link
Member

@mkborregaard mkborregaard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice as a first approximation.
I think we'd like the points to follow the line as it curves, but be next to it not on it, preferably so that the text is always on the outside of the curve. I don't know if directed edges always curve counterclockwise (or clockwise) given the current weak support for directed edges, but I think they should. That would make it much more transparent which curve the labels belong to. I think your example also shows that the directed_curve function could take some tweaking to get rounder edges.

Why not make the default to have the labels at 50% rather than at the end? Not sure here.

Does this code also work if the annotations / edgelabels have been wrapped in a Plots.Font object? That's the Plots way of applying fonts, sizes, colors etc to the edgelabels. I don't particularly like that interface, btw, so might be nice to think of alternatives.

src/graphs.jl Outdated
@@ -365,13 +371,35 @@ end
_3d && push!(zseg, z[si], z[di], NaN)
push!(l_wg, wi)
end
if typeof(edgelabel) == Dict
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean if edgelabel isa Dict?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, thanks.

src/graphs.jl Outdated
@@ -365,13 +371,35 @@ end
_3d && push!(zseg, z[si], z[di], NaN)
push!(l_wg, wi)
end
if typeof(edgelabel) == Dict
if edge_label_exsists(edgelabel, (si, di))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo shound be exists

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that is embarassing. I am really bad without a spellchecker.

src/graphs.jl Outdated
edge_label_array = Vector{Tuple}()
if typeof(edgelabel) <: AbstractArray
edgelabel = vec(edgelabel)
(edge_label_array_size = round(Int, sqrt(length(edgelabel))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this in brackets? And what's the thinking exactly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The brackets are a mistake from some sort of copy/paste job. This piece of code makes it so that the user can pass in a matrix and the code will still work. Now that I think of it, I think that we should go the other way round. If the user passes in a matrix, then we don't do any conversion and if they pass in a vector, then we reshape into a matrix. That way I wouldn't have to do any of the confusing LinearIndices business that I go through on lines 389 and 393.

src/graphs.jl Outdated
push!(edge_label_array,
(0.25xsi + 0.75xdi, 0.25ysi + 0.75ydi,
string(edgelabel[LinearIndices((1:edge_label_array_size, 1:edge_label_array_size))[si, di]])))
else # TODO: Make 3d work.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, annotations are tricky in 3d

src/utils.jl Outdated
@@ -119,6 +119,16 @@ function arcdiagram_limits(x, source, destiny)
(xmin-margin, xmax+margin), ylims
end

edge_label_exsists(label_list::Dict, inds::Tuple) = haskey(label_list, inds)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same typo here and below

src/graphs.jl Outdated
if edge_label_exsists(edgelabel, (si, di))
if dim == 2
push!(edge_label_array,
(0.25xsi + 0.75xdi, 0.25ysi + 0.75ydi, string(edgelabel[(si, di)])))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know this code super well, but shouldn't this be xpts and ypts so the labels follow along the curve?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that will make things look a little nicer.

@JackDevine
Copy link
Member Author

JackDevine commented Jun 15, 2019

Thanks for the in depth review!

I think we'd like the points to follow the line as it curves, but be next to it not on it, preferably so that the text is always on the outside of the curve.

Agreed.

I don't know if directed edges always curve counterclockwise (or clockwise) given the current weak support for directed edges, but I think they should.

They curve randomly:

xpt, ypt = if method != :chorddiagram

Deciding on either clockwise or anticlockwise sounds better to me.

Why not make the default to have the labels at 50% rather than at the end? Not sure here.

TikzGraphs and GraphPlots have their labels further along the edge than 50%. Although, maybe we should make that an opt in property.

Does this code also work if the annotations / edgelabels have been wrapped in a Plots.Font object?

No, that does not work right now. I will make that work soon.

@JackDevine JackDevine mentioned this pull request Jun 15, 2019
@JackDevine
Copy link
Member Author

I have gone through all of the points made, although there are some things that I should point out.

  1. I have made it so that edges curve in a given direction, not randomly, see graphs.jl line 369
  2. I still cannot get 3d to work
  3. I have added a new kwarg called edgelabel_offset that allows the user to say how far they want the labels from the edges
  4. Edge labels are now halfway between the source and destiny nodes
  5. I have not made the labels work with Plots.Font. In fact, the kwarg names does not work with Plots.Font either. I have simply made it so that the edgelabels use the kwarg fontsize to get their fontsize, just like the nodelabels do.

@mkborregaard
Copy link
Member

mkborregaard commented Jun 19, 2019

Sounds great 👍 Do you have an example plot?

@JackDevine
Copy link
Member Author

Example plot:

using LightGraphs
using GraphRecipes
using Plots
n = 5
g = WheelGraph(n)
edgelabel = Any["$(i)_$(j)_hello" for i in 1:size(adjacency_matrix(g))[1], j in 1:size(adjacency_matrix(g))[2]]
# edgelabel = Dict()
# for i in 1:n
#     for j in 1:n
#         edgelabel[(i, j)] = string(i, "_", j, "hello_long_name")
#     end
# end
graphplot(g, names=collect(1:n), curvature_scalar=0.1, edgelabel=edgelabel, fontsize=7,
          edgelabel_offset=0.05)

xy

If you uncomment the commented bit, then you get the same result, just that edgelabel is a Dict.

One thing that I forgot in my list above is that the logic of the program is that if the user passes a vector or a matrix, then graphplot converts to a Dict. Although, you probably already worked that out.

Copy link
Member

@mkborregaard mkborregaard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great

@JackDevine
Copy link
Member Author

Thanks for your help @mkborregaard !

I will leave this open for a little longer on the off chance that anyone else wants to comment.

@JackDevine JackDevine merged commit e219601 into JuliaPlots:master Jun 19, 2019
@JackDevine JackDevine mentioned this pull request Jun 29, 2019
@JackDevine JackDevine mentioned this pull request Jan 4, 2020
4 tasks
@ManCornet
Copy link

Hello everyone !

I've recently noticed a strange behaviour when I set the curves argument of the graphplot function to false. In this case, the edgelabel_offset does not work. Is it normal ?

Thank you !

Manon

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

Successfully merging this pull request may close these issues.

3 participants