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

fix: obtain expected quantity values from manual scale #5364

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

liang09255
Copy link

This PR fixes an error in the code and adds corresponding unit tests.

When using manual_scale to create a discrete_scale, the palette should be a function that can return the specified number of values when called with a single integer argument.

Here is the parameter description excerpt from scale-.R:

@param palette A palette function that when called with a single integer argument (the number of levels in the scale) returns the values that they should take (e.g., [scales::hue_pal()]).

In the original manual_scale method, it simply returned all the values. However, when values are not named, we only need to return the corresponding first 'n' values. Therefore, the modified code includes a condition to handle this case.

@teunbrand
Copy link
Collaborator

teunbrand commented Jul 26, 2023

The case where a palette returns more values than requested by n is handled internally in ScaleDiscrete$map(). Aside from the manual palette, this also handles other cases for e.g. custom palettes:

library(ggplot2)

ggplot(mpg, aes(displ, hwy, colour = factor(cyl))) +
  geom_point() +
  discrete_scale(
    "colour", "colour",
    palette = function(n) rainbow(10) # more than the 4 needed for `factor(cyl)`
  )

Created on 2023-07-26 with reprex v2.0.2

What I'm not understanding here is why this should be fixed if this case is dealt with already.

@liang09255
Copy link
Author

Thank you for your patient explanation, i realize that the reason why i think it is a bug is that i incorrectly use "scale_color_manual"

But I still believe that this PR can improve the robustness of the code without affecting other functions

library(ggplot2)
library(ggsci)


plot_palette <- function(name){
  if (name %in% c("npg", "jco", "igv")){
    eval(parse(text = sprintf("ggsci::scale_colour_%s()", name)))
  }else if (name == "traffic_light"){
    # The reason why i make this mistake is i wrongly think 
    # "scale_color_manual" is the wrapped for "discrete_scale" 
    # which can create a color palette conveniently
    
    # This is my incorrect code causing a bug
    scale_color_manual(values = c("red", "green", "yellow", "blue", "black"))
    
    # This is the correct code
    # discrete_scale("color", "color", function(n){
    #   colors <- c("red", "green", "yellow", "blue", "black")
    #   colors[1:n]
    # })
  }else{
    stop("wrong name") 
  }
}

palette_name <- "traffic_light"
# palette_name <- "jco"

palette <- plot_palette(palette_name)

# when i using my function, it can correctly draw the image
ggplot(mpg, aes(displ, hwy, colour = factor(cyl))) +
  geom_point() +
  palette

# When I try to output the colors used in the drawing, it output all of the colors
cat(palette$palette(length(unique(mpg$cyl))))

# traffice_light output all the color: 
#   red green yellow blue black

# jco output first 4 color: 
#   #0073C2FF #EFC000FF #868686FF #CD534CFF

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

Successfully merging this pull request may close these issues.

None yet

2 participants