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

element_text size using vector #3492

Closed
mpgerstl opened this issue Aug 19, 2019 · 39 comments · Fixed by #3493
Closed

element_text size using vector #3492

mpgerstl opened this issue Aug 19, 2019 · 39 comments · Fixed by #3493

Comments

@mpgerstl
Copy link

@mpgerstl mpgerstl commented Aug 19, 2019

Hi,

When using a vector for axis text size in a flipped bar chart, the bars seems to be moved far outside the plot region, and also the text is shifted. I am using ggplot2 version 3.2.1.
I was able to produce the wished plot with ggplot2 version 3.1.1

library(tidyverse)
library(tibble)
ds <- mtcars %>%
  rownames_to_column() %>%
  select(car = rowname, mpg)

ds_size = rep(8, nrow(ds))
ds_size[10] <- 12

ds %>%
  ggplot(aes(x = car, y = mpg)) +
  coord_flip() +
  theme(axis.text.y = element_text(size = ds_size))

Thank you

@batpigandme
Copy link
Member

@batpigandme batpigandme commented Aug 19, 2019

Would you mind turning this into a reprex (short for minimal reproducible example)? It's especially helpful for ggplot2, since it automatically generates and uploads the plots (or lack thereof), which makes it much easier to go through at a glance.

If you've never heard of a reprex before, you might want to start by reading the tidyverse.org help page.

Right now the best way to install reprex is:

# install.packages("devtools")
devtools::install_github("tidyverse/reprex")

If you run into problems with access to your clipboard, you can specify an outfile for the reprex, and then copy and paste (or drag and drop) the contents here.

reprex::reprex(input = "fruits_stringdist.R", outfile = "fruits_stringdist.md")

Thanks

@mpgerstl
Copy link
Author

@mpgerstl mpgerstl commented Aug 19, 2019

Thank you for the hint with the clipboard

library(tidyverse)
library(tibble)
ds <- mtcars %>%
  rownames_to_column() %>%
  select(car = rowname, mpg)

ds_size = rep(8, nrow(ds))
ds_size[10] <- 12

ds %>%
  ggplot(aes(x = car, y = mpg)) +
  coord_flip() +
  theme(axis.text.y = element_text(size = ds_size))

Created on 2019-08-19 by the reprex package (v0.3.0)

@batpigandme
Copy link
Member

@batpigandme batpigandme commented Aug 19, 2019

Hmm, I get the same results you've shown above with the dev version.

Here it is with ggplot2 3.0.0

library(tidyverse)

ds <- mtcars %>%
  rownames_to_column() %>%
  select(car = rowname, mpg)

ds_size = rep(8, nrow(ds))
ds_size[10] <- 12

ds %>%
  ggplot(aes(x = car, y = mpg)) +
  coord_flip() +
  theme(axis.text.y = element_text(size = ds_size))


And below is with 3.1.1

library(tidyverse)

ds <- mtcars %>%
  rownames_to_column() %>%
  select(car = rowname, mpg)

ds_size = rep(8, nrow(ds))
ds_size[10] <- 12

ds %>%
  ggplot(aes(x = car, y = mpg)) +
  coord_flip() +
  theme(axis.text.y = element_text(size = ds_size))

Created on 2019-08-19 by the reprex package (v0.3.0)

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Aug 19, 2019

It seems the element takes the sum of the values as margin. BTW, is element_*() supposed to accept vectors? I've seen this kind of tricks somewhere several times, but I guess none of the official docs refer to this.

library(ggplot2)

d <- data.frame(x = letters[1:10], y = 1:10, stringsAsFactors = FALSE)

f <- function(n) {
  ggplot(d) +
    geom_point(aes(x, y)) +
    theme(axis.text.x = element_text(size = rep(8, n)))
}

f(1)

f(10)

f(100)

Created on 2019-08-19 by the reprex package (v0.3.0)

@mpgerstl
Copy link
Author

@mpgerstl mpgerstl commented Aug 19, 2019

@batpigandme Sorry, I forgot the geometry in my reprex. Here is a new one

library(ggplot2)

ds <- data.frame(
  car = rownames(mtcars),
  mpg = mtcars$mpg
)

ds_size = rep(8, nrow(ds))
ds_size[10] <- 12

ggplot(ds, ggplot2::aes(x = car, y = mpg)) +
  geom_col() +
  coord_flip() +
  theme(axis.text.y = element_text(size = ds_size))

Created on 2019-08-19 by the reprex package (v0.3.0)

@yutannihilation I cannot remember where I got the hint, but using a vector for face still works

library(ggplot2)

ds <- data.frame(
  car = rownames(mtcars),
  mpg = mtcars$mpg
)

ds_face = rep("plain", nrow(ds))
ds_face[10] <- "bold"

