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

add function scale_label_identity function #5389

Closed
wants to merge 2 commits into from

Conversation

Yunuuuu
Copy link

@Yunuuuu Yunuuuu commented Aug 12, 2023

Through these commits, we are enabled to effortlessly exhibit legend labeling using the guide parameter within the scale_label_identity function, aligning it with analogous aesthetic settings for other geoms. Furthermore, the assignment of the label key can be readily accomplished through the intrinsic data within the draw_key_ function (#2004). This bring scale_label_identity function and catch following concers:

  1. label as a geom aesthetic should appear in the scaled data from draw_key_text function. As we can see with following code
library(ggplot2)
# Create sample data
data <- data.frame(
    x = c(1, 2, 3, 4, 5),
    y = c(1, 2, 3, 4, 5),
    label = c("A", "B", "C", "D", "E")
)
# Create the plot with geom_text
ggplot(data, aes(x, y)) +
    geom_text(aes(label = label, color = label),
        key_glyph = function(data, params, size) {
            browser()
        }
    )

image

  1. users are inclined to modify the parameter show.legend to exhibit textual aesthetics (only to discover that it cannot fulfill their intended purpose), while the help document means the default value of show.legend shoule have include the mapping aesthetic:
show.legend: logical. Should this layer be included in the legends? NA, the default, includes if any aesthetics are mapped. FALSE never includes, and TRUE always includes. It can also be a named logical vector to finely select the aesthetics to display.
  1. The hard-coded label in the draw_key_text function appears superfluous.

@@ -236,7 +236,6 @@ draw_key_smooth <- function(data, params, size) {
#' @export
#' @rdname draw_key
draw_key_text <- function(data, params, size) {
if(is.null(data$label)) data$label <- "a"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little bit worried with this line.
I fear that the following code wouldn't draw labels in the legend:

ggplot(mtcars, aes(mpg, disp)) +
  geom_text(aes(
    label = rownames(mtcars), 
    colour = factor(cyl)
  ))

Could you confirm and amend if necessary?

@teunbrand
Copy link
Collaborator

Overall I'm a little bit cautious about the need for such function, which might have been fruitful to discuss in an issue before posting a PR. Labels typically aren't mapped to a scale and introducing scale_label_identity() might prompt the expectation of various other label scales. I don't think the identity scale is a problem, but the other ones are tricky.

@Yunuuuu
Copy link
Author

Yunuuuu commented Aug 17, 2023

Thanks for your comment, I wanted bring it into ggplot2 package for these issues: #2004, https://stackoverflow.com/questions/49965758/change-geom-texts-default-a-legend-to-label-string-itself?noredirect=1&lq=1, https://stackoverflow.com/questions/18337653/remove-a-from-legend-when-using-aesthetics-and-geom-text?noredirect=1&lq=1, https://stackoverflow.com/questions/59091627/add-new-legend-for-geom-text-with-text-labels-as-legend-key, without scale_label function, the data in draw_key_text function will not contain the label informations, which has prompted users to modify the internal grob object or GEOM object which shouldn't be touched in usual.

image

@Yunuuuu
Copy link
Author

Yunuuuu commented Aug 17, 2023

I have noticed that the existence of the scale_label function leads to the incorporation of the label data within the draw_key_text function. Evidently, many users, including myself, have encountered difficulties with the hard-coded "a" in the textual legend. I have conducted a search on Stack Overflow and found several questions that are similar to this issue.

@Yunuuuu
Copy link
Author

Yunuuuu commented Aug 17, 2023

Perhaps it might be advisable not to alter the draw_key_text function. Instead, could we explore the possibility of integrating the scale_label_identify function? Alternatively, are there any superior approaches to address the hardcoded "a" issue?

@Yunuuuu
Copy link
Author

Yunuuuu commented Aug 17, 2023

It has come to my attention that the current draw_key_text function is expected to include the scaled label information as outlined in the documentation

#' @param data A single row data frame containing the scaled aesthetics to

@teunbrand
Copy link
Collaborator

I'll admit that the current way of dealing with this is not ideal. I'd say the recommended route without this PR would be to use the override.aes argument of the guide and manually relabelling it.

library(ggplot2)
#> Warning: package 'ggplot2' was built under R version 4.3.1

data <- data.frame(
  x = c(1, 2, 3, 4, 5),
  y = c(1, 2, 3, 4, 5),
  label = c("A", "B", "C", "D", "E")
)

ggplot(data, aes(x, y)) +
  geom_text(aes(label = label, color = label)) +
  guides(colour = guide_legend(override.aes = list(label = LETTERS[1:5])))

You could still have an identity scale, but you'd need to set the guide for it to work as expected.

scale_label_identity <- function(..., guide = "none") {
  sc <- discrete_scale("label", "identity", force, ...,
                       guide = guide,
                       super = ScaleDiscreteIdentity
  )
  
  sc
}

ggplot(data, aes(x, y)) +
  geom_text(aes(label = label, color = label)) +
  scale_label_identity(guide = "legend")

Created on 2023-08-17 with reprex v2.0.2

It has come to my attention that the current draw_key_text function is expected to include the scaled label information as outlined in the documentation

There is a subtle nuance in what is 'mapped' and what is 'scaled'. The label aesthetic is typically mapped but not scaled.

@Yunuuuu
Copy link
Author

Yunuuuu commented Aug 17, 2023

Thank you for your explanations. I now realize the differences between "map" and "scale". Given this understanding, it might not be advisable to include such a function in this context. For me, It'll be okay to close the pull request.

@yutannihilation
Copy link
Member

As it seems closing this pull request is the conclusion here, I'm closing now.

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.

3 participants