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

Make make_labels() more consistent #2981

Merged

Conversation

yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Nov 5, 2018

Fix #2975

Problem

Currently, ggplot2 makes labels in three different ways:

  1. make_labels() uses quo_text(). This handles the cases where aes() is passed to ggplot() or geom_*().
  2. ggplot_add.uneval() uses quo_name(). This handles the case where aes() is +ed to the plot.
  3. 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() (#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.

@hadley hadley requested a review from lionel- Nov 5, 2018
@lionel-
Copy link
Member

@lionel- lionel- commented Nov 5, 2018

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
Copy link
Member Author

@yutannihilation yutannihilation commented Nov 5, 2018

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
Copy link
Member

@clauswilke clauswilke commented Nov 6, 2018

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-
Copy link
Member

@lionel- lionel- commented Nov 6, 2018

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

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Nov 6, 2018

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
Copy link
Member

@clauswilke clauswilke commented Nov 6, 2018

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
Copy link
Member

@clauswilke clauswilke commented Nov 6, 2018

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

@lionel-
Copy link
Member

@lionel- lionel- commented Nov 6, 2018

In rlang I also see:

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

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Nov 6, 2018

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
Copy link
Member Author

@yutannihilation yutannihilation commented Nov 6, 2018

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
Copy link
Member Author

@yutannihilation yutannihilation commented Nov 6, 2018

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 yutannihilation changed the title Use quo_name() for making labels Make make_labels() more consitent Nov 6, 2018
@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Nov 6, 2018

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

@lionel-
Copy link
Member

@lionel- lionel- commented Nov 6, 2018

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

@hadley
Copy link
Member

@hadley hadley commented Nov 6, 2018

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
Copy link
Member Author

@yutannihilation yutannihilation commented Nov 6, 2018

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 clauswilke changed the title Make make_labels() more consitent Make make_labels() more consistent Nov 6, 2018
@clauswilke
Copy link
Member

@clauswilke clauswilke commented Nov 6, 2018

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
Copy link
Member Author

@yutannihilation yutannihilation commented Nov 6, 2018

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:

  1. make_labels() uses the aesthetic name instead for the label
  2. ggplot_add.uneval() displays no label (introduced by #2617)
  3. 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
Copy link
Member

@hadley hadley commented Nov 6, 2018

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
Copy link
Member

@clauswilke clauswilke commented Nov 6, 2018

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
Copy link
Member Author

@yutannihilation yutannihilation commented Nov 6, 2018

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

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Nov 10, 2018

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

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Nov 10, 2018

Yes, I think so.

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 * ..."))
Copy link
Member

@lionel- lionel- Nov 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@yutannihilation yutannihilation Nov 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Nov 11, 2018

@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
Copy link
Member

@clauswilke clauswilke commented Nov 11, 2018

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
Copy link
Member Author

@yutannihilation yutannihilation commented Nov 11, 2018

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

Thanks, makes sense!

@yutannihilation yutannihilation merged commit 868fdb7 into tidyverse:master Nov 11, 2018
4 checks passed
@yutannihilation yutannihilation deleted the issue-2975-fix-make-label branch Nov 11, 2018
@lock
Copy link

@lock lock bot commented May 10, 2019

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 May 10, 2019
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 this pull request may close these issues.

4 participants