ggplot(ds, ggplot2::aes(x = car, y = mpg)) +
  geom_col() +
  coord_flip() +
  theme(axis.text.y = element_text(face = ds_face))

Created on 2019-08-19 by the reprex package (v0.3.0)

Thank you

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Aug 19, 2019

Yeah, it works and probably should, but I feet the individual specifications should belong to the scale, not to the theme.

@clauswilke
What do you think? I expect this kind of tricks will be unnecessary once gridtext gets on CRAN.

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Aug 19, 2019

I'm not a big fan of setting text parameters as vectors in this way, mostly because it's unpredictable when it works and when it doesn't. It works for axes but it doesn't work for legends, for internal technical reasons that are not visible to the enduser.

Having said that, I don't understand why this specific example fails. We should track down the cause, because it suggests something weird is going on with the size argument.

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Aug 19, 2019

Thanks, agreed. Let's see what's happening here.

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Aug 19, 2019

Here is what I think is happening. (I don't quite understand yet why it only happens for font size, and not for font face, so maybe I'm wrong.)

This line returns now a vector of descent values rather than just one value:

descent <- font_descent(gp$fontfamily, gp$fontface, gp$fontsize, gp$cex)

ggplot2:::font_descent("", "plain", rep(10, 5), 1)
#> [1] 0.0292290581597222inches 0.0292290581597222inches
#> [3] 0.0292290581597222inches 0.0292290581597222inches
#> [5] 0.0292290581597222inches

Created on 2019-08-19 by the reprex package (v0.3.0)

This vector results in vectors text_width and text_height, which are given to add_margins() here:

ggplot2/R/margins.R

Lines 197 to 205 in 1ae034e

add_margins(
grob = grob_details$text_grob,
height = grob_details$text_height,
width = grob_details$text_width,
gp = gp,
margin = margin,
margin_x = margin_x,
margin_y = margin_y
)

And the add_margins() function then build layouts assuming width and height are single values rather than vectors:

ggplot2/R/margins.R

Lines 115 to 121 in 1ae034e

widths <- unit.c(margin[4], width, margin[2])
heights <- unit.c(margin[1], height, margin[3])
vp <- viewport(
layout = grid.layout(3, 3, heights = heights, widths = widths),
gp = gp
)

Again, this doesn't yet fully explain what exactly is happening, but I think it's on the right track. Importantly, it's based on a line that was changed with the latest release:
f5a88a7#diff-004d6436e77ba3e9705ab23890dace32

The previous function descentDetails() produced the wrong results, but it didn't create vectors.

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Aug 19, 2019

Thanks. For facet strips, it seems we do the right thing to pass a single height and width to add_margins() even when height and/or width is a vector.

ggplot2/R/labeller.r

Lines 608 to 631 in b842024

if (horizontal) {
height <- max_height(lapply(grobs, function(x) x$text_height))
width <- unit(1, "null")
} else {
height <- unit(1, "null")
width <- max_width(lapply(grobs, function(x) x$text_width))
}
# Add margins around text grob
grobs <- apply(
grobs,
c(1, 2),
function(x) {
add_margins(
grob = x[[1]]$text_grob,
height = height,
width = width,
gp = gp,
margin = element$margin,
margin_x = TRUE,
margin_y = TRUE
)
}
)

edit:

But, facet strips doesn't accept variable sizes...

library(ggplot2)

d <- data.frame(x = letters[1:10], y = 1:10, stringsAsFactors = FALSE)

ggplot(d) +
  geom_point(aes(x, y)) +
  facet_wrap(vars(x)) +
  theme(strip.text = element_text(size = 1:10 * 5))

Created on 2019-08-20 by the reprex package (v0.3.0)

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Aug 19, 2019

I made a PR that seems to fix the problem, though I still don't fully understand how exactly things go wrong. #3493

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Aug 20, 2019

Confirmed #3493 fixes the issue. But, I too don't understand how the things are supposed to work here...

Is font_descent() really intended to work with multiple sizes? I see it returns the same values for different sizes in the vector (Does this happen only on Windows?).

ggplot2:::font_descent("", "plain", 10, 1)
#> [1] 0.03125inches
ggplot2:::font_descent("", "plain", c(10, 10), 1)
#> [1] 0.03125inches 0.03125inches

ggplot2:::font_descent("", "plain", c(10, 20), 1)
#> [1] 0.03125inches 0.03125inches
ggplot2:::font_descent("", "plain", 1:10 * 10, 1)
#>  [1] 0.03125inches 0.03125inches 0.03125inches 0.03125inches 0.03125inches
#>  [6] 0.03125inches 0.03125inches 0.03125inches 0.03125inches 0.03125inches

