Parse problem for label axis using scales::parse_format

[agila5]

Hi! I want to create a plot with col and axis labels that need to be firstly parsed. I read here that I have to use scales::parse_format and, while it works for the fill legend, it does not seem to work for the x axis since it shows only the ticks . Am I doing something wrong? Is this a bug?

I originally posted the same issue at the github page of ggplot2 and they suggested that this could be a bug with scales since the same example works using ggplot2 internal functions.

# packages
library(ggplot2)

# data
fake_data <- data.frame(
  classes = c("alpha", "beta"),
  value = c(1, 2),
  stringsAsFactors = FALSE
)

ggplot(fake_data) + 
  geom_col(aes(x = classes, y = value, fill = classes)) + 
  scale_fill_discrete(labels = scales::parse_format()) + 
  scale_x_discrete(labels = scales::parse_format())

^{Created on 2019-04-09 by the reprex package (v0.2.1)}

[clauswilke]

The problem is that scales::parse_format() produces a list of expressions when what is needed is an expression object:

scales::parse_format()(c("alpha", "beta"))
#> [[1]]
#> expression(alpha)
#> 
#> [[2]]
#> expression(beta)
ggplot2:::parse_safe(c("alpha", "beta"))
#> expression(alpha, beta)

^{Created on 2019-04-09 by the reprex package (v0.2.1)}

This problem was discussed in detail in this ggplot2 issue: tidyverse/ggplot2#2864

Probably the right way forward is to copy ggplot2:::parse_safe() over to scales and use it for scales::parse_format().

[dpseidel]

Interesting to me that the list of expressions works for one discrete scale but not the other....:thinking:

I can build a PR and test this change out later this week (dissertation deadlines looming right now). Or, alternatively, happy to help review if either of you want to work one up a little sooner.

[clauswilke]

Interesting to me that the list of expressions works for one discrete scale but not the other....🤔

That's because the axis labels are drawn with one single text grob but the legend uses one text grob per label. It had me confused for a long time when we discussed tidyverse/ggplot2#2864.

[clauswilke]

Oh, and I'll probably not get around to making a PR at this time, so I'm happy to leave this to you.

[dpseidel]

Great! I'll get to it when I can. And thanks for the quick explanation - I'll definitely read through the larger discussion when I get a minute. 👍

[agila5]

I'd like to help with this issue too but I'm not really sure what to do to create a useful PR. I'll try to do it tomorrow following the instructions reported here.

[dpseidel]

I would be happy to help you work through a PR! That's a great link to start with. Also, the ggplot2 contributing guide has some useful general advice.

Open a PR whenever you're ready, don't worry about it being absolutely perfect, and we can iterate on it together.

[agila5]

I implemented some changes to the parse_format function following the suggestion proposed by @clauswilke (i.e. copying the code in ggplot2:::parse_safe()) but there is a problem with one of the common tests that I can't easily solve.

The new function definition should be the following:

new_parse_format <- function(text) {
  stopifnot(is.character(text))
  out <- vector("expression", length(text))
  for (i in seq_along(text)) {
    expr <- parse(text = text[[i]])
    out[[i]] <- if (length(expr) == 0)
      NA
    else expr[[1]]
  }
  out
}

One of the last tests check that parse_format()(x) is identical to list() when x is an empty numeric vector:

scales/tests/testthat/test-formatter.r

Line 341 in c374014

expect_identical(parse_format()(x), list())

and, reading #203 (comment), I changed the test as

expect_identical(parse_format(x), expression())

The problem is that the function does not work since x is not a character vector but an empty numeric vector. Should I create a new ad-hoc test? Otherwise I could modify the function definition but I'm not sure how.

new_parse_format <- function(text) {
  stopifnot(is.character(text))
  out <- vector("expression", length(text))
  for (i in seq_along(text)) {
    expr <- parse(text = text[[i]])
    out[[i]] <- if (length(expr) == 0)
      NA
    else expr[[1]]
  }
  out
}

new_parse_format(c("alpha", "beta", "gamma"))
#> expression(alpha, beta, gamma)
new_parse_format(c(""))
#> expression(NA)
new_parse_format(character())
#> expression()
new_parse_format(numeric())
#> Error in new_parse_format(numeric()): is.character(text) is not TRUE

