Implement labeller option for facet_wrap()

[lionel-]

so I finally understood what was the concern in #656. I found a somewhat hacky solution (documented in the source code) that should nevertheless work in most cases.

[hadley]

We are going to make other API breaking changes in the next release, so it would be ok to consider a less-hacky solution, if you're interested.

[lionel-]

I may have time in July.

[hadley]

That should work - my crazy summer conference schedule starts on next Wednesday, and I won't be back doing much development until July 20.

[lionel-]

@hadley Would it be ok to have multiple factors (e.g., ~ cyl + am) labelled on multiple rows as in grid facetting? Currently facet_wrap() label them with a single row with comma separation and I think it'd be better to have the same behaviour as with grid facets. Or should we avoid making visible changes?

[lionel-]

I'll add a parameter to the labellers to specify whether it should be one row or multiple rows. I think multiple rows is more readable but kind of ugly so it'll be nice to have the single row option for publications.

Then we can either change the default of wrap to match that of grid. Or we can have the old behaviour by setting the default to this lambda: labeller = function(labels) label_value(labels, newline = FALSE).

[lionel-]

@hadley ready for review.

Currently facet_wrap() with multiple factors will display the labels on multiple lines by default, which introduces a slight change from previous behaviour. I can change it back if you prefer.

Additionally, I think we should have a new default labeller for both wrap and grid. It would call label_value() in case of a single factor, and label_both() in case of multiple factors. This would make ggplot facets much more readable.

[lionel-]

ok, I added label_default() which behaves as discussed above. I made it the default. Now the following:

ggplot(mtcars, aes(wt, mpg)) +
  geom_point() +
  facet_grid(gear ~ am + cyl)

yields

which is more readable because you can tell which values belong to which variable. Let me know if you'd prefer no changes in the visual defaults.

[lionel-]

If you look closely there is currently a visible thin line separating the rows of the labels with the default theme. And here is what happens when strip.background colour property is set:

I think it's not so nice. I could implement the row separation as a newline \n instead of a gtable row, which would get rid of this. But I don't know if this would break some plots/software relying on this for grid facetting?

[hadley]

I think this should be just class(label_default) <- "labeller") (technically function isn't a class, but a type)

[lionel-]

@hadley when I remove the "function" class, roxygen stops documenting the usage of these functions, which leads to bad documentation and a CMD CHECK warning. I'll leave the function class for now and file an issue for roxygen.

[hadley]

I'd prefer to make no changes to the default display. I also wonder if it would make sense to document all the labeller functions in one Rd file, so there's a place to document how the labeller function works in more detail.

[lionel-]

I'd prefer to make no changes to the default display.

Alright, I'll rename label_default() to label_flexible() then.

Does this also applies to wrap facetting? I.e., currently wrap facets display multiple factors on a single line while grid facets display them on multiple lines. In this PR, they are both displayed consistently as multiple lines but I could pass a lambda function(labels) label_value(labels, multi_line = FALSE) as default argument to facet_wrap() to produce the same default as before.

[hadley]

I think making that consistent is fine - it's fairly unusual to use multiple vars with facet_wrap() anyway.

[lionel-]

@hadley The labellers are now documented together.

Also the last commit proposes another API update for label_bquote(). Before the commit, this labeller mapped variables of multiple factors to x, y and z in the order they occur in the formula. I think this is a bit obscure, so the new commit makes label_bquote() keep the original names. So, instead of

p + facet_grid(. ~ vs + am, labeller = label_bquote(.(x) ^ .(y)))

we have

p + facet_grid(. ~ vs + am, labeller = label_bquote(.(vs) ^ .(am)))

For backward compatibility, it still bounds x to the first factor with a deprecation warning, except when one of the factor facetted over is also called x.

I'll cancel the commit if you prefer the old behaviour.

[hadley]

This will break existing custom labellers, right?

[lionel-]

yes, before the labellers took a value and variable arguments and returned a character vector, now they take and return a rectangular list of character vectors.

[hadley]

Can you make that more clear in the NEWS?

[hadley]

Muultiple typo

[hadley]

This switch seems overly complicated to me. Why not just require that you use a "labeller function", i.e. a function that has arguments labels and multiline? (obviously you're just copying the existing structure, but that now seems overly complex)

[lionel-]

This is a legacy from before. We could remove the ability to use non-labeller functions that take and return a chr vector if you prefer?

Before the labeller function was checking the arguments to determine which functions are labellers, now I use the S3 class because I thought it was more robust. I can make it check the arguments as before if you think that'd be easier to create custom labellers. But we can't rely on multi_line, only on labels.

[hadley]

Ah, I guess it's documented and you've already done it, so lets leave it.

[hadley]

Overall, this looks good. I like the unification across wrap and grid, and the code is not much more complicated than previously.

Any thoughts on unit testing?

[hadley]

Can you please merge/rebase and squash and then I'll merge?

[lionel-]

Here you go

[hadley]

And squash please?

[lionel-]

hmm I can squash into Winston's commit (which is the earliest one) but then he gets credited as the author of the whole patch. What's the proper way to do it?

[hadley]

Oh I see - don't worry about it then.

[lock]

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/