# probably the cache works wrong here...
ggplot2:::font_descent("", "plain", 1:10 * 20, 1)
#>  [1] 0.03125inches            0.03125inches           
#>  [3] 0.03125inches            0.03125inches           
#>  [5] 0.03125inches            0.0520833333333333inches
#>  [7] 0.0520833333333333inches 0.0520833333333333inches
#>  [9] 0.0520833333333333inches 0.0520833333333333inches

Created on 2019-08-20 by the reprex package (v0.3.0)

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Aug 20, 2019

Yes, the font cache is not working correctly for vectorized input. It loops over the keys but then uses the same parameters for descent lookup each time:

ggplot2/R/margins.R

Lines 339 to 355 in b842024

descents <- lapply(key, function(k) {
descent <- descent_cache[[k]]
if (is.null(descent)) {
descent <- convertHeight(grobDescent(textGrob(
label = "gjpqyQ",
gp = gpar(
fontsize = size,
cex = cex,
fontfamily = family,
fontface = face
)
)), 'inches')
descent_cache[[k]] <- descent
}
descent
})

@thomasp85 Do you have any strong opinions on how to fix this? Since font_descent() is only used by title_spec() and that is not vectorized, maybe it's best to let font_descent() return only the first result? In general, the fact that title_spec() is not properly vectorized causes all sorts of problems, but fixing that is a pretty substantial rewrite, and probably better left to something like the gridtext package.

clauswilke added a commit to wilkelab/ggplot2_archive that referenced this issue Aug 20, 2019
@clauswilke
Copy link
Member

@clauswilke clauswilke commented Aug 20, 2019

I made a new PR that simplifies font_descent() by only returning the first result. I'm Ok with that, because even if it returned multiple results correctly the title_spec() function wouldn't know what to do with them.

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Aug 20, 2019

(Just a note to self)

It seems the element takes the sum of the values as margin.

This behaviour comes from here; widthDetails() and heightDetails() takes the sum of the widths and heights of a titleGrob.

ggplot2/R/margins.R

Lines 214 to 216 in b842024

heightDetails.titleGrob <- function(x) {
sum(x$heights)
}

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Aug 20, 2019

But why do we see the behavior only for font size but not for example for font face? I still don't get that.

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Aug 20, 2019

Curiously, it seems another bug... Here, gp$fontface is passed to title_spec(), but gp doesn't contains fontface.

descent <- font_descent(gp$fontfamily, gp$fontface, gp$fontsize, gp$cex)

e.g.

library(ggplot2)

d <- data.frame(x = letters[1:10], y = 1:10, stringsAsFactors = FALSE)

ggplot(d) +
  geom_point(aes(x, y)) +
  theme(axis.text.x = element_text(size = 30, face = rep("plain", 10)))

ggplot(d) +
  geom_point(aes(x, y)) +
  theme(axis.text.x = element_text(size = 30, family = rep("", 10)))

Created on 2019-08-20 by the reprex package (v0.3.0)

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Aug 20, 2019

It seems gpar() treats fontface differently than other parameters.

grid::gpar(
  fontsize = NULL,
  col = NULL,
  fontfamily = NULL,
  fontface = NULL,
  lineheight = NULL
)
#> $fontface
#> NULL

Created on 2019-08-20 by the reprex package (v0.3.0)

As gp keeps fontface = NULL, it removes fontface from element_gp at modify_list(element_gp, gp) here:

ggplot2/R/theme-elements.r

Lines 209 to 219 in b842024

gp <- gpar(fontsize = size, col = colour,
fontfamily = family, fontface = face,
lineheight = lineheight)
element_gp <- gpar(fontsize = element$size, col = element$colour,
fontfamily = element$family, fontface = element$face,
lineheight = element$lineheight)
titleGrob(label, x, y, hjust = hj, vjust = vj, angle = angle,
gp = modify_list(element_gp, gp), margin = margin,
margin_x = margin_x, margin_y = margin_y, debug = element$debug, ...)
}

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Aug 20, 2019

Ok, I see it now. The strange handling of fontface in gpar() is actually one of my number one pet peeves. It takes so much effort to work around it. I'm constantly battling with it in gridtext.

@mpgerstl
Copy link
Author

@mpgerstl mpgerstl commented Aug 20, 2019

I have tested the different parameters for element_text() in the reprex above using vectors.
It is working for: face, colour, hjust and vjust - whereas the font int hjust did not render well
it is not working for: size, angle and family

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Aug 20, 2019

it is not working for: size, angle and family

Opps, thanks...

@clauswilke
#3493 doesn't fix angle. We need to consider the case when angle is a vector here:

ggplot2/R/margins.R

Lines 74 to 75 in b842024

text_height <- unit(1, "grobheight", text_grob) + abs(cos(angle / 180 * pi)) * descent
text_width <- unit(1, "grobwidth", text_grob) + abs(sin(angle / 180 * pi)) * descent

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Aug 20, 2019

