-
Notifications
You must be signed in to change notification settings - Fork 22
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
Default label rotation upwards #67
Conversation
@@ -274,10 +274,19 @@ function Makie.plot!(gp::GraphPlot) | |||
if $(gp.elabels_rotation) isa Real | |||
# fix rotation to a single angle | |||
rot = $(gp.elabels_rotation) | |||
elseif $(gp.elabels_rotation) == automatic | |||
#point the labels up | |||
rot = broadcast($to_angle, edge_paths[], $positions, gp.elabels_shift[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Later in development, I've started using the getattr
function (in utils.jl) to get a attribute for a specific edge/node instead of the broadcasting. This might be worth thinking about. Something like
rotations = zeros(length($positions))
for (i, path, pos)) in zip(1:ne(g), edge_paths[], $positions)
rot = getattr(gp.elabels_rotation, i, automatic)
if rot == automatic
...
elseif rot isa Real
...
end
end
this would allow for elabels_rotation
to be a single float, a vector or a dict possibly mixing fixed values with automatic
. The same way could be used to access the elabels_shift
, elabels_align
and so forth.
Just a thought which came to mind, might not be worth the change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would be a good direction towards a more finely grained interface. I specially like the Dict
for which then the user doesn't need to handle in a whole matrix rather only the edges/vertices of interest. Do you think the user could also supply a Dict
of type Dict{Edge, Any}
or strictly Dict{Integer, Any}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I havn't thought about the Dict{Edge,Any}
option. My gut feeling is that this is probably something to think about in a separate PR. I like it, because the importance of the edge iterator order feels weird. However, the dependence on the order of the edge iterator is basically baked into every part of the recipe...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, the dependence on the order of the edge iterator is basically baked into every part of the recipe
Yes, this is why I got excited with that Dict
compatibility of the getattr
function. Maybe using this function one can turn it around ?
And for the cases where such order is very much hardwired, maybe a wrapper can be built to translate data structures e.g. from (Edge(6 => 10) => :blue )
to [:automatic, :automatic, ..., :automatic, :blue, :automatic, ...]
But yeah, probably a different issue/PR about this would make sense.
rot = broadcast($to_angle, edge_paths[], $positions, gp.elabels_shift[]) | ||
upsidedownlabels = (rot .> π/2) .| (rot .< -π/2) | ||
rot[upsidedownlabels] .+= π | ||
gp.elabels_align[] isa Vector || (gp.elabels_align[] = fill(gp.elabels_align[], ne(gp.graph[]) )) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not entirely happy with this because it might overwrite the user specified align. What do you think about defaulting the align to (:center, automatic)
?
rot == automatic && align[2] == automatic
: add theπ
and chose top/bottom automatic, default behaviourrot == automatic && align[2] != automatic
: do not add theπ
, basicially same behaviour as before the pr, automatic rotation just aligns the text with the edgerot != automatic && align[2] == automatic
: the rotation is specifially given, probably defaultalign[2]
to:bottom
?rot != automatic && align[2] != automatic
: well its all user specified
then it would be neccessary to store those in a new internal observable elabels_align_internal
or something. Otherwise, the automatic
arguments won't be persistent, ie if one for example drags nodes around, everything explicit derived from automatic needs to be derived again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this because it might overwrite the user specified align
The purpose is to exactly to do not overwrite it. Rotating the uspide down labels about π
basicly has as a result to show :top
as :bottom
and :bottom
as :top
(exactly as opposite). That's why later I switch this 2 labels in case it was reverted.
This has as a result to hold the specified align of the user.
E.g.
gc = circular_ladder_graph(5)
labs_align = insert!(fill((:center, :bottom), ne(gc)-1), 12, (:center, :top))
labs_cols = insert!(fill(:black, ne(gc)-1), 12, :red)
graphplot(gc, elabels = repr.(edges(gc)), nlabels=repr.(vertices(gc)), elabels_rotation=nothing, elabels_align=labs_align, elabels_color=labs_cols)
The label of the (Edge 6=>10) is on :top
as specified from the user
graphplot(gc, elabels = repr.(edges(gc)), nlabels=repr.(vertices(gc)), elabels_rotation=Makie.automatic, elabels_align=labs_align, elabels_color=labs_cols)
and stays on :top
as specified by the user, even if the labels are rotated.
So I think overall the user preferences are being preserved. Does something make you think different ?
then it would be neccessary to store those in a new internal observable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About the elabels_opposite
I was thinking about its usability. In the end it is just a specific case of elabels_rotation
. So do you think maybe it can be handled as an input to elabels_rotation
e.g. a symbol of :opposite
?
If an element-wise operation is being followed as you suggested above you can end up having elabels_rotation = (45, automatic, :opposite, ... )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I think about the align is like the anchor of the text node (in the text node coordiante system). In the example above I'd clearly say that the anchor of the Edge 6 => 10
label is south, therefore (:center, :bottom)
, which is the opposite of the behaviour defined by the user.
Regarding the opposite
argument: on master does act mainly on the rotation. After your PR however, I'd say it is more a :top
vs :bottom
thing.
Normally, all edge labels are placed on the left side of the edge (defined from src to dst). I think the opposite
argument should to something like placing the label on the right side of the edge.
Slowly i get the feeling that the current parameters are not really suitable for this problem. Here is a idea, allways on per edge basis:
elabels_rotation
: if float, ignore all other prameters. probably interprete(:center, automatic)
as(:center, :center)
. Label can be moved using theoffset
parameter then. Default:automatic
elabels_side
: either:left
or:right
, defaults to:left
.elabels_anchor
: defaults to(:center, automatic)
. On default, GraphMakie will chose the right combination of angle and:top
or:bottom
to keep the label on the correct side of the edge while turning it in a way, that it is not upside down. By specifing(:center, :bottom)
for example, the text will stay on the specified side of the line but in a way, that the baseline of the text is near the edge. This might lead to upside-down glyphs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I think about the align is like the anchor of the text node (in the text node coordiante system). In the example above I'd clearly say that the anchor of the Edge 6 => 10 label is south, therefore (:center, :bottom), which is the opposite of the behaviour defined by the user.
I tested this on the master branch and the behavior is exactly the same. Maybe the reference system for Edge 6 => 10 is going from 6 to 10 with the left hand and then it's not south but north i.e. :top as specified from the user. As you also mention:
edge labels are placed on the left side of the edge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On master, changing between top and bottom changes the side of the label (from left to right) because the calculated angle stays the same. I think this is not good, because with automatic curved edges and stuff it is a good feature that edge labels are placed on one side per default.
My thinking is that the interface might be more straight forward for the user, if we explicitly specify the side of the label. Then, :top
and :bottom
just changes, whether the bottom of the glyphs is attached to the edge or the top. Defaulting this to automatic
allows the recipe to choose :top or :bottom in way, that the text is upright (while staying on the specified side of the edge).
The rotation of each edge labels would have to be adjusted based on the combination of valign and side of the edge.
This way, if the label is in the way of another edge, the user would have to change elabels_side = Dict(4=>:right)
to move the label to the other side. But the text would still be upright. Which would make the opposite
thing obsolete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey. I could push another commit with the following modifications:
- Use
getattr
for the edge label rotation part. (now behavior will be element-wise). Eg user could input something like [automatic, 45, 90, automatic] - Introduce
elabels_side
to choose side of the edge label. (:left
orright
withleft
beigin the default) - Introduce
elabels_anchor
in place ofelabels_align
- As long as
elabels_anchor
is of:x, automatic
Makie will show edge labels point up - Make
elabels_opposite
obselete
These are breaking changes. Do we want to do the pull request from another branch ?
Future possible work
- Use
getattr
for all the recipe - Use of Dict(Edge, Any)
Let me know if the above are fine or if I missed something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, breaking change would be fine. I think this set of changes would be quite the improvement for the label placement overall. I guess the most breaking/unexpected thing would be the renaming of align to anchor. I'm not entirely sure about that. If done, i think this change should be also done for the node labels before releasing.
Something which isn't explicitly in your list: I'd use getattr
for elabels_side
and elabels_anchor
too.
Do we want to do the pull request from another branch ?
I can try to add you to the repo, then you could work from a branch within. First I need to get the rights for that though.
I am grateful for your input and contribution. Please don't feel obliged to implement more stuff than you originally intended just because I said it'd be a nice idea! And please express clearly if you think this proposed interface isn't good or to far from what you envisioned. I'm not experienced with open source colab at all. Also feel free to contact me on Julia Slack or Zulip for further discussion if you happen to prefer this over Github.
implemented in #73 |
Regarding issue #61 .
Old behavior:
New behavior:
Passing in
elabels_align
will influence the result.Passing in
elabels_opposite
will not influence the result. In my opinion this shouldn't be the case and I would propose to move the relativefor
loop outside theif-else
statement. But this would mean that the "opposite" applies to the rotations handled in and it doesn't give out a particular opposite angle as it is now.