Manual scale limits default to present names of values

[teunbrand]

Why this PR?

In ggplot2 v3.3.4, #4471 was introduced to allow fewer items in values than auto-detected breaks.
This has had some consequences, most notably that an extra legend key was produced if a named values was longer than auto-detected breaks (#4511, #4534, #4556, #4545). One workaround is currently to use limits = force, which feels unneccessary (#4511 (comment)). Additionally, the ordering of the legend keys was effected by the names in the values argument (#4616).

What does this PR do?

This PR aims to revert back to the old behaviour (in most part), while preserving the spirit of #4471. It does so by replacing a vector-based limits by a function-based limits that intersects the names of the values by the observed limits.

Foreseen problems

Both this PR and #4547 aim to do the same thing and therefore these PRs are in conflict. This PR solely affects manual scales, whereas all scales are affected in #4547.

Examples

library(ggplot2)

p <- ggplot(mpg, aes(displ, hwy, colour = drv)) +
  geom_point(size = 2)

values <- c("f" = "dodgerblue", "r" = "tomato", "4" = "limegreen", 
            "dummy" = "black")

# Does not change default order (#4616), nor produces extra key (#4511)
p + scale_colour_manual(
    values = values
  )

# Can add extra key by setting limits
# The plot is the same as CRAN version without limits argument
p + scale_colour_manual(
  values = values,
  limits = names(values)
)

# Can set undefined values to na.value (spirit of #4471)
p + scale_colour_manual(
  values = values[1:2]
)

# Can set fewer keys by setting breaks (same as CRAN version)
p + scale_colour_manual(
  values = values,
  breaks = names(values)[1:2]
)

^{Created on 2021-09-15 by the reprex package (v2.0.1)}

[yutannihilation]

Thanks, while I'm not fully confident yet, this pull request looks the right direction to me. Manual scales are somewhat special, so it's probably a good idea to treat them specially, rather than modifying the whole Scale.

[clauswilke]

I have to admit I'm quite confused by this. It's hard to keep track of the many different issues that were filed. I thought we had merged the limits = force solution. Is that not the case?

In any case, @teunbrand, could you add tests and a news item, just like in #4547? It would be good to have a test for every single problem that has shown up.

And also, #4471 was exclusive to manual scales, so a fix exclusive to manual scales would be preferred. This argues in favor of this PR over #4547.

[yutannihilation]

It's hard to keep track of the many different issues that were filed.

I too feel so...

I thought we had merged the limits = force solution.

If I understand correctly, this pull request is based on the same idea. This comment (#4511 (comment)) might help:

In addition: perhaps, if the aim of #4471 was to allow fewer elements, then the line above could be changed to:

[yutannihilation]

In any case, @teunbrand, could you add tests and a news item, just like in #4547? It would be good to have a test for every single problem that has shown up.

I hope we can narrow #4547 to only the tests and merge it. Even if we'll choose this pull request, it's good if we can count these prior works on #4547 as a contribution.

[clauswilke]

Ah, Ok. I have no concerns about this, except the order of merging. Can we merge the test before merging the fix?

[yutannihilation]

Yes, let's merge the tests first.

[banfai]

@clauswilke @yutannihilation
I agree, this is a better solution. I have modified #4547 as requested to contain the tests only.

[banfai]

@teunbrand
This PR should include an update to NEWS.md
It could be changed to something like this, if you agree to share credit 😉

* Fixes issue with `scale_*_manual` introduced in #4471 where extra elements
  in `values` or their ordering changes the legend
  (@teunbrand, @banfai, #4511, #4534, #4556, #4545, #4616)

[teunbrand]

@banfai Yes of course, I'm sorry for the conflict with your PR and the hassle that came with it!

[yutannihilation]

@banfai
Ah, thanks, I was going to ask to include you in NEWS otherwise :)

@teunbrand
Now that #4547 is merged, could you merge the current master for tests?

[yutannihilation]

Minor comments. Let me for a while to check if this fixes all the problems that are reported so far.

I'd also like to request to add a short comment on the if branch why limits needs to be a function.

[yutannihilation]

Hmm, I have no idea what this error is...

══ Failed tests ════════════════════════════════════════════════════════════════
── Error (test-scale-manual.r:119:3): limits and breaks (#4619) ────────────────
Error: Error: argument is missing, with no default
Error: Error: R CMD check found ERRORs
Execution halted
Backtrace:
    █
 1. └─ggplot2::scale_colour_manual(...) test-scale-manual.r:119:2
 2.   └─ggplot2:::manual_scale(aesthetics, values, breaks, ..., na.value = na.value)
 3.     └─ggplot2::discrete_scale(...)
 4.       └─ggplot2::ggproto(...)

[ FAIL 1 | WARN 0 | SKIP 66 | PASS 1619 ]

[yutannihilation]

Ah, I should have caught that trailing comma, sorry.

[teunbrand]

Alright this seems all in its intended state for now :) Thanks for the feedback @yutannihilation!

Let me for a while to check if this fixes all the problems that are reported so far.

That seems like a good plan to me, please let me know if you find something.

[banfai]

@teunbrand This was a better solution, I'm glad we could reconcile the two PRs.
@yutannihilation Sorry about the trailing comma error, there used to be an na.values arg there and it didn't throw an error on my machine.

[yutannihilation]

@banfai
No worries, it seems something changed between R 3.4 and 3.5. It's what only the CI can find, so it's not your fault. I was a bit too lazy to check the details of the failures.

[yutannihilation]

I confirmed this solves all the related issues. Now this looks good to me!

c.f. just in case this is useful for someone, this is my quick summary of the issues.
https://gist.github.com/yutannihilation/3d0954fe2a183c08288807215e3c9705

[yutannihilation]

The word "previous behaviour" might be a bit confusing to those who want further previous behaviour, i.e. behaviour before v3.3.4. How about describing it a bit more explicitly something like "To display all the values on the legend, use scale_*_manual(values = vals, limits = names(vals))"? What do you think?

[clauswilke]

Agreed, "previous behavior" is confusing. Better to describe the specific behavior. I like the proposed sentence.

[yutannihilation]

Perfect, thank you so much!

[dankessler]

I noticed that this fix does not seem to have made its way into the current release on cran (v3.3.6). It is merged into the release candidate branch v3.3.6-rc, but it isn't merged into the v3.3.6 release tag. Indeed, there are many commits that are in the release candidate branch but not in the release. I tried to find some documentation of ggplot's branching workflow, but couldn't, so it's possible I'm mistaken, but it seemed odd to me that the release candidate branch hadn't been merged into its corresponding release.

Update: Just found this comment which suggests that release 3.3.6 was very minor and suggests that it is not in fact based on the release candidate branch v3.3.6-rc. I see that the v3.3.6-rc branch is contained in the v3.4.0-rc, so perhaps the latter has superseded the former.