Ok. I think we should just ignore all but the first angle. I expect using multiple angles along the axis never worked (but I've never tried it).

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Aug 20, 2019

Agreed.

I expect using multiple angles along the axis never worked (but I've never tried it).

I confirmed it didn't work at the time of v3.1.1.

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Aug 24, 2019

A bit late to this. I honestly think we shouldn’t allow vectorised input in element_text(). As mentioned the behaviour is unpredictable but when it works by chance it gives the impression that it is a feature.

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Aug 24, 2019

There are valid obscure reason for the weirdness of the fontface element in gpar and that is not going to change. For performance reasons the gpar list is treated specially in the grid C internals and fiddling around with it can mess things up unexpectedly

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Aug 24, 2019

Do you remember how many revdep packages were broken when you tried to stop support vectorised input? #3237

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Aug 24, 2019

Sounds like it was just a single package but I honestly can’t remember. I’d propose that we catch this at the element_*() and throw a warning so that it is clear what is not allowed/supported. We will never support this behaviour fully anyway

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Aug 24, 2019

OK, if it's just one or two packages, I agree with the idea of warning. It sounds the right approach.

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Aug 25, 2019

Let's not say "never". My element_markdown() from ggtext calculates margins, descents, grob extents, etc. correctly for vectorized input (not that the example necessarily shows that).

library(ggplot2)
library(ggtext)

d <- data.frame(x = letters[1:16], y = 1:16, stringsAsFactors = FALSE)

ggplot(d) +
  geom_point(aes(x, y)) +
  theme(axis.text.x = element_markdown(size = 3*c(5:12, 11:4)))

Created on 2019-08-24 by the reprex package (v0.3.0)

Also, I feel that the whole title_spec() code is a bit of a mess and should be replaced at some point.

I'll add a warning at the drawing stage, so we get a warning exactly where the code receives input it can't handle. This way, if at some point in the future we have more general support for vectorized input, the warning will simply disappear.

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Aug 25, 2019

I can only agree with you on title_spec(). What I mean with never is that while the element_*() might work vectorised it will depend on the context in which they are called. Your element_markdown() will not behave as if it was vectorised for strip texts so having it work that way for axis text will only confuse.

On a deeper level, I really don’t like that the theming can become tightly coupled with the exact plot, eg your example will only work with plots that have 16 x-axis labels. I don’t think that should be allowed

@hadley
Copy link
Member

@hadley hadley commented Aug 25, 2019

I think @thomasp85’s reasoning is compelling: a theme should not be tied to a specific plot, and any use of vectors should be considered an unsupported hack.

That said, I think people we be unhappy if we remove this without warning so I think it’ll have to go through a deprecation cycle.

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Aug 25, 2019

I agree we should deprecate it carefully, but we should at the least not build out support for the hack🙂

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Aug 25, 2019

FWIW, I think most of the use cases for this hack can be solved properly using element_markdown()

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Aug 25, 2019

I've updated my PR. How is this?

library(ggplot2)

d <- data.frame(x = letters[1:16], y = 1:16, stringsAsFactors = FALSE)

ggplot(d) +
  geom_point(aes(x, y)) +
  theme(axis.text.x = element_text(size = 3*c(5:12, 11:4)))
#> Warning: Vectorized input to `element_text()` is not officially supported.
#> Results may be unexpected or may change in future versions of ggplot2.

Created on 2019-08-25 by the reprex package (v0.3.0)

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Aug 26, 2019

#> Results may be unexpected or may change in future versions of ggplot2.

If we agree to put the vectorized behavior into the deprecation cycle, I think we should choose a bit stronger words to discourage the usage.

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Aug 26, 2019

(Just a side comment)

On a deeper level, I really don’t like that the theming can become tightly coupled with the exact plot

I too felt so, but, after some thinking, now I'm not sure if I can fully agree with this. Leaving the technical details aside, a theme can be tightly coupled with the exact plot; For example, it depends on the length of labels how much angle we should rotate the labels. Using the analogy of HTML and CSS, they are designed to separate theming from the data, but, in practice, elaborated CSSs are so tightly coupled with the exact HTMLs that they cannot be reused easily.

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Aug 26, 2019

I don’t think your example is exactly fitting. What I’m talking about is a deep “technical” coupling — not an aesthetic one. True, label rotation may only look good with packed labels, but there are no technical requirements of the plot that means that a plot will fail to render with certain rotation settings.

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Aug 26, 2019

there are no technical requirements of the plot that means that a plot will fail to render with certain rotation settings.

Ah, I see.

clauswilke added a commit that referenced this issue Aug 26, 2019
* make font_descent() not vectorized. closes #3492.

* work around vectorized angles

* Add warning when element_text() is used with vectorized input.

* add news item
@lock
Copy link

@lock lock bot commented Feb 22, 2020

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock bot locked and limited conversation to collaborators Feb 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants