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

Rendering book fails at scales-other.qmd #5991

Closed
py9mrg opened this issue Jul 10, 2024 · 7 comments · Fixed by #5992
Closed

Rendering book fails at scales-other.qmd #5991

py9mrg opened this issue Jul 10, 2024 · 7 comments · Fixed by #5992

Comments

@py9mrg
Copy link

py9mrg commented Jul 10, 2024

Hello,

When rendering the book (quarto render --to pdf) there is a failure in scales-other.qmd as per:

Error in `discrete_scale()`:
! formal argument "palette" matched by multiple actual arguments
Backtrace:
 1. ggplot2::scale_linetype(palette = linetypes)



Quitting from lines 259-266 [unnamed-chunk-18] (scales-other.qmd)
Execution halted

The offending lines are:

linetypes <- function(n) {
  types <- c("55",  "75", "95", "1115", "111115", "11111115",
             "5158", "9198", "c1c8")
  return(types[seq_len(n)])
}

base + scale_linetype(palette = linetypes)

I guess this is due to some of the changes in discrete_scale()?

Thanks.

@py9mrg
Copy link
Author

py9mrg commented Jul 10, 2024

Ugh, sorry, had this repo and book repo open at same time - used the wrong one.

@py9mrg py9mrg closed this as completed Jul 10, 2024
@py9mrg
Copy link
Author

py9mrg commented Jul 10, 2024

Actually, I'll reopen this because as far as I can tell this is a bug with ggplot2 not with the book code specifically.

@teunbrand
Copy link
Collaborator

Thanks for the report!

I guess the reprex for the problem is as follows:

library(ggplot2)

base <- ggplot(economics_long, aes(date, value01, linetype = variable)) +
  geom_line()

ltys <- c("55",  "75", "95", "1115", "111115", "11111115",
          "5158", "9198", "c1c8")

linetypes <- function(n) {
  return(ltys[seq_len(n)])
}

base + scale_linetype(palette = linetypes)
#> Error in discrete_scale("linetype", name = name, palette = pal_linetype(), : formal argument "palette" matched by multiple actual arguments

The plot above should yield:

base + scale_linetype_manual(values = ltys)

Created on 2024-07-10 with reprex v2.1.0

@teunbrand
Copy link
Collaborator

Might be best to fix this in #5946, as it touches the same code.

@teunbrand
Copy link
Collaborator

On second thought: no the book is wrong and the only thing ggplot2 should change is dedocument the palette parameter from scales that prepopulate them.

These problems started occurring after #5343 when we started to provide the palette() argument explicitly (rather than with unnamed arguments matched by position)

If we look at the scale_linetype() definition from ggplot2 3.4.3, we see the following:

> ggplot2::scale_linetype
function (..., na.value = "blank") 
{
    discrete_scale("linetype", "linetype_d", linetype_pal(), 
        na.value = na.value, ...)
}

So if we declare palette = custom_palette, that will pass through ... of discrete_scale(). Because we now have a named palette argument, the 3rd argument (linetype_pal()) will now get passed to the 4th argument (name). It is only because the plot has turned off the legend (in the linked line below) that the name argument never needs to be rendered and isn't causing any problems.

https://github.com/hadley/ggplot2-book/blob/223989dcb7e88f28c4f567e7d22fdb7a7bee62bb/scales-other.qmd#L246

So, throwing errors when we have an extra palette argument that won't be accepted, as we do in the current version of ggplot2, is correct.

@py9mrg
Copy link
Author

py9mrg commented Jul 10, 2024

Aaaah I see. Ok thank you, I'll manually fix the book for now and leave my issue up there. Thanks for your help.

@py9mrg py9mrg closed this as completed Jul 10, 2024
@teunbrand
Copy link
Collaborator

I'm going to re-open this to adress the documentation issue:

the only thing ggplot2 should change is dedocument the palette parameter from scales that prepopulate them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants