Skip to content

Map or set segment_colour and segment_alpha #59

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

Merged
merged 2 commits into from
Nov 22, 2016

Conversation

jiho
Copy link
Contributor

@jiho jiho commented Nov 22, 2016

This was made possible before by pull-request #29 but was then overridden by other commits. These changes are inspired by geom_boxplot and take the best of both worlds: map to colour and alpha of text by default but allow to override with a fixed value.

jiho added 2 commits November 22, 2016 01:45
By default, map them to colour and alpha of the text, as it is done in
geom_boxplot.

Allow to override them.

Allow both spelling of colour but default to the UK one, like the rest of
ggplot2 does.
Map this to the transparency of the text
@slowkow
Copy link
Owner

slowkow commented Nov 22, 2016

ggplot(
    mtcars,
    aes(x = wt, y = mpg, label = rownames(mtcars), colour = factor(cyl))
  ) +
  geom_point() +
  geom_text_repel()

Before this pull request (dark grey line segments):

image

After this pull request (line segment color matches text color):

image

@slowkow
Copy link
Owner

slowkow commented Nov 22, 2016

@jiho Thank you very much for your pull request! I appreciate it.

Can I ask you to please confirm that this is your desired behavior for this pull request? I wonder if you intended to change the border of the label with the alpha option, or if you intended to leave the border unchanged.

ggplot(
    mtcars,
    aes(x = wt, y = mpg, label = rownames(mtcars), colour = factor(cyl))
  ) +
  geom_point() +
  geom_label_repel(alpha = 0.33)

Before this pull request:

It seems that only the background color of the label is affected by the alpha option. Meanwhile, the text color and the segment color are unchanged.

image

After this pull request:

It seems that the segment color matches the text color. Also, the alpha option is now changing 3 things: the segment, the text, and the label background. It seems that the border of the label is unchanged.

image

@jiho
Copy link
Contributor Author

jiho commented Nov 22, 2016

Hum, seems OK for geom_text_repel but not for geom_label_repel. I intended alpha to change everything.

Let me check against geom_label and edit the pull request accordingly (I am not very familiar with geom_label).

@jiho
Copy link
Contributor Author

jiho commented Nov 22, 2016

In geom_label, alpha only controls the background opacity. I don't like it (if I want to hide stuff, I want to hide everything, not just the background) but you probably want to be consistent with ggplot2. I'll update the pull request to be similar to geom_label for now and will submit a PR to ggplot2 to make everything transparent.

@slowkow
Copy link
Owner

slowkow commented Nov 22, 2016

I think I agree with your judgment that alpha should modify the transparency of everything, including text, background color, border color, and segment color.

I'm happy to go ahead and merge your pull request with this new behavior.

It looks like this:

image

@slowkow slowkow merged commit 4424870 into slowkow:master Nov 22, 2016
@jiho
Copy link
Contributor Author

jiho commented Nov 22, 2016

Great, thanks. That looks good indeed!

The inability to change separately the colour of the text and of the outline is a bit bothersome but again, this is the same for geom_label

Thanks for being so reactive (and for ggrepel!)

@aphalo
Copy link
Contributor

aphalo commented Nov 22, 2016

I seem to remember that at some point during development of ggplot 2.0.0 alpha was changed to affect everything but then a decision was made to go back to the original behaviour. The main problem seems to be bar plots and "point" shapes like 21, 22, and 23, for which one usually wants to retain solid borders while making the fill transparent.
Found!
tidyverse/ggplot2#1371
tidyverse/ggplot2#1523

@jiho
Copy link
Contributor Author

jiho commented Nov 22, 2016

Thanks for the precision. While it may make sense for geom_density() I see little reason not to have alpha affect the whole label here. Anyhow, we'll see how Hadley reacts, the change is submitted: tidyverse/ggplot2#1924 (it was pretty easy!)

@slowkow
Copy link
Owner

slowkow commented Nov 22, 2016

@aphalo Thank you for those links! I think I'm comfortable letting alpha control all properties of a label (text, background, border, segment). Perhaps alpha should behave differently for point, bar, or density.

@jiho Just a small note: your pull request changed the appearance of one of my vignette figures. This is OK with me, because I can change the segment color with segment.color = 'grey50'.

Before pull request

image

After pull request

image

Ater pull request, segment.color = 'grey50'

image

@aphalo
Copy link
Contributor

aphalo commented Nov 22, 2016

@slowkow Yes, I agree with you about label. @jiho I was mostly thinking about a PR for ggplot2. It will need good argumentation to have a chance of being accepted. @jiho @slowkow If rejected for ggplot2, could a label_fixed that behaves with respect to alpha exactly as label_repel be added to ggrepel?

@slowkow
Copy link
Owner

slowkow commented Nov 22, 2016

Instead of adding a new function

ggplot(...) + geom_label_fixed()

I would recommend

ggplot(...) + geom_label_repel(..., max.iter = 0)

@aphalo
Copy link
Contributor

aphalo commented Nov 22, 2016

Yes, of course! Thanks!

@jiho
Copy link
Contributor Author

jiho commented Nov 22, 2016

All this makes me think that really there should be separate aesthetics, than can be set or mapped separately, for each plot element: segment.colour, text.colour, label.colour, label.fill, segment.alpha, etc. with some basic inheritance rules (i.e. setting colour sets both text.colour and label.colour, but if both colour and label.colour are set, label.colour takes precedence for the label). What is done here (and in geom_boxplot, etc.) allows the inheritance, allows to set aesthetics but does not allow to map separately. I'm not sure this is at all possible in ggplot2 (colour, fill, etc. have a special status but I can't find where it is defined).

@jiho
Copy link
Contributor Author

jiho commented Nov 22, 2016

Also, to simplify things, both in code and visually, I would have defaulted to coloured labels with no stroke with fill setting the inside colour and colour setting the text colour. But again, you probably want to be compatible with ggplot2 there.

@aphalo
Copy link
Contributor

aphalo commented Nov 22, 2016

@jiho Yes, very true. Visually a lot cleaner! I think if one wants to keep compatibility, one could have a geom behaving like you suggest under a different name. Most of the code could be shared, I think. (But today it seems only new geoms come to my mind.)

@aphalo
Copy link
Contributor

aphalo commented Nov 22, 2016

@jiho Or would removing the border line/stroke of the labels be better handled as a theme setting?

@slowkow slowkow mentioned this pull request Nov 29, 2016
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