Make make_labels() more consistent #2981

yutannihilation · 2018-11-05T16:04:33Z

Problem

Currently, ggplot2 makes labels in three different ways:

make_labels() uses quo_text(). This handles the cases where aes() is passed to ggplot() or geom_*().
ggplot_add.uneval() uses quo_name(). This handles the case where aes() is +ed to the plot.
qplot() uses quo_name(), (but the implementation is yet different from ggplot_add.uneval()). This handles qplot().

Consequently, we see some inconsistencies among them. For example,

make_labels() adds backticks to invalid symbols (e.g. `a b`), while others don't.
If the mapping is NULL, make_labels() uses the aesthetic name, ggplot_add.uneval() removes the label, and qplot() uses "NULL".

Scope of this PR

The backticks can be easily removed if we stop using quo_text() for symbols. So, at least we can and should fix make_labels().

Next, ideally, we should use make_label() for making labels everywhere. But, in order to keep this PR small, this PR does only these:

Make make_labels() to follow the disirable behaviours (below).
Use make_labels() in ggplot_add.uneval().

and I leave these issues:

qplot() uses different implementation
Facetting also uses different implementation, which uses deparse() (ggplot adds backticks to labels (regression?) #2975 (comment))

Desirable Behaviours

For quosures of a symbol, use symbols as is whether or not the symbol is valid by the R's parser (i.e. do not add backticks). Use as_string(quo_get_expr()) for this.
For other types of quosures, deparse them as R's expressions (i.e. add backticks if necessary). If the expression is long, the label should be abbreviated with a tailing .... Use quo_text() and gsub() for this.
For literals, use the aesthetic name instead.
For NULL, use the aesthetic name instead.

lionel- · 2018-11-05T16:49:37Z

I think it makes sense, but the abbreviation of the calls may cause trouble in revdeps and production plots. Should you maybe run the revdeps to get a sense of how this impacts backward compatibility?

Or you could implement aes_deparse_name() which would use as_string() for symbols and quo_text() for everything else, that might be safer.

yutannihilation · 2018-11-05T23:35:47Z

Thanks, let's care about backward compatibility.

use as_string() for symbols

In this case, it's not symbols; I want to convert quosures of a symbol to strings (my description above is a bit confusing, sorry). What is the supposed way to use as_string() for quosures? as_string(quo_get_expr())? Or, can I just use quo_name() after checking with quo_is_symbol()?

clauswilke · 2018-11-06T00:54:32Z

This seems to work.

make_labels2 <- function(mapping) {
  default_label <- function(aesthetic, mapping) {
    # e.g., geom_smooth(aes(colour = "loess"))
    if (is.atomic(mapping)) {
      aesthetic
    } else if (rlang::quo_is_symbol(mapping)) {
      rlang::as_string(ggplot2:::strip_dots(rlang::get_expr(mapping)))
    } else {
      x <- rlang::quo_text(ggplot2:::strip_dots(mapping))
      if (length(x) > 1) {
        x <- paste0(x[[1]], "...")
      }
      x
    }
  }
  Map(default_label, names(mapping), mapping)
}

m <- ggplot2::aes(`a b`)
ggplot2:::make_labels(m)
#> $x
#> [1] "`a b`"
make_labels2(m)
#> $x
#> [1] "a b"

m <- ggplot2::aes(..x..)
ggplot2:::make_labels(m)
#> $x
#> [1] "x"
make_labels2(m)
#> $x
#> [1] "x"

m <- ggplot2::aes(15)
ggplot2:::make_labels(m)
#> $x
#> [1] "x"
make_labels2(m)
#> $x
#> [1] "x"

m <- ggplot2::aes(2*x*exp(`coef 1`*x^2))
ggplot2:::make_labels(m)
#> $x
#> [1] "2 * x * exp(`coef 1` * x^2)"
make_labels2(m)
#> $x
#> [1] "2 * x * exp(`coef 1` * x^2)"

^{Created on 2018-11-05 by the reprex package (v0.2.1)}

lionel- · 2018-11-06T02:54:36Z

Instead of get_expr() I would use quo_get_expr() because it fails early if the input type is not right.

yutannihilation · 2018-11-06T03:17:18Z

Thanks! I would use the quo_get_expr() version of @clauswilke's implementation.

BTW, is it OK to remove this if branch? I don't understand what case the current code considers since rlang::quo_text() always returns a single string (right?).

      if (length(x) > 1) {
        x <- paste0(x[[1]], "...")
      }

clauswilke · 2018-11-06T03:26:00Z

It seems that the current implementation introduces newlines instead of returning a vector of strings. So maybe check for the presence of a newline and if one is there split the string there, take the first part, and add "..."?

make_labels2 <- function(mapping) {
  default_label <- function(aesthetic, mapping) {
    # e.g., geom_smooth(aes(colour = "loess"))
    if (is.atomic(mapping)) {
      aesthetic
    } else if (rlang::quo_is_symbol(mapping)) {
      rlang::as_string(ggplot2:::strip_dots(rlang::quo_get_expr(mapping)))
    } else {
      x <- rlang::quo_text(ggplot2:::strip_dots(mapping))
      if (grepl("\\n", x)) {
        x <- paste0(strsplit(x, "\\n")[[1]][1], "...")
      }
      x
    }
  }
  Map(default_label, names(mapping), mapping)
}

m <- ggplot2::aes(2*x*exp(`coef 1`*x^2)*2*x*exp(`coef 1`*x^2)*2*x*exp(`coef 1`*x^2)*2*x*exp(`coef 1`*x^2)*2*x*exp(`coef 1`*x^2)*2*x*exp(`coef 1`*x^2)*2*x*exp(`coef 1`*x^2)*2*x*exp(`coef 1`*x^2))
ggplot2:::make_labels(m)
#> $x
#> [1] "2 * x * exp(`coef 1` * x^2) * 2 * x * exp(`coef 1` * x^2) * 2 * \n    x * exp(`coef 1` * x^2) * 2 * x * exp(`coef 1` * x^2) * 2 * \n    x * exp(`coef 1` * x^2) * 2 * x * exp(`coef 1` * x^2) * 2 * \n    x * exp(`coef 1` * x^2) * 2 * x * exp(`coef 1` * x^2)"
make_labels2(m)
#> $x
#> [1] "2 * x * exp(`coef 1` * x^2) * 2 * x * exp(`coef 1` * x^2) * 2 * ..."

^{Created on 2018-11-05 by the reprex package (v0.2.1)}

clauswilke · 2018-11-06T03:31:52Z

I edited my previous comment to include an example implementation of what I meant.

lionel- · 2018-11-06T03:40:15Z

In rlang I also see:

name <- quo_text(expr)
name <- gsub("\n.*$", "...", name)

clauswilke · 2018-11-06T04:21:51Z

Yes, that's cleaner. Though I don't understand whether it's "\n.*$" or "\\n.*$". Both seem to work.

make_labels2 <- function(mapping) {
  default_label <- function(aesthetic, mapping) {
    # e.g., geom_smooth(aes(colour = "loess"))
    if (is.atomic(mapping)) {
      aesthetic
    } else if (rlang::quo_is_symbol(mapping)) {
      rlang::as_string(ggplot2:::strip_dots(rlang::quo_get_expr(mapping)))
    } else {
      x <- rlang::quo_text(ggplot2:::strip_dots(mapping))
      gsub("\n.*$", "...", x)
    }
  }
  Map(default_label, names(mapping), mapping)
}

m <- ggplot2::aes(2*x*exp(`coef 1`*x^2)*2*x*exp(`coef 1`*x^2)*2*x*exp(`coef 1`*x^2)*2*x*exp(`coef 1`*x^2)*2*x*exp(`coef 1`*x^2)*2*x*exp(`coef 1`*x^2)*2*x*exp(`coef 1`*x^2)*2*x*exp(`coef 1`*x^2))
ggplot2:::make_labels(m)
#> $x
#> [1] "2 * x * exp(`coef 1` * x^2) * 2 * x * exp(`coef 1` * x^2) * 2 * \n    x * exp(`coef 1` * x^2) * 2 * x * exp(`coef 1` * x^2) * 2 * \n    x * exp(`coef 1` * x^2) * 2 * x * exp(`coef 1` * x^2) * 2 * \n    x * exp(`coef 1` * x^2) * 2 * x * exp(`coef 1` * x^2)"
make_labels2(m)
#> $x
#> [1] "2 * x * exp(`coef 1` * x^2) * 2 * x * exp(`coef 1` * x^2) * 2 * ..."

^{Created on 2018-11-05 by the reprex package (v0.2.1)}

yutannihilation · 2018-11-06T05:17:24Z

Ah, got it! Thanks. I didn't know the difference between quo_text() and deparse()...

Note that, to be precise about the backward compatibility, it seems that ... had never appeared in the release versions of ggplot2. Version 2.2.1 uses deparsed string as is (https://github.com/tidyverse/ggplot2/blob/v2.2.1/R/labels.r#L86):

# Convert aesthetic mapping into text labels
make_labels <- function(mapping) {
  remove_dots <- function(x) {
    gsub(match_calculated_aes, "\\1", x)
  }

  default_label <- function(aesthetic, mapping) {
    # e.g., geom_smooth(aes(colour = "loess"))
    if (is.character(mapping)) {
      aesthetic
    } else {
      remove_dots(deparse(mapping))
    }
  }
  Map(default_label, names(mapping), mapping)
}

I don't follow the codes detailedly, but it seems only the first element is used for labeling.

library(ggplot2)

packageVersion("ggplot2")
#> [1] '2.2.1'

d <- data.frame(a = 1)

ggplot(d, aes(a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a + a)) +
  geom_bar()

^{Created on 2018-11-06 by the reprex package (v0.2.1)}

But, using gsub() seems to do the right thing to me. So, I think I'm going to implement as such if I find some reason not to :)

yutannihilation · 2018-11-06T12:30:25Z

For ggplot_add.uneval(), I chose to follow #2975 (comment) and use make_labels() as well.

For qplot(), I'm failing to see what is the expected way to make labels, so I want to leave them as is for now...

library(ggplot2)

patchwork::wrap_plots(
  qplot(x = 1, geom = "vline"),
  qplot(x = 1, y = NULL, geom = "vline"),
  ggplot(mapping = aes(x = 1)) + geom_vline(),
  ggplot(mapping = aes(x = 1, y = NULL)) + geom_vline()
)

^{Created on 2018-11-06 by the reprex package (v0.2.1)}

yutannihilation · 2018-11-06T12:52:40Z

Hmm, it seems the handling of NULL diverged after #2617, but maybe make_labels(aes(x = NULL)) should return NULL for x?

lionel- · 2018-11-06T12:54:39Z

Perhaps this patch should make minimal changes and we'll solve the problem more fully as part of r-lib/rlang#636

hadley · 2018-11-06T12:59:49Z

At a minimum, we should write down the labelling principles that ggplot2 wants, so we can make sure to enforce them in rlang. I think the reason that quo_name(), quo_text() etc keep changing is that I failed to properly analyse and document the use cases.

yutannihilation · 2018-11-06T13:26:47Z

At a minimum, we should write down the labelling principles that ggplot2 wants

I'm not fully sure, but I think the test cases I added (hopefully) cover the principles minimally, and, in order to pass them, the changes would be this size.

Of course I can wait for r-lib/rlang#636 if necessary :)

clauswilke · 2018-11-06T14:54:55Z

I think NULL was handled correctly before. The principle seems to be that we use the aesthetic name when a specific value is provided. This makes sense, because we want the x axis to be called x, not 15. Similarly, we also don‘t want the x axis to be called NULL.

yutannihilation · 2018-11-06T15:33:29Z

I think NULL was handled correctly before.

Now I gradually feel you are right, but let me confirm what you mean by "before". Currently, ggplot2 handles NULL in three different ways:

make_labels() uses the aesthetic name instead for the label
ggplot_add.uneval() displays no label (introduced by Handle null aesthetics when labelling #2617)
qplot() uses "NULL" for the label

Do you mean 1. is the supposed behaviour? (I think we can agree 3. is not the answer at least)

library(ggplot2)

patchwork::wrap_plots(
  ggplot(mapping = aes(x = 1, y = NULL)) + geom_vline() + ggtitle("make_labels()"),
  ggplot() + aes(x = 1, y = NULL) + geom_vline() + ggtitle("ggplot_add.uneval()"),
  qplot(x = 1, y = NULL, geom = "vline") + ggtitle("qplot()")
)

^{Created on 2018-11-07 by the reprex package (v0.2.1)}

hadley · 2018-11-06T15:40:14Z

I'd consider 1 to be the desired behaviour. 3 is bad, but it's not necessary to fix it (unless it's easy)

clauswilke · 2018-11-06T15:40:14Z

Yes, I think option 1 is the most likely to make sense in an ambiguous situation.

library(ggplot2)
ggplot(mapping = aes(x = 1, y = NULL)) + geom_point(data = data.frame(x = 1, y = 1), mapping = aes(x, y))

ggplot() + aes(x = 1, y = NULL) + geom_point(data = data.frame(x = 1, y = 1), mapping = aes(x, y))

^{Created on 2018-11-06 by the reprex package (v0.2.1)}

yutannihilation · 2018-11-06T16:08:57Z

Thanks! OK, I'll modify the codes to follow option 1.

This reverts commit f75440b.

yutannihilation · 2018-11-10T06:38:16Z

I updated the description of this PR. Does this correctly reflect our discussion so far?

clauswilke · 2018-11-10T06:49:20Z

Yes, I think so.

lionel- · 2018-11-10T07:20:49Z

tests/testthat/test-aes-calculated.r

+  expect_identical(make_labels(aes(x = `a b`)), list(x = "a b"))
+  # long expression is abbreviated with ...
+  expect_identical(make_labels(aes(x = 2 * x * exp(`coef 1` * x^2) * 2 * x * exp(`coef 1` * x^2) * 2 * x)),
+                   list(x = "2 * x * exp(`coef 1` * x^2) * 2 * x * exp(`coef 1` * x^2) * 2 * ..."))


Can you grep for ... instead please? This way the test does't depend on how quo_text() deparses complex expressions.

yutannihilation · 2018-11-11T02:56:27Z

@clauswilke @dpseidel May I merge this now? If you are planning a quick release with the fix for the transformation of secondary axis, I think I should wait. Though this PR is only about fixing a minor bug, worries about breaking revdeps should be minimized...

clauswilke · 2018-11-11T05:54:57Z

I think this should be merged now. The change in rlang had much bigger implications for revdeps, and this PR mostly undoes that change.

yutannihilation · 2018-11-11T06:57:19Z

The change in rlang had much bigger implications for revdeps, and this PR mostly undoes that change.

Thanks, makes sense!

lock · 2019-05-10T08:09:46Z

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/

yutannihilation added 2 commits November 5, 2018 23:17

Add a failing test about labelling

a21baea

Use quo_name() for making labels

e9fe0ed

hadley requested a review from lionel- November 5, 2018 16:42

yutannihilation added 4 commits November 6, 2018 21:06

Add more tests about make_labels()

c9c3b0c

Improve make_labels()

2f3fffd

Add a case when the mapping is NULL

bec320f

Use make_labels() in ggplot_add.uneval() as well

2552f95

yutannihilation changed the title ~~Use quo_name() for making labels~~ Make make_labels() more consitent Nov 6, 2018

Return NULL if the mapping is NULL

f75440b

clauswilke changed the title ~~Make make_labels() more consitent~~ Make make_labels() more consistent Nov 6, 2018

yutannihilation mentioned this pull request Nov 6, 2018

ggplot adds backticks to labels (regression?) #2975

Closed

yutannihilation added 2 commits November 7, 2018 01:11

Revert "Return NULL if the mapping is NULL"

71c00b2

This reverts commit f75440b.

Tweak a test

dde6093

lionel- reviewed Nov 10, 2018

View reviewed changes

lionel- approved these changes Nov 10, 2018

View reviewed changes

yutannihilation added 3 commits November 11, 2018 11:13

Soften the test for a long expression

1a96032

Add a news bullet

8d7bb60

Fix a typo

64529db

Merge branch 'master' into issue-2975-fix-make-label

4df7acc

yutannihilation merged commit 868fdb7 into tidyverse:master Nov 11, 2018

yutannihilation deleted the issue-2975-fix-make-label branch November 11, 2018 07:30

yutannihilation mentioned this pull request Nov 27, 2018

backticks on non-ascii labels #3017

Closed

batpigandme mentioned this pull request Mar 14, 2019

Backticks in labels when aesthetics are defined in a geom #3186

Closed

lock bot locked and limited conversation to collaborators May 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make make_labels() more consistent #2981

Make make_labels() more consistent #2981

yutannihilation commented Nov 5, 2018 •

edited

Loading

lionel- commented Nov 5, 2018

yutannihilation commented Nov 5, 2018

clauswilke commented Nov 6, 2018 •

edited

Loading

lionel- commented Nov 6, 2018

yutannihilation commented Nov 6, 2018

clauswilke commented Nov 6, 2018 •

edited

Loading

clauswilke commented Nov 6, 2018

lionel- commented Nov 6, 2018

clauswilke commented Nov 6, 2018

yutannihilation commented Nov 6, 2018 •

edited

Loading

yutannihilation commented Nov 6, 2018

yutannihilation commented Nov 6, 2018 •

edited

Loading

lionel- commented Nov 6, 2018

hadley commented Nov 6, 2018

yutannihilation commented Nov 6, 2018

clauswilke commented Nov 6, 2018

yutannihilation commented Nov 6, 2018

hadley commented Nov 6, 2018

clauswilke commented Nov 6, 2018

yutannihilation commented Nov 6, 2018

yutannihilation commented Nov 10, 2018

clauswilke commented Nov 10, 2018

lionel- Nov 10, 2018

yutannihilation Nov 11, 2018

yutannihilation commented Nov 11, 2018

clauswilke commented Nov 11, 2018

yutannihilation commented Nov 11, 2018

lock bot commented May 10, 2019

Make make_labels() more consistent #2981

Make make_labels() more consistent #2981

Conversation

yutannihilation commented Nov 5, 2018 • edited Loading

Problem

Scope of this PR

Desirable Behaviours

lionel- commented Nov 5, 2018

yutannihilation commented Nov 5, 2018

clauswilke commented Nov 6, 2018 • edited Loading

lionel- commented Nov 6, 2018

yutannihilation commented Nov 6, 2018

clauswilke commented Nov 6, 2018 • edited Loading

clauswilke commented Nov 6, 2018

lionel- commented Nov 6, 2018

clauswilke commented Nov 6, 2018

yutannihilation commented Nov 6, 2018 • edited Loading

yutannihilation commented Nov 6, 2018

yutannihilation commented Nov 6, 2018 • edited Loading

lionel- commented Nov 6, 2018

hadley commented Nov 6, 2018

yutannihilation commented Nov 6, 2018

clauswilke commented Nov 6, 2018

yutannihilation commented Nov 6, 2018

hadley commented Nov 6, 2018

clauswilke commented Nov 6, 2018

yutannihilation commented Nov 6, 2018

yutannihilation commented Nov 10, 2018

clauswilke commented Nov 10, 2018

lionel- Nov 10, 2018

Choose a reason for hiding this comment

yutannihilation Nov 11, 2018

Choose a reason for hiding this comment

yutannihilation commented Nov 11, 2018

clauswilke commented Nov 11, 2018

yutannihilation commented Nov 11, 2018

lock bot commented May 10, 2019

yutannihilation commented Nov 5, 2018 •

edited

Loading

clauswilke commented Nov 6, 2018 •

edited

Loading

clauswilke commented Nov 6, 2018 •

edited

Loading

yutannihilation commented Nov 6, 2018 •

edited

Loading

yutannihilation commented Nov 6, 2018 •

edited

Loading