^{Created on 2019-04-10 by the reprex package (v0.2.1)}

A possible modification could be:

modified_parse_format <- function(text) {
  if(length(text) == 0L) return(expression())
  stopifnot(is.character(text))
  out <- vector("expression", length(text))
  for (i in seq_along(text)) {
    expr <- parse(text = text[[i]])
    out[[i]] <- if (length(expr) == 0)
      NA
    else expr[[1]]
  }
  out
}

modified_parse_format(c("alpha", "beta", "gamma"))
#> expression(alpha, beta, gamma)
modified_parse_format(c(""))
#> expression(NA)
modified_parse_format(character())
#> expression()
modified_parse_format(numeric())
#> expression()
modified_parse_format(list())
#> expression()
modified_parse_format(integer())
#> expression()

^{Created on 2019-04-10 by the reprex package (v0.2.1)}

Is it ok?

[clauswilke]

It's common that we have to adjust tests so they reflect our latest understanding of how a function should work. It seems the most important test, which doesn't exist currently, is that character() as input returns expression() as output.

Regarding numeric input, I don't think the function should special case input of length zero. It should just do whatever it does with finite-length numeric input. In general, there are two reasonable options:

1. Throw an error if the input is not character. (That's what the ggplot2 function does.)
2. Coerce the input to character and then proceed.

I lean towards option 2 and don't recall why we chose option 1 for ggplot2. This may be a question for @hadley.

[clauswilke]

Paging @slowkow also since he wrote the original parse_safe() function for ggplot2.

[slowkow]

I agree with Claus that adjusting the tests to fit latest understanding is the way to go.

I think my approach at the time was to search for all instances of parse() in the ggplot2 code. I noticed that it was usually parse(as.character(x)) rather than parse(x), so this motivated me to add the stopifnot(is.character(text)) line.

After reviewing the discussion, I cannot find any good reason why I wrote stopifnot(is.character(text)), and I can't remember why I would have done that. Sorry! Maybe we don't need that?

My guess is that you can safely delete this line, but you might consider checking some tests to build confidence.

[clauswilke]

Instead of deleting this line, I would replace it with text <- as.character(text). That seems to be doing the right thing in most contexts one might encounter.

new_parse_format <- function(text) {
  text <- as.character(text)
  out <- vector("expression", length(text))
  for (i in seq_along(text)) {
    expr <- parse(text = text[[i]])
    out[[i]] <- if (length(expr) == 0)
      NA
    else expr[[1]]
  }
  out
}

new_parse_format(1:5)
#> expression(1, 2, 3, 4, 5)
new_parse_format(c("a", "b", "c", "a"))
#> expression(a, b, c, a)
new_parse_format(factor(c("a", "b", "c", "a")))
#> expression(a, b, c, a)
new_parse_format(expression(a*b+c, b, c, a))
#> expression(a * b + c, b, c, a)

^{Created on 2019-04-10 by the reprex package (v0.2.1)}

[agila5]

On the help page of the function parse it's written text : character vector. The text to parse. Elements are treated as if they were lines of a file. Other R objects will be coerced to character if possible. Does it mean that we can also remove the line text <- as.character(text)? It seems to be ok.

new_parse_format <- function(text) {
  out <- vector("expression", length(text))
  for (i in seq_along(text)) {
    expr <- parse(text = text[[i]])
    out[[i]] <- if (length(expr) == 0)
      NA
    else expr[[1]]
  }
  out
}

new_parse_format(1:5)
#> expression(1, 2, 3, 4, 5)
new_parse_format(c("a", "b", "c", "a"))
#> expression(a, b, c, a)
new_parse_format(factor(c("a", "b", "c", "a")))
#> expression(1, 2, 3, 1)
new_parse_format(expression(a*b+c, b, c, a))
#> expression(+a * b, b, c, a)
new_parse_format(numeric())
#> expression()
new_parse_format(NULL)
#> expression()
new_parse_format(NA)
#> expression(NA)

^{Created on 2019-04-10 by the reprex package (v0.2.1)}

EDIT: Sorry, I didn't check the factor and the expression which are clearly wrong. Anyway thank you both, tomorrow I'll try to create